| Age | Commit message (Collapse) | Author |
|---|
|
The `errorlint` lint check is used to find code that will cause problems
with the error wrapping scheme introduced in Go 1.13. With the fixes in
the previous commits, we can finally enable this linter.
|
|
|
|
Disallow importing the public `client` package, which is only intended
to be used by external clients. This brings multiple advantages:
- We have a clean boundary between public and internal code. This
ensures that we don't accidentally add interfaces to our public
packages that really are low-level details.
- It is easier to remain backwards compatibility as we now have two
distinct layers.
- We can perform internal refactorings more readily and iterate on
how things are handled internally without breaking the public API.
Other public packages will eventually follow to be added to the list of
disallowed imports.
|
|
With our migration to use the internal pool functionality we have also
converted all callsites to use `testhelper.MustClose()` to close the
pool with proper error checking in place. Because of that, we only have
a single callsite left where we don't check the error code when closing
the pools, which thus makes our linter rule superfluous where we don't
require error checks for that function.
Remove the rule and add error checking to the last remaining callsite
that doesn't.
|
|
Our tests will check for leaked goroutines, but only if `testhelper.Run`
is called in the package being tested. A recent production outage was
caused by leaked goroutines which our tests did not catch as the package
did not use the required helper.
Create a custom linter to validate the following:
- Each package that has tests also contains a `TestMain`.
- `TestMain` calls `testhelper.Run`.
- `TestMain` is located in a file named `testhelper_test.go`.
One limitation is that the `_test` package is scanned in a separate pass
from the primary package, preventing us from validating that either the
primary package or its `_test` package has defined `TestMain`. By
default each analysis pass has no knowledge of what other packages have
or will be scanned.
To avoid accidentally requiring `TestMain` to be defined twice, we skip
linting any package with a name ending with `_test`. However, this
potentially leads to coverage gaps if all tests are in the `_test`
package. We would pass the primary package as it has no tests, and then
skip checking the test package based on its name, missing that it did
not execute `TestMain`.
A future iteration of this linter should use `Facts` to pass information
on package state between passes, but this first implementation gets us
90% of the value.
|
|
Disallow importing the labkit logger package in favor of using our own
`internal/log` package.
|
|
We have forbidden a bunch of different logging statements with the hint
that clients should either use `log.Default()` or `ctxlogrus.Extract()`.
The former is essentially deprecated though in favor of using injected
loggers.
Rephrase the hints to reflect that.
|
|
The `With` functions provided by logrus use the standard logger provided
by the logrus package. It is discouraged to use it though in favor of
either `internal/log.Default()` or the logger that is part of the RPC
context.
Disallow the use of those functions via golangci-lint's forbidigo
linter.
|
|
The regular expression we have in place to disallow several logrus calls
was using a "," to delimit alternatives, while it should have been using
"|". Fix this.
|
|
Globally disable the use of `logrus.New()`. Production code should
obtain a logger via either `log.Default()` or `ctxlogrus.Extract()`,
, while test code should be using `testhelper.NewDiscardingLogEntry()`.
|
|
Forbid the use of the standard logrus logger in favor of using our own
logger accessible via the `internal/log` package.
|
|
We have some forbidigo rules in place that disallow the usage some
context functions in our tests. This is realized by excluding all files
from the rule that do not end in "_test.go". Unfortunately though, this
keeps us from using the linter for actual production code, which is
arguably more important.
As we cannot both have our cake and eat it, too, let's drop our current
rules. We will add new rules that apply to production code in the next
commit.
|
|
The prior commit fixes inelligible usages of Unavailable status codes.
To prevent this situation from happening in the future, this commit
implements a linter that it warns occurrences where Unavailable code is
used. Engineers can ignore it by adding a comment, but at least this
practice catches their eyes.
|
|
Update golangci-lint to the latest version. This also updates depguard
to the latest version, so we need to modify our config to match the new
version of depguard.
|
|
We've grown quite a bunch of packages that provide various different
functionality in the context of gRPC. But it is not easy to identify
them as there is no common hierarchy for gRPC-related functionality.
We're thus introducing a new `internal/grpc` package that will host
various bits and pieces related to gRPC.
Move the `internal/sidechannel` package into this new package.
|
|
We're about to release Gitaly v16.0. As we've landed a bunch of
previously announced removals it's thus time to bump our Go module
version from v15 to v16.
|
|
The introduction of generics in Go 1.18 caused some of the linters to
stop working correctly. These linters have now been fixed, so let's
reenable them.
While at it, sort the gitaly-linters linter alphabetically as is the
case for all the other linters.
|
|
Start using the `errname` linter, which enforces idiomatic names for
error types:
- Error types end in `Error`, e.g. `DecodeError`.
- Error variables start with `Err`, e.g. `ErrUnsupportedAlgorithm`.
Let's fully adopt this idiomatic convention by enabling the linter.
Fix code that currently violates these rules.
|
|
Since the introduction of the `t.Setenv()` helpers in Go 1.17 it has
become discourages to use `os.Setenv()` in tests directly. This is
because `t.Setenv()` has additional safeguards that cause the test to
fail if called in a parallel test as setting environment variables is
not thread-safe.
We enforce this rule via a set of custom forbidigo rules. But since
adding those rules, a new `tenv` linter has been added to golangci-lint
that supports exactly that usecase.
Use the new `tenv` linter and drop our own custom rules.
|
|
|
|
In a prior change
(https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5398), we added
the support for custom linters on top of golangci-lint. In the first
iteration, all settings for them are put in the implemenation file.
This commit moves those settings to the manifeset file, along side with
other linter settings.
golangci-lint doesn't support an official way to load the settings in
de-facto manifest file `.golangci.yml`. Fortunately, we can hook into
configuration parsing library `viper` that golangci-lint uses. Our
custom linters are loaded at the end, just before executing the
commands. At this time, golangci-lint established some state that stores
the configuration. This state is process global, and accessible in our
source code. In the future, if this approach doesn't work anymore, we
can consider parsing the configuration file ourselves.
|
|
This commit adds the infrastructure to support custom linters.
Unfortunately, golangci-lint doesn't support dynamic linter. It means
that we need to compile each linter with `-buildmode=plugin` flag. The
compiled artifacts are then configured in `.golangci.yml` file. New
linter implementation follows the guide in this lint:
https://golangci-lint.run/contributing/new-linters/
Implementing this linter requires the following steps:
* Add analyzers implemting the checks to tools/gitaly-linters package
* Add the new checks to `lint.go` file
* Re-compile via `make gitaly-linters` or `make lint`
|
|
In the last few commits we reinitialized the test variable wherever
needed and also fixed tests. This is due to a bug/feature around how Go
treats looped
variables (https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721).
To avoid this mistake in the future, let's add a linter check around
this issue. This is done by adding the `paralleltest` linter. We also
enable `ignore-missing` because we don't want to complain about missing
usage of `t.Parallel()` (for now...).
|
|
Some of our enabled linters are still incompatible with Go 1.18. Disable
them.
|
|
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.
|
