| Age | Commit message (Collapse) | Author |
|---|
|
With 7e87131ed (golangci-lint: Allow `testing.T` as first parameter,
2022-08-10) we set `context-as-argument` the sole argument to
`revive:rules`. This causes golangci-lint to disable all revive lints
that were previously enabled by default.
Re-enable the default revive lints by explicitly listing them in our
config. This triggers a large number of lint errors, resolve these as
well in this commit.
|
|
Refactor remaining users of the ioutil package
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4899
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
|
|
Go 1.15 has deprecated the `ioutil` package and made replacements
available in the `io` or `os` packages. While we have already migrated
most callsites away, one notable omission was `ioutil.ReadDir()` as this
function doesn't have an exact replacement: the new `os.ReadDir()`
function does not stat all directory entries anymore.
Refactor the cleanup of stale cached files in the `cache` package to use
the new function.
|
|
Go 1.15 has deprecated the `ioutil` package and made replacements
available in the `io` or `os` packages. While we have already migrated
most callsites away, one notable omission was `ioutil.ReadDir()` as this
function doesn't have an exact replacement: the new `os.ReadDir()`
function does not stat all directory entries anymore.
Refactor the cleanup of stale leases in the `cache` package to use the
new function.
|
|
Go 1.15 has deprecated the `ioutil` package and made replacements
available in the `io` or `os` packages. While we have already migrated
most callsites away, one notable omission was `ioutil.ReadDir()` as this
function doesn't have an exact replacement: the new `os.ReadDir()`
function does not stat all directory entries anymore.
Refactor the cleanup of stale keep-around references in the
`GarbageCollect` RPC to use the new function.
|
|
Go 1.15 has deprecated the `ioutil` package and made replacements
available in the `io` or `os` packages. While we have already migrated
most callsites away, one notable omission was `ioutil.ReadDir()` as this
function doesn't have an exact replacement: the new `os.ReadDir()`
function does not stat all directory entries anymore.
Refactor the cleanup of stale worktrees in the `housekeeping` package
to use the new function.
|
|
Go 1.15 has deprecated the `ioutil` package and made replacements
available in the `io` or `os` packages. While we have already migrated
most callsites away, one notable omission was `ioutil.ReadDir()` as this
function doesn't have an exact replacement: the new `os.ReadDir()`
function does not stat all directory entries anymore.
Refactor `stats.GetPackfiles()` to use the new function and manually
stat the packfiles at callsites as required.
|
|
Go 1.15 has deprecated the `ioutil` package and made replacements
available in the `io` or `os` packages. While we have already migrated
most callsites away, one notable omission was `ioutil.ReadDir()` as this
function doesn't have an exact replacement: the new `os.ReadDir()`
function does not stat all directory entries anymore.
Refactor the `tempdir` package's clean walker to use the new function
and manually stat the directory entries as required.
|
|
[ci skip]
|
|
[ci skip]
|
|
[ci skip]
|
|
proto: Deprecate embedded errors
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4894
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
|
|
While the embedded error in the UserDeleteBranchResponse message has
been deprecated already, there is no information when the field will be
removed. Add it so we don't forget to clean it up.
|
|
The UserCherryPick RPC was converted to return structured errors instead
of embedding errors in a successful response. These embedded errors are
thus never set by Gitaly anymore and should be phased out.
Mark them as deprecated and slate them for removal in release 16.0.
Change: deprecated
|
|
The UserCreateBranch RPC was converted to return structured errors
instead of embedding errors in a successful response. These embedded
errors are thus never set by Gitaly anymore and should be phased out.
Mark them as deprecated and slate them for removal in release 16.0.
Change: deprecated
|
|
The UserCreateTag RPC was converted to return structured errors instead
of embedding errors in a successful response. These embedded errors are
thus never set by Gitaly anymore and should be phased out.
Mark them as deprecated and slate them for removal in release 16.0.
Change: deprecated
|
|
praefect: fix race condition on logger
Closes #4457 and #4473
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4886
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>
|
|
The `praefect` command has a global logger that is passed to any
subcommands executed. When executing normally this is fine as each
process will only run a single subcommand. However, in testing this
creates a risk of race conditions when tests are run in parallel, as
each test goroutine will share this logger.
To resolve race detector failures on `TestAddRepositories_Exec()` and
`TestAddRepositories_Exec()`, we will now run these tests serially. This
has minimal impact on test execution time, increasing from 4.663s to
5.129s.
In the medium-term we should replace the global logger with a local that
allows parallel testing. Issue #4500[0] has been opened for this.
[0] https://gitlab.com/gitlab-org/gitaly/-/issues/4500
|
|
make: use 'override' when adding to TEST_OPTIONS
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4890
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>
|
|
operations: Always use structured errors in UserCreateTag RPC
Closes #4372
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4882
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
doc: Clarify GitLab/Gitaly boundary for RPCs
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4878
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Andras Horvath <ahorvath@gitlab.com>
Approved-by: Mark Wood <mwood@gitlab.com>
|
|
Bump .tool-versions to use Go 1.18.6
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4892
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Stan Hu <stanhu@gmail.com>
|
|
The GitLab Development Kit is moving to 1.18.6, so let's make all the
versions consistent to reduce the number of interpreters needed for
development.
Part of https://gitlab.com/groups/gitlab-org/-/epics/8843
|
|
go: Update module github.com/google/go-cmp to v0.5.9
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4863
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
When `TEST_OPTIONS` is specified as an argument to `make`, any options
specified in the `Makefile` will be overwritten with the user-provided
value. In several targets we append arguments to `TEST_OPTIONS`, making
them vulnerable to being squashed inadvertently.
For example, if running `make race-go TEST_OPTIONS='-run=<TEST>
-count=20'` to reproduce a race condition, the intended command would
be:
gotestsum --format short -- -run <TEST> -count 20 -race ./...
But because `TEST_OPTIONS` is specified, this is actually executed as:
gotestsum --format short -- -run <TEST> -count 20 ./...
Resolve this by using the `override` directive in targets that attempt
to append to `TEST_OPTIONS`. This will allow the user to add custom
options without squashing the target's additions.
|
|
'4455-feature-flag-roll-out-user_create_branch_structured_errors' into 'master'
operations: Structured errors in UserCreateBranch
Closes #4455
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4883
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
Revert "Merge branch...
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4888
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: karthik nayak <knayak@gitlab.com>
|
|
objectpool: Always prune refs on fetches to fix D/F conflicts
Closes #4394
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4885
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
|
|
With 018958fb1 (objectpool: Fix conflicting references when fetching
into pools, 2022-07-27) we have started to prune references in object
pools that have been removed in their primary pool member. This is a
necessity to avoid conflicting references in the object pool and its
member, which would otherwise break updating such pools completely.
This code has been rolled out to production on August 11th without any
observed negative fallout, and the flag was subsequently enabled by
default. Remove the feature flag guarding it.
Changelog: changed
|
|
golangci-lint: Stop excluding many `Close` and `Serve` functions from errcheck linter
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4810
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
|
|
'4452-feature-flag-roll-out-simplify_find_local_branches_response' into 'master'"
This reverts merge request !4884
|
|
We don't properly verify the error code when closing the file that
corresponds to the packed binary. While this doesn't really matter in
the first place, fixing this one case lets us drop another exception to
our linting rules.
|
|
Many of our tests don't properly verify the error returned by the server
factory's `Serve()` function. Fix this shortcoming, which surfaces that
we mistakenly close some listeners manually that should indeed be closed
by gRPC itself. Drop the corresponding linting exception.
|
|
The `http.Server.Serve()` function will return an error when the HTTP
server has either failed unexpectedly or in case it wasn't properly shut
down. We don't check this error in many places though, which both causes
the errcheck linter to complain and hides issues we have with some of
our test setups.
Fix the missing error checks via a new `testhelper.MustServe()` helper
function that does the error checking for us. Drop the corresponding
linting exceptions.
|
|
The `grpc.Server.Serve()` function will return an error when the gRPC
server has either failed unexpectedly or in case it wasn't properly shut
down. We don't check this error in many places though, which both causes
the errcheck linter to complain and hides issues we have with some of
our test setups.
Fix the missing error checks via a new `testhelper.MustServe()` helper
function that does the error checking for us. This surfaces some test
errors for tests that forget to shut down the server or that manually
close the listener, which should be handled by gRPC itself. All of those
errors are fixed while at it.
Drop the corresponding linting exceptions.
|
|
We don't properly check the error when closing the `SidechannelWaiter`
in many places. Fix this and drop the corresponding linting exceptions.
|
|
The `close()` function we return as part of the server handshake is
responsible for closing all associated resources. In case closing any of
them fails though we never bubble up errors at all, but instead silently
succeed.
Fix this and return the first error we encounter. As we now correctly
check returned errors the errcheck linter is happy and we can drop
another exclude for the `yamux.Session` type.
|
|
We already check the error when we manually close the zlib writer, but
don't in the deferred call. This is fine though at it won't do anything
in the happy path as the writer was closed already, and we already got
an error in the unhappy path.
Explicitly ignore the error to make our linters happy.
|
|
While we check error codes returned by the tar writer in the general
case when writing an archive, we ignore it in case we have already seen
an error in `TarBuilder.Close()`. We didn't explicitly ignore the error
with a blanket-assignment though, and thus the errcheck linter
complains.
Fix this by assigning the error to the blanket variable and drop the
corresponding exclude from our linting rules.
|
|
Check error codes when closing a `safe.LockingFileWriter` or
`safe.Writer` and drop the corresponding exclude from our linting rules.
|
|
Check the error code when closing the Unix socket listener and drop the
corresponding exclude from our linting rules.
|
|
The errcheck linter is tasked with finding cases where we are mistakenly
not checking errors returned by a function. While we have it globally
enabled, one gap that we currently have is that we exclude all function
calls to `Close` and `Serve`. This exception is something we added back
when we added this linter to our rules as it has proven to be too much
work to fix all violations in the code base. Not checking any such
errors may easily hide issues we're not aware of though and is thus bad
practice.
Convert the blanket-exclude for all functions to instead explicitly
exclude all functions that currently violate it. Like this, it becomes a
lot more manageable to fix missing error checks one function at a time
and makes sure that we don't add new violations to our codebase that are
not covered by any of these explicit function excludes.
|
|
'4452-feature-flag-roll-out-simplify_find_local_branches_response' into 'master'
ff: set SimplifyFindLocalBranchesResponse to true
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4884
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
With 2e642ccd (branches: Use 'UserCreateBranchError' behind a flag) we
have started using structured errors for the UserCreateBranch RPC
instead of relying on `Internal` errors.
This code was rolled out to production and the feature flag is currently
enabled by default. Let's remove the feature flag guarding it.
Changelog: changed
|
|
ref: Always use structured errors in FindTag RPC
Closes #4398
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4881
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Since this featureflag has a rails/gitlab counterpart we cannot simply
delete it. We need to ensure that both Gitaly and rails have it set to
true by default for one release and then we can delete the feature flag
in the next release.
|
|
With 0fedeb41f (operations: Introduce structured errors for
UserCreateTag, 2022-07-22) we have adapted the UserCreateTag RPC to
return structured errors instead of embedding errors in a successful
response. This both improves visibility into this RPC and fixes cases
where we needlessly create replication jobs due to missing votes.
This code has been rolled out to production on August 3rd without any
observed negative fallout. Remove the feature flag guarding it.
Changelog: changed
|
|
With 453ac57fc (ref: Fix `Internal` errors in `FindTag()` when tag
doesn't exist, 2022-08-01) we have adapted the FindTag RPC to return a
`NotFound` error instead of an `Internal` one when the tag wasn't found
in order to clearly label this error condition as expected.
This code has been rolled out to production on August 2nd without any
observed negative fallout. Remove the feature flag guarding it.
Changelog: changed
|
|
Makefile: Update Delve
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4879
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Toon Claes <toon@gitlab.com>
|
