-
Notifications
You must be signed in to change notification settings - Fork 2.2k
ci: enable stylecheck linter, fix ST1005 warnings #3857
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Maybe too strict? |
Maybe. We wrap some errors, and those need to be non-capitalized, and we don't wrap some others, and those are actually better if capitalized. @thaJeztah wdyt? |
Problem is, we don't know which errors are wrapped and which are not.
@AkihiroSuda what if we only apply the linter to libcontainer? |
Nope; in fact, kubernetes, cri-o, containerd and other users also wrap errors printed by runc, so everything should be lowercased, so this PR makes total sense. |
Sorry, not sure how wrapping is relevant to capitalization |
The idea here is something like "Error: can't start container: can't run init: permission denied" looks better than "Error: Can't start container: Can't run init: permission denied". Here' a quote from Google Go style guide (https://google.github.io/styleguide/go/decisions#error-strings): Error strings should not be capitalized (unless beginning with an exported name, a proper noun or an acronym) and should not end with punctuation. This is because error strings usually appear within other context before being printed to the user. Another quote, from https://gist.github.com/adamveld12/c0d9f0d5f0e1fba1e551#error-strings:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall I agree with these changes, but slightly concerned about changing the error-messages, as it's possible there's some string-matching in other codebases that handle the errors returned from runc (binary).
@@ -314,10 +314,10 @@ func validateProcessSpec(spec *specs.Process) error { | |||
return errors.New("process property must not be empty") | |||
} | |||
if spec.Cwd == "" { | |||
return errors.New("Cwd property must not be empty") | |||
return errors.New("cwd property must not be empty") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For others reviewing (I was wondering if this should be CWD), but the property is lowercase in JSON, so probably makes sense to match that
runc/vendor/github.com/opencontainers/runtime-spec/specs-go/config.go
Lines 78 to 80 in e42446f
// Cwd is the current working directory for the process and must be | |
// relative to the container's root. | |
Cwd string `json:"cwd"` |
1. Enable stylecheck linter (with some checks disabled). 2. Enabled checks disabled above in golangci-extra.yml (used for new code). 3. Fix "ST1005: error strings should not be capitalized (stylecheck)" warnings. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
224215c
to
e831637
Compare
Rebased |
Maybe we should randomly change our error messages to ensure people don't do this? 😉 |
Guess I'll close this one due to lack of interest. |
The new configuration file was initially generated by golangci-lint migrate, when tweaked to minimize and simplify. In particular, golangci-lint v2 switches to a new version of staticcheck which shows much more warnings. Some of them were fixed by a few previous commits, and the rest of them are disabled. In particular, ST1005 had to be disabled (an attempt to fix it was made in opencontainers#3857 but it wasn't merged). Also, golangci-extra was modified to include ALL staticcheck linters. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The new configuration file was initially generated by golangci-lint migrate, when tweaked to minimize and simplify. golangci-lint v2 switches to a new version of staticcheck which shows much more warnings. Some of them were fixed by a few previous commits, and the rest of them are disabled. In particular, ST1005 had to be disabled (an attempt to fix it was made in opencontainers#3857 but it wasn't merged). Also, golangci-extra was modified to include ALL staticcheck linters. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The new configuration file was initially generated by golangci-lint migrate, when tweaked to minimize and simplify. golangci-lint v2 switches to a new version of staticcheck which shows much more warnings. Some of them were fixed by a few previous commits, and the rest of them are disabled. In particular, ST1005 had to be disabled (an attempt to fix it was made in opencontainers#3857 but it wasn't merged). Also, golangci-extra was modified to include ALL staticcheck linters. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The new configuration file was initially generated by golangci-lint migrate, when tweaked to minimize and simplify. golangci-lint v2 switches to a new version of staticcheck which shows much more warnings. Some of them were fixed by a few previous commits, and the rest of them are disabled. In particular, ST1005 had to be disabled (an attempt to fix it was made in opencontainers#3857 but it wasn't merged). Also, golangci-extra was modified to include ALL staticcheck linters. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 127e8e6) Signed-off-by: lifubang <lifubang@acmcoder.com>
Enable stylecheck linter (with some checks disabled).
Enabled checks disabled above in golangci-extra.yml (used for new code).
Fix "ST1005: error strings should not be capitalized (stylecheck)" warnings.