| Age | Commit message (Collapse) | Author |
|---|
|
Remove the deprecated deadcode, structcheck and varcheck linters. All
of them have been replaced by the unused linter that we've already got
enabled anyway.
|
|
We don't typically have package-level comments, even though we arguably
should. And while we don't have any such comments in the general case,
the Revive rule never complained about them. This is because the
reported confidence was setto 0.2, and as a result it never got printed.
This was fixed via Revive v1.2.2 so that these violations are now
reported. Let's thus disable the rule.
|
|
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.
|
|
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.
|
|
The `thelper` linter has a rule that will flag test helper functions
that don't call `t.Helper()`. Unfortunately, this will also flag all
kinds of functions that really shouldn't have this call, which makes the
linter effectively useless for us.
Document why we disable it to not keep folks wondering.
|
|
Enforce consistent naming of `testing.TB` variables, which should be
called `tb`, and adapt tests that violate this rule.
|
|
Enforce that `testing.T` et al must be the first parameter of test
functions. Unfortuntately, the linter explicitly allows for contexts to
precede `testing.T`, which causes us to still not enforce a uniform
style for our test functions.
Fix cases where we violate this rule.
|
|
Add the `thelper` linter, which knows to enforce consistent code styles
around tests:
- `testing.T` should be the first parameter in test-related
functions.
- Functions called by tests should call `t.Helper()`.
- `testing.T`, `testing.B` and testing.TB` variables should be
consistently named as `t`, `b`, and `tb`, respectively.
Right now we still have all rules disabled that would trigger warnings.
|
|
We currently exclude a revive rule that `context.Context` should be the
first parameter for our test sources. This can be handled better though
because golangci-lint allows us to exclude certain types from this rule.
Adapt the rule to allow `testing.T` et al before `context.Context` and
remove the excluded rule. Interestingly, this now surfaces a whole bunch
of `nolint: revive` annotations that aren't needed anymore, so we fix
them in the same commit.
|
|
We have recently started to see that golangci-lint is timing out more
frequently. Let's bump its timeout to 10 minutes to stop this from
happening.
|
|
Rewrite callers of `testhelper.ModifyEnviroment()`, which is about to be
removed in favor of either `t.Setenv()` or `testhelper.Unsetenv()`.
|
|
Disallow use of `os.Setenv()` and `os.Unsetenv()` in tests. Callers
should instead use `testhelper.ModifyEnvironment()`. Adjust existing
callers to do so. Note: the log tests cannot use the testhelper package
due to a cyclic import and thus use `t.Setenv()` directly.
|
|
Remove an unused golangci-lint linting exemption.
|
|
There's several locations where we don't verify that the `Kill()`
syscall succeeded. Move the linting exceptions we have in our
golangci-lint configuration inline to clearly indicate that this is
something we may want to fix in the future.
|
|
Both the local and SQL electors don't check errors returned by
`checkNodes()`. While we could convert all sites to properly do this, it
doesn't really feel worth it: both electors only exist due to legacy
reasons and should not be used in production systems at all.
Ignore most of those errors inline so that we can remove the excemptions
from golangci-lint's config file.
|
|
We currently ignore any errors when waiting for the supervised command
fails. Log the error so that we at least have a chance to see that
something may have gone wrong.
|
|
Our testhelper context has recently gained the ability to verify that
feature flags are exercised as expected. This is an important safety
guard which can help us to avoid releasing untested and thus potentially
buggy code.
Disallow usage of "normal" contexts which don't have this ability such
that we do not introduce their usage by accident.
|
|
Much of the flakiness we're seeing every day is caused by timeouts which
were too tight. This is only natural: our own developer machines are
much beefier than the CI runners we use, and furthermore they typically
don't suffer from noisy neighbours. So any timeout that works for our
machine is likely to not work for CI workers. It is questionable why
we'd want to have timeouts in the first place: most tests really
shouldn't verify how fast a certain operation is, but instead they
should verify that it does what we expect it to do. Otherwise, if one
cares about speed, one should write a benchmark instead.
One valid reason is to detect tests which are stuck completely, but the
Go runtime handles this for us already with the default 10 minute
timeout for tests. Other usecases which want to test for example
cancellation should really try hard to avoid using timeouts for this,
but instead the system under test should be designed in a way that makes
it possible to easily test it, for example by using manual tickers which
can be injected from a test.
Forbid usage of a subset of functions which we historically used for
creating contexts with timeouts. Most of the tests which used this have
been refactored to not do so anymore, with the exception of some tests
which explicitly want to exercise deadlines themselves.
This is only a first step: we also want to discourage the use of
`time.After()` and `time.Sleep()`. These are left for a different commit
series though.
|
|
While gofumpt will by default check for possible code simplifications
when run directly, its corresponding linter for golangci-lint doesn't
seem to do so. As a result, linting may not complain about misformatted
code while running the formatter would still perform reformatting.
Fix this by using both `gofmt` and `gofumpt` linters: `gofmt` by default
has simplifications enabled, and `gofumpt` will verify the extra rules
which are on top of `gofmt`.
|
|
By default, stylecheck checks for missing package comments. Given that
we don't really use these, this of course generates a lot of linting
violations for us. Because of this, we have added an exclude rule to
just skip over those violations completely. Excludes aren't the correct
way to go about this, though: the stylecheck linter can be configured to
skip specific checks, which is what we should be doing instead.
Convert the exclude rule to stylecheck configuration.
|
|
Linters and their settings are torn apart by our exclude rules. Let's
move them together to make the settings easier to discover.
|
|
Most of the linting exception we have are about missing documentation of
public variables or comments. Let's inline all of these -- it's hard to
keep track of exceptions and keep the list up-to-date. By having a
`//nolint` directive at the site where the document is missing it
becomes a breeze to directly clean it up. Furthermore, it also acts as a
reminder that one might want to add a comment when one stumbles over any
of these comments.
|
|
Clean up various exceptions for the errcheck linter, either by removing
exceptions which don't apply anymore, by handling the errors or by
ignoring errors.
|
|
The noctx linter labels code which should use a context but doesn't.
We've got a single location in our tests which violate this linter,
which is why we have an exception for this linter in place.
Fix this callsite and remove the exception.
|
|
Remove the unused exception for the "maligned" linter.
|
|
The "old_path" and "new_path" fields of the GetRawChanges responses have
been deprecated ever since we have moved the protobuf definitions into
our repo in b9c9aec5c (Start preparation for migrating .proto files,
2019-07-05). They had originally been deprecated given that they were
using strings as types, and invalid Unicode characters in strings get
dropped when serializing and deserializing Protobuf messages. They had
thus been replaced with two fields "old_path_bytes" and "new_path_bytes"
and downstream callers have long been converted to use these.
Let's drop the old and deprecated fields -- they're not used by
anything, and neither should they ever be used.
|
|
We've added a special exclude rule for the govet linter such that it
doesn't bug us about use of unkeyed fields in composite literals. We
only have two callsites which get this wrong though, and using field
names is preferable such that we don't by mistake assign a value to the
wrong field.
Reenable this linter and fix the two violations.
|
|
With golangci-lint v1.43, a new bidichk linter has been added. This
linter checks sources for bidirectional control characters which can be
abused to conceal adversary code behind seemingly benign code. This
issue (CVE-2021-42694, Trojan Source [1]), makes it easy for an
adversary to get changes through code review.
Enable the bidichk linter to detect any use of such characters to
protect us against such supply chain attacks.
[1]: https://trojansource.codes/
|
|
The "io/ioutil" package has been deprecated in Go 1.16 and thus
shouldn't be used in our codebase anymore. Disallow its usage by adding
the "depguard" linter with a single entry for this specific package.
Note that we still use the package in some places where we used to rely
on `ioutil.ReadDir()`'s old behaviour of stat'ting all directory
entries. Conversion of these is not part of this patch series given that
it would be a change in behaviour compared to all the other changes
which aren't. Those sites are thus annotated with `nolint` directives.
|
|
Add the gofumpt linting rule to golangci. This allows us to replace our
"check-formatting" Makefile target with a run of golangci-lint.
|
|
The golint linter has been deprecated by upstream and is thus also
marked as deprecated by golangci-lint. Replace it with the newer revive
linter, which has a superset of linters of golint.
|
|
The change removes out.Flush from lint rules exclusion
and fixes all the places where it causes the issue.
It affects only gitaly-debug binary, so it has no impact
on the production code.
The change adds a couple of helper functions that now used
in places where error were not checked, but should be.
|
|
The change removes os.Remove from lint rules exclusion
and fixes all the places where it causes the issue.
If removal ends up with an error we try to log it if the
logger is accessible. Otherwise the error is just omitted.
We don't return it back to the caller because we don't want
functional changes here.
|
|
The change removes os.RemoveAll from lint rules exclusion
and fixes all the places where it causes the issue.
If removal ends up with an error we try to log it if the
logger is accessible. Otherwise the error is just omitted.
We don't return it back to the caller because we don't want
functional changes here.
|
|
The change removes io.Copy from lint rules exclusion
and fixes all the places where it causes the issue.
|
|
The dependency github.com/golang/protobuf is deprecated and should
be replaced with google.golang.org/protobuf.
This change replaces usage of the ptypes package. The replacement
packages has a different structure and it requires code changes
to be done to migrate.
The exclusion rules removed from the golanci-lint tool configuration
file to prevent future use of the deprecated package.
|
|
The dependency github.com/golang/protobuf is deprecated and should
be replaced with google.golang.org/protobuf.
This change replaces usage of the proto package. The packages have
incompatible API that is why the code changes were done to migrate.
The exclusion rule removed from the golanci-lint tool configuration
file to prevent future use of the deprecated package.
|
|
The dependency github.com/golang/protobuf is deprecated and should
be replaced with google.golang.org/protobuf.
This change replaces usage of the jsonpb package with encoding/protojson.
The packages have incompatible API that is why the code changes
were done to migrate.
The exclusion rule removed from the golanci-lint tool configuration file
to prevent future use of the deprecated package.
|
|
Except for a single use in Praefec's tests, the "storage" module is only
used by Gitaly. Despite that, it's still located at `internal/storage`
and thus indicates that it may be a generic component for reuse.
Move it into `internal/gitaly` to clarify that it is indeed an
implementation detail of Gitaly, only.
|
|
We're about to extend dependencies required to run gitaly-git2go
subcommands, which will make calling them tedious. This commit thus
refactors the revert subcommand to be implemented on top of the
`Executor` structure, which encapsulates all required dependencies and
thus avoids the additional complexity.
|
