Skip to content
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

Fix various linting issues #2974

Merged
merged 9 commits into from
Jun 2, 2021

Conversation

thaJeztah
Copy link
Member

splitting this from #2966
relates to #2627

@thaJeztah
Copy link
Member Author

@kolyshkin @AkihiroSuda PTAL

still some to address after this (see #2966 (comment)), but cleaning up the other PR

kzys
kzys previously approved these changes Jun 1, 2021
type Cgroup struct {
// Deprecated, use Path instead
// Name specifies the name of the cgroup
// Deprecated: use Path instead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently found out that Name and Parent are not deprecated at least for systemd cgroup manager case, because Path can not be used in that case. This might have to be fixed, but for now, I guess, it's better to remove the deprecation notice.

minId = 0
maxId = 1<<31 - 1 //for 32-bit systems compatibility
minID = 0
maxID = 1<<31 - 1 // for 32-bit systems compatibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have a similar change in #2975

libcontainer/user/user.go Outdated Show resolved Hide resolved
libcontainer/user/user.go Outdated Show resolved Hide resolved
libcontainer/user/user.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM except for a few nits.

I wonder where these errors are coming from? I don't see it running golangci-lint run ./... locally, nor in GHA CI. Are you using a different golangci config or version?

@kolyshkin kolyshkin added this to the 1.0.0 milestone Jun 1, 2021
@kolyshkin
Copy link
Contributor

Tentatively set 1.0.0 milestone as this seems like a good fit.

@cyphar
Copy link
Member

cyphar commented Jun 2, 2021

I'm not sure we need to tag this for 1.0.0. If it makes it in, great -- but I think we should go for a release if #2986 fixes the Moby CI regression.

@cyphar cyphar removed this from the 1.0.0 milestone Jun 2, 2021
@thaJeztah
Copy link
Member Author

Thanks for review! I'll try to update this PR later tonight

I wonder where these errors are coming from? I don't see it running golangci-lint run ./... locally, nor in GHA CI. Are you using a different golangci config or version?

Yes; enabled "golint" and EXC002, which is a default exception in golang-ci-lint ("because nobody writes docs the right way 🙄) see the last commit on #2966

I'm not sure we need to tag this for 1.0.0. If it makes it in, great -- but I think we should go for a release if #2986 fixes the Moby CI regression.

I agree for the changes in this PR (they're "nice to have", but most can be merged at any time). That said; the linter also picked up some incorrectly named exported variables and properties; see #2966 (comment)

Changing those after v1.0.0 would be a breaking change, so perhaps it's good to have a look at those, and decide wether or not they should be renamed, or "ignored" ("won't fix").

This prevents having to maintain GoDoc for the stub implementations,
and makes sure that the "stub" implementations have the same signature
as the "non-stub" versions.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
    libcontainer/cgroups/devices/devices_emulator.go:261:9: `if` block ends with a `return` statement, so drop this `else` and outdent its block (golint)
    	} else {
    	       ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    libcontainer/keys/keyctl.go:17:2: var `sessKeyId` should be `sessKeyID` (golint)
        sessKeyId, err := unix.KeyctlJoinSessionKeyring(name)
        ^

    libcontainer/keys/keyctl.go:27:21: func parameter `ringId` should be `ringID` (golint)
    func ModKeyringPerm(ringId KeySerial, mask, setbits uint32) error {
                        ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

@kolyshkin @cyphar @AkihiroSuda updated; PTAL

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (some missing periods at end of sentences but it's OK).

Thank you @thaJeztah!

@kolyshkin kolyshkin merged commit 48d76ad into opencontainers:master Jun 2, 2021
@thaJeztah thaJeztah deleted the fix_some_linting branch June 3, 2021 08:13
@thaJeztah
Copy link
Member Author

some missing periods at end of sentences but it's OK

Ah, sorry. yes, that's me. I have a tendency of omitting punctuation for single-line comments 😓 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants