CLOUDP-302068: Add a linter to forbid env var usage #1690
Conversation
We want to avoid spreading the practice of accessing environment variables deep in the call hierarchy. We only want to access env vars in the `main` package or very close to it. This commit adds a linter to help us with that. It also adds `nolint:forbidigo` exceptions into places: * Where we allow access (`main` package) * Where we still unfortunately have access. These places ideally need to change with time. To find remaining undesired places, from repo root you can `grep` all files for `nolint:forbidigo` excluding files with `main` package. For example: `grep "nolint:forbidigo" --exclude main.go -r .`
m1kola
added
the
safe-to-test
m1kola
requested review from
mircea-cosbuc,
lsierant,
nammn,
Julien-Ben,
MaciejKaras,
lucian-tosa and
fealebenpae
as code owners
cmd/manager/main.go
Outdated
| @@ -44,7 +44,7 @@ func configureLogger() (*zap.Logger, error) { | |||
| func hasRequiredVariables(logger *zap.Logger, envVariables ...string) bool { | |||
| allPresent := true | |||
| for _, envVariable := range envVariables { | |||
| if _, envSpecified := os.LookupEnv(envVariable); !envSpecified { | |||
| if _, envSpecified := os.LookupEnv(envVariable); !envSpecified { // nolint:forbidigo | |||
There was a problem hiding this comment.
q: I thought we are fine with all files named main.go to keep the env var usage? If yes, could we exclude the whole file instead of single lines? Or is that on purpose here?
There was a problem hiding this comment.
@nammn I considered doing this, but as far as I understand it is only possible to exclude whole linter. It is not an issue right now because we only use forbidigo only for env vars right now. But in the future we decide to use forbidigo for something else main.go will become a blind spot for this linter.
What do you think - still worth adding an exception for main.go?
There was a problem hiding this comment.
I think its better to skip the whole file for now and in case we add more linters we can revisit this decision.
m1kola
added
safe-to-test
m1kola
added
safe-to-test
Summary:
We want to avoid spreading the practice of accessing environment variables deep in the call hierarchy. We only want to access env vars in the
mainpackage or very close to it.This commit adds a linter to help us with that. It also adds
nolint:forbidigoexceptions into places where we still unfortunately have access. These places ideally need to change with time.To find remaining undesired places, from repo root you can
grepall files fornolint:forbidigo. For example:grep "nolint:forbidigo" -r .All Submissions:
closes #XXXXin your comment to auto-close the issue that your PR fixes (if such).