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 problems reported by staticcheck #8947

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Jan 12, 2021

staticcheck is a golang code analysis tool. https://staticcheck.io/

This commit fixes a lot of problems found in our code. Common problems are:

  • unnecessary use of fmt.Sprintf
  • duplicated imports with different names
  • unnecessary check that a key exists before a delete call

There are still a lot of reported problems in the test files but I have
not looked at those.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2021
Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

Nice improvement but a couple of nits.

libpod/image/pull.go Outdated Show resolved Hide resolved
libpod/image/pull.go Outdated Show resolved Hide resolved
@mheon
Copy link
Member

mheon commented Jan 12, 2021

LGTM once comments addressed

`staticcheck` is a golang code analysis tool. https://staticcheck.io/

This commit fixes a lot of problems found in our code. Common problems are:
- unnecessary use of fmt.Sprintf
- duplicated imports with different names
- unnecessary check that a key exists before a delete call

There are still a lot of reported problems in the test files but I have
not looked at those.

Signed-off-by: Paul Holzinger <[email protected]>
@@ -48,7 +48,7 @@ func verifyContainerResourcesCgroupV1(s *specgen.SpecGenerator) ([]string, error
warnings = append(warnings, "Your kernel does not support memory swappiness capabilities, or the cgroup is not mounted. Memory swappiness discarded.")
memory.Swappiness = nil
} else {
if *memory.Swappiness < 0 || *memory.Swappiness > 100 {
Copy link
Member

Choose a reason for hiding this comment

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

Why is the less than zero check removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The swappiness type is uint64 and therefore cannot be less than zero.

Copy link
Member

Choose a reason for hiding this comment

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

I'd just be concerned that someone somewhere down the road decided to change the variable to a signed int. I'd keep the check unless it causes real headaches. Others may have differing opinions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Swappiness is defined here. It is part of the OCI runtime specification. I highly doubt that this will ever be changed to a signed integer.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@mheon
Copy link
Member

mheon commented Jan 12, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2021
@openshift-merge-robot openshift-merge-robot merged commit db5e7ec into containers:master Jan 12, 2021
@Luap99 Luap99 deleted the cleanup-code branch January 17, 2021 13:50
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants