Skip to content

Conversation

kolyshkin
Copy link
Contributor

  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.

@AkihiroSuda
Copy link
Member

Maybe too strict?

@kolyshkin
Copy link
Contributor Author

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?

@kolyshkin
Copy link
Contributor Author

Problem is, we don't know which errors are wrapped and which are not.

  • anything that is returned from libcontainer may be wrapped (so needs to be lowercased)
  • anything outside of libcontainer may or may not be wrapped

@AkihiroSuda what if we only apply the linter to libcontainer?

@kolyshkin
Copy link
Contributor Author

Problem is, we don't know which errors are wrapped and which are not.

  • anything that is returned from libcontainer may be wrapped (so needs to be lowercased)
  • anything outside of libcontainer may or may not be wrapped

@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.

@AkihiroSuda
Copy link
Member

Sorry, not sure how wrapping is relevant to capitalization

@kolyshkin
Copy link
Contributor Author

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.
// Bad:
err := fmt.Errorf("Something bad happened.")
// Good:
err := fmt.Errorf("something bad happened")

On the other hand, the style for the full displayed message (logging, test failure, API response, or other UI) depends, but should typically be capitalized.

// Good:
log.Infof("Operation aborted: %v", err)
log.Errorf("Operation aborted: %v", err)
t.Errorf("Op(%q) failed unexpectedly; err=%v", args, err)

Another quote, from https://gist.github.com/adamveld12/c0d9f0d5f0e1fba1e551#error-strings:

Error strings should not be capitalized (unless beginning with proper nouns or acronyms) or end with punctuation, since they are usually printed following other context. That is, use fmt.Errorf("something bad") not fmt.Errorf("Something bad"), so that log.Print("Reading %s: %v", filename, err) formats without a spurious capital letter mid-message. This does not apply to logging, which is implicitly line-oriented and not combined inside other messages.

Copy link
Member

@thaJeztah thaJeztah left a 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")
Copy link
Member

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

// 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>
@kolyshkin
Copy link
Contributor Author

Rebased

@cyphar
Copy link
Member

cyphar commented Aug 6, 2023

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).

Maybe we should randomly change our error messages to ensure people don't do this? 😉

@kolyshkin
Copy link
Contributor Author

Guess I'll close this one due to lack of interest.

@kolyshkin kolyshkin closed this Nov 13, 2023
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Mar 24, 2025
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>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Mar 25, 2025
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>
aojea pushed a commit to aojea/runc that referenced this pull request Apr 9, 2025
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>
lifubang pushed a commit to lifubang/runc that referenced this pull request Apr 24, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants