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.

Sign up for GitHub

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

Jump to bottom

Cleanup Kubelet static analysis issues #81206

Merged
merged 6 commits into from
Aug 22, 2019
Merged

Cleanup Kubelet static analysis issues #81206

merged 6 commits into from
Aug 22, 2019

Conversation

tallclair
Copy link
Member

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Cleanup the pkg/kubelet/... issues identified by staticcheck in #81189

Which issue(s) this PR fixes:
#36858

Special notes for your reviewer:
Used to validate the options in #81189. Feel free to wait for 81189 to merge first before reviewing.
/hold

Does this PR introduce a user-facing change?:

NONE

/sig node
/priority important-longterm

/assign @yujuhong

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 9, 2019
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet sig/network Categorizes an issue or PR as relevant to SIG Network. labels Aug 9, 2019
This was referenced Aug 9, 2019
Merged
Merged
mattjmcnaughton
mattjmcnaughton reviewed Aug 9, 2019
View reviewed changes
Copy link
Contributor

@mattjmcnaughton mattjmcnaughton left a comment

Choose a reason for hiding this comment

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

Cool stuff @tallclair!

Overall, love this direction and after a brief skim, all the changes look like the increase code quality.

Just to confirm, staticcheck doesn't actually address the errors correct? It just identifies them?
Would be interested to know ballpark how long it took you to make the fixes for pkg/kubelet/....

@mattjmcnaughton
Copy link
Contributor

/test pull-kubernetes-e2e-gce-storage-slow
/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-gce-100-performance

@tallclair
Copy link
Member Author

@mattjmcnaughton Right, it doesn't address them, you have to manually clean up. I don't have a good estimate because I was multitasking and going back and forth with tweaking the staticcheck script, but my guess is about 1-2 hours. My workflow was to paste the staticcheck errors into emacs, which lets me quickly open each file at the triggered line, which sped it up a lot. I think most editors support that format?

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2019
@mattjmcnaughton
Copy link
Contributor

mattjmcnaughton commented Aug 10, 2019 via email

@yujuhong
Copy link
Contributor

This looks great! Let me know if this is ready for review.

@tallclair tallclair changed the title [HOLD] Cleanup Kubelet static analysis issues Cleanup Kubelet static analysis issues Aug 20, 2019
@tallclair
Copy link
Member Author

/hold cancel

Ready for review!

@k8s-ci-robot k8s-ci-robot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 20, 2019
mattjmcnaughton
mattjmcnaughton approved these changes Aug 21, 2019
View reviewed changes
Copy link
Contributor

@mattjmcnaughton mattjmcnaughton left a comment

Choose a reason for hiding this comment

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

/lgtm

good stuff :)

definitely think it makes sense to merge this as soon as possible, before we start getting duplicate prs for fixes in kubelet.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2019
@mattjmcnaughton
Copy link
Contributor

/test pull-kubernetes-kubemark-e2e-gce-big

@mattjmcnaughton
Copy link
Contributor

/assign @vishh

@RainbowMango RainbowMango mentioned this pull request Aug 21, 2019
Closed
yujuhong
yujuhong reviewed Aug 21, 2019
View reviewed changes
pkg/kubelet/pleg/generic.go
@@ -253,7 +253,7 @@ func (g *GenericPLEG) relist() {
needsReinspection[pid] = pod

continue
} else if _, found := g.podsToReinspect[pid]; found {
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing the condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

delete is a no-op if the element isn't in the map, so the condition is redundant.

pkg/kubelet/remote/utils.go Outdated
}

if status.CreatedAt == 0 {
return fmt.Errorf("CreatedAt is not set")
return fmt.Errorf("createdAt is not set")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we want the error messages to start with a lower-case letter, but changing the variable name for this is weird....

I'm okay with this since adding exceptions may not be worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Woops, this was just me not paying enough attention to my find-replace. We're actually not using this check, it was more an experiment to see what applying it here would do. Given that, I just reverted this file.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 21, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2019
@tallclair
Copy link
Member Author

Rebased. Picked up some new failures from #81434, but I opted to just keep them on the blacklist and follow up with a separate PR.

Hoping we can merge this quickly to avoid treading water with new failures...

For hack/.staticcheckfailures approval:
/assign @BenTheElder
(Should we expand the approvers set for the blacklist files?)

@mattjmcnaughton
Copy link
Contributor

mattjmcnaughton commented Aug 22, 2019 via email

BenTheElder
BenTheElder approved these changes Aug 22, 2019
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, tallclair

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2019
@k8s-ci-robot k8s-ci-robot merged commit a3488b4 into kubernetes:master Aug 22, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 22, 2019
@mattnworb mattnworb mentioned this pull request Aug 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yujuhong yujuhong yujuhong left review comments

@BenTheElder BenTheElder BenTheElder approved these changes

@mattjmcnaughton mattjmcnaughton mattjmcnaughton approved these changes

@danwinship danwinship Awaiting requested review from danwinship

@dashpole dashpole Awaiting requested review from dashpole

Assignees

@BenTheElder BenTheElder

@mattjmcnaughton mattjmcnaughton

@vishh vishh

@yujuhong yujuhong

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.16
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tallclair @mattjmcnaughton @yujuhong @k8s-ci-robot @BenTheElder @vishh