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

golangci: enable GoDoc linting #2966

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented May 27, 2021

relates to #2627

golint is scoring pretty low currently, because many exported
functions and variables are missing a well-formatted GoDoc comment:

https://goreportcard.com/report/github.com/opencontainers/runc

Ran inside a container (ignoring the CGO issues, as I was a bit lazy);

docker run -e CGO_ENABLED=1 --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.40.1 golangci-lint run -v --max-issues-per-linter=0 --max-same-issues=0

@thaJeztah thaJeztah force-pushed the more_linting branch 3 times, most recently from c462330 to bdec579 Compare June 1, 2021 11:22
@thaJeztah

This comment has been minimized.

@kolyshkin kolyshkin added this to the post-1.0 milestone Jun 1, 2021
@thaJeztah thaJeztah force-pushed the more_linting branch 8 times, most recently from e9612df to da1c733 Compare June 3, 2021 21:28
@thaJeztah
Copy link
Member Author

Remaining ones:

To discuss: (rename or add nolint comments?)

libcontainer/user/lookup_unix.go:31:6: var-naming: func LookupUid should be LookupUID (revive)
libcontainer/user/user.go:30:2: var-naming: struct field Uid should be UID (revive)
libcontainer/user/user.go:214:2: var-naming: struct field Uid should be UID (revive)
libcontainer/devices/device.go:23:2: var-naming: struct field Uid should be UID (revive)
libcontainer/configs/cgroup_linux.go:60:2: var-naming: struct field CpuShares should be CPUShares (revive)
libcontainer/configs/cgroup_linux.go:63:2: var-naming: struct field CpuQuota should be CPUQuota (revive)
libcontainer/configs/cgroup_linux.go:66:2: var-naming: struct field CpuPeriod should be CPUPeriod (revive)
libcontainer/configs/cgroup_linux.go:69:2: var-naming: struct field CpuRtRuntime should be CPURtRuntime (revive)
libcontainer/configs/cgroup_linux.go:72:2: var-naming: struct field CpuRtPeriod should be CPURtPeriod (revive)
libcontainer/configs/cgroup_linux.go:125:2: var-naming: struct field CpuWeight should be CPUWeight (revive)
libcontainer/configs/config.go:159:2: var-naming: struct field UidMappings should be UIDMappings (revive)
libcontainer/cgroups/stats.go:16:6: var-naming: type CpuUsage should be CPUUsage (revive)
libcontainer/cgroups/stats.go:37:6: var-naming: type CpuStats should be CPUStats (revive)
libcontainer/cgroups/stats.go:38:2: var-naming: struct field CpuUsage should be CPUUsage (revive)
libcontainer/cgroups/stats.go:150:2: var-naming: struct field CpuStats should be CPUStats (revive)
types/events.go:60:6: var-naming: type CpuUsage should be CPUUsage (revive)
types/events.go:70:6: var-naming: type Cpu should be CPU (revive)
libcontainer/cgroups/fs/cpu.go:16:6: var-naming: type CpuGroup should be CPUGroup (revive)
libcontainer/error.go:12:2: var-naming: const IdInUse should be IDInUse (revive)
libcontainer/error.go:13:2: var-naming: const InvalidIdFormat should be InvalidIDFormat (revive)
libcontainer/criu_opts_linux.go:20:2: var-naming: struct field TcpEstablished should be TCPEstablished (revive)
libcontainer/init_linux.go:65:2: var-naming: struct field ContainerId should be ContainerID (revive)

@kolyshkin
Copy link
Contributor

To discuss: (rename or add nolint comments?)

I think renaming is fine and make sense (unless we're changing public API, it which case we need to see how many users are there, how easy is for them to adapt etc.)

@thaJeztah thaJeztah force-pushed the more_linting branch 3 times, most recently from 55895d9 to ad081fa Compare June 8, 2021 15:26
@thaJeztah
Copy link
Member Author

@kolyshkin (forgot to ping you); last commit is a quick renamed of variables to the correct capitalisation. There's some inconsistency in CRIU vs Criu (looks like the CRIU sdk itself uses the "wrong" casing). Thought I'd just rename them all for discussion.

@AkihiroSuda
Copy link
Member

Needs rebase


issues:
include:
- EXC0002
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a warning produced by golint? Since this commit does not enable golint, it makes no sense.

Or, if it is also produced by revive, I think a comment is needed here, telling what this does.

@thaJeztah thaJeztah force-pushed the more_linting branch 2 times, most recently from 67d0457 to 9695867 Compare January 4, 2022 15:43
    Warning: receiver-naming: receiver name source should be consistent with previous receiver name e for Emulator (revive)
    Warning: var-declaration: should omit type int from declaration of var cgroupFd; it will be inferred from the right-hand side (revive)
    Warning: var-naming: don't use underscores in Go names; type Stat_t should be StatT (revive)
    Warning: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive)
    Warning: var-naming: method parameter containerId should be containerID (revive)
    Warning: var-naming: method parameter containerId should be containerID (revive)
    Warning: exported: exported var ErrV1NoUnified should have comment or be unexported (revive)
    Warning: exported: comment on exported function FindCgroupMountpoint should be of the form "FindCgroupMountpoint ..." (revive)
    Warning: exported: comment on exported type Device should be of the form "Device ..." (with optional leading article) (revive)
    Warning: exported: comment on exported type DeviceRule should be of the form "DeviceRule ..." (with optional leading article) (revive)
    Warning: exported: comment on exported type DeviceType should be of the form "DeviceType ..." (with optional leading article) (revive)
    Warning: exported: comment on exported type DevicePermissions should be of the form "DevicePermissions ..." (with optional leading article) (revive)
    Warning: exported: comment on exported function ConvertCPUSharesToCgroupV2Value should be of the form "ConvertCPUSharesToCgroupV2Value ..." (revive)
    Warning: exported: comment on exported function ConvertBlkIOToIOWeightValue should be of the form "ConvertBlkIOToIOWeightValue ..." (revive)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    Warning: var-naming: func parseCpuInfoFile should be parseCPUInfoFile (revive)
    Warning: var-naming: don't use underscores in Go names; func mockResctrlL3_MON should be mockResctrlL3MON (revive)
    Warning: var-naming: don't use underscores in Go names; var mockedL3_MON should be mockedL3MON (revive)

libcontainer/intelrdt: fix various linting issues

    libcontainer/intelrdt/intelrdt.go:530:1: exported: comment on exported function IsCATEnabled should be of the form "IsCATEnabled ..." (revive)
    // Check if Intel RDT/CAT is enabled
    ^
    libcontainer/intelrdt/intelrdt.go:536:1: exported: comment on exported function IsMBAEnabled should be of the form "IsMBAEnabled ..." (revive)
    // Check if Intel RDT/MBA is enabled
    ^
    libcontainer/intelrdt/intelrdt.go:542:1: exported: comment on exported function IsMBAScEnabled should be of the form "IsMBAScEnabled ..." (revive)
    // Check if Intel RDT/MBA Software Controller is enabled
    ^
    libcontainer/intelrdt/intelrdt.go:548:1: exported: comment on exported function GetIntelRdtPath should be of the form "GetIntelRdtPath ..." (revive)
    // Get the 'container_id' path in Intel RDT "resource control" filesystem
    ^
    libcontainer/intelrdt/util_test.go:31:40: unexported-return: exported func NewIntelRdtTestUtil returns unexported type *intelrdt.intelRdtTestUtil, which can be annoying to use (revive)
    func NewIntelRdtTestUtil(t *testing.T) *intelRdtTestUtil {
                                           ^
libcontainer/indelrdt: un-export IntelRdtTasks

It's only used within the package itself, so un-exporting

    libcontainer/intelrdt/intelrdt.go:184:2: exported: exported const IntelRdtTasks should have comment (or a comment on this block) or be unexported (revive)
        IntelRdtTasks = "tasks"
        ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    libcontainer/system/linux.go:50:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
        if err := unix.Prctl(unix.PR_SET_PDEATHSIG, sig, 0, 0, 0); err != nil {
            return err
        }
    libcontainer/system/linux.go:65:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
        if err := unix.Prctl(unix.PR_SET_KEEPCAPS, 1, 0, 0, 0); err != nil {
            return err
        }
    libcontainer/system/linux.go:73:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
        if err := unix.Prctl(unix.PR_SET_KEEPCAPS, 0, 0, 0, 0); err != nil {
            return err
        }

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    libcontainer/utils/cmsg.go:28:1: exported: comment on exported const MaxNameLen should be of the form "MaxNameLen ..." (revive)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    libcontainer/configs/config_linux.go:10:26: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
                return -1, fmt.Errorf("User namespaces enabled, but no uid mappings found.")
                                      ^
    libcontainer/configs/config_linux.go:14:26: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
                return -1, fmt.Errorf("User namespaces enabled, but no user mapping found.")
                                      ^
    libcontainer/configs/config_linux.go:33:26: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
                return -1, fmt.Errorf("User namespaces enabled, but no gid mappings found.")
                                      ^
    libcontainer/configs/config_linux.go:37:26: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
                return -1, fmt.Errorf("User namespaces enabled, but no group mapping found.")
                                      ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Unexporting NewCgroupTestUtil to prevent linting warnings

    libcontainer/cgroups/fs/util_test.go:37:56: unexported-return: exported func NewCgroupTestUtil returns unexported type *fs.cgroupTestUtil, which can be annoying to use (revive)
    func NewCgroupTestUtil(subsystem string, t *testing.T) *cgroupTestUtil {
                                                           ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
The CgroupNamePrefix looks like something unlikely to change (perhaps better
for mountCgroupV1 to be moved to libcontainer/cgroups?).

    libcontainer/cgroups/v1_utils.go:21:2: exported: exported const CgroupNamePrefix should have comment (or a comment on this block) or be unexported (revive)
    libcontainer/cgroups/utils.go:25:2: exported: exported const CgroupProcesses should have comment (or a comment on this block) or be unexported (revive)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    notify_socket.go:97:1: receiver-naming: receiver name n should be consistent with previous receiver name s for notifySocket (revive)
    func (n *notifySocket) waitForContainer(container libcontainer.Container) error {
    notify_socket.go:105:1: receiver-naming: receiver name n should be consistent with previous receiver name s for notifySocket (revive)
    func (n *notifySocket) run(pid1 int) error {

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    libcontainer/specconv/spec_linux_test.go:647:2: var-naming: var ttyUid should be ttyUID (revive)
    libcontainer/init_linux.go:456:7: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive)
    const _P_PID = 1
          ^
    libcontainer/state_linux.go:76:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
    libcontainer/init_linux.go:460:2: var-naming: don't use underscores in Go names; struct field si_signo should be siSigno (revive)
    libcontainer/init_linux.go:461:2: var-naming: don't use underscores in Go names; struct field si_errno should be siErrno (revive)
    libcontainer/init_linux.go:462:2: var-naming: don't use underscores in Go names; struct field si_code should be siCode (revive)
    libcontainer/init_linux.go:464:2: var-naming: don't use underscores in Go names; struct field si_pid should be siPid (revive)
    libcontainer/standard_init_linux.go:59:6: var-naming: var sessKeyId should be sessKeyID (revive)
    libcontainer/rootfs_linux.go:232:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
    libcontainer/rootfs_linux.go:920:11: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)
    libcontainer/integration/exec_test.go:563:6: var-naming: func testCpuShares should be testCPUShares (revive)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    utils_linux.go:395:2: exported: exported const CT_ACT_CREATE should have comment (or a comment on this block) or be unexported (revive)
        CT_ACT_CREATE CtAct = iota + 1
        ^
    utils_linux.go:396:2: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive)
        CT_ACT_RUN
        ^
    utils_linux.go:397:2: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive)
        CT_ACT_RESTORE
        ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
golint is scoring pretty low currently, because many exported
functions and variables are missing a well-formatted GoDoc comment:

https://goreportcard.com/report/github.com/opencontainers/runc

Note that `golint` has been depracted, and replaced by `revive`:

    level=warning msg="[runner] The linter 'golint' is deprecated (since v1.41.0) due to: The repository of the linter has been archived by the owner.  Replaced by revive."

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    libcontainer/user/lookup_unix.go:31:6: var-naming: func LookupUid should be LookupUID (revive)
    libcontainer/user/user.go:30:2: var-naming: struct field Uid should be UID (revive)
    libcontainer/user/user.go:214:2: var-naming: struct field Uid should be UID (revive)
    libcontainer/devices/device.go:23:2: var-naming: struct field Uid should be UID (revive)
    libcontainer/configs/cgroup_linux.go:60:2: var-naming: struct field CpuShares should be CPUShares (revive)
    libcontainer/configs/cgroup_linux.go:63:2: var-naming: struct field CpuQuota should be CPUQuota (revive)
    libcontainer/configs/cgroup_linux.go:66:2: var-naming: struct field CpuPeriod should be CPUPeriod (revive)
    libcontainer/configs/cgroup_linux.go:69:2: var-naming: struct field CpuRtRuntime should be CPURtRuntime (revive)
    libcontainer/configs/cgroup_linux.go:72:2: var-naming: struct field CpuRtPeriod should be CPURtPeriod (revive)
    libcontainer/configs/cgroup_linux.go:125:2: var-naming: struct field CpuWeight should be CPUWeight (revive)
    libcontainer/configs/config.go:159:2: var-naming: struct field UidMappings should be UIDMappings (revive)
    libcontainer/cgroups/stats.go:16:6: var-naming: type CpuUsage should be CPUUsage (revive)
    libcontainer/cgroups/stats.go:37:6: var-naming: type CpuStats should be CPUStats (revive)
    libcontainer/cgroups/stats.go:38:2: var-naming: struct field CpuUsage should be CPUUsage (revive)
    libcontainer/cgroups/stats.go:150:2: var-naming: struct field CpuStats should be CPUStats (revive)
    types/events.go:60:6: var-naming: type CpuUsage should be CPUUsage (revive)
    types/events.go:70:6: var-naming: type Cpu should be CPU (revive)
    libcontainer/cgroups/fs/cpu.go:16:6: var-naming: type CpuGroup should be CPUGroup (revive)
    libcontainer/error.go:12:2: var-naming: const IdInUse should be IDInUse (revive)
    libcontainer/error.go:13:2: var-naming: const InvalidIdFormat should be InvalidIDFormat (revive)
    libcontainer/criu_opts_linux.go:20:2: var-naming: struct field TcpEstablished should be TCPEstablished (revive)
    libcontainer/init_linux.go:65:2: var-naming: struct field ContainerId should be ContainerID (revive)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants