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

Start verifying golint on a per-package basis as packages are fixed #27911

Merged
merged 3 commits into from
Aug 11, 2016
Merged

Start verifying golint on a per-package basis as packages are fixed #27911

merged 3 commits into from
Aug 11, 2016

Conversation

jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Jun 23, 2016 •
edited by k8s-oncall
Loading 

Added `golint` for pkg/security/podsecuritypolicy/capabilities` along with validation.

Analytics

This is a POC to start enabling golint checks on a per-package basis, we did this on the docker project and it was a great way for new contributors to help and it benefits the project overall. All they have to do is add the package they fixed to the bash array in hack/verify-golint.sh and fix all the lint errors.
Eventually when all the packages have been fixed we can change the function to find_files. Or something based off which files are changed in a patch set to verify golint.
Now I used this specific package as the POC because I wanted to show the downside of this changing the api of the package.
Most of the times this arose in docker/docker we decided that if someone wasn't importing their deps locally then it was their loss, but I'm not sure if you all will agree.

This change is Reviewable

@k8s-bot
Copy link

k8s-bot commented Jun 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

3 similar comments
@k8s-bot
Copy link

k8s-bot commented Jun 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jun 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jun 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jun 23, 2016
@jessfraz
Copy link
Contributor Author

If you decide yes, then I can open an issue much like this one (moby/moby#14756) and people can start helping!

lavalamp
lavalamp reviewed Jun 23, 2016
View reviewed changes
hack/verify-golint.sh
cd "${KUBE_ROOT}"

# Use this function when we are ready to start validating all packages.
find_files() {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest omitting this because it will probably have bit rotted by the time it's safe to plug it in.

@k8s-bot
Copy link

k8s-bot commented Jun 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@lavalamp
Copy link
Member

add to whitelist

@MHBauer
Copy link
Contributor

MHBauer commented Jul 7, 2016

I'm totally willing to throw in effort on this if it's desired. Would be great to establish a list of packages needing this work done. I really want to get into the kube codebase, and this was helpful, if a bit overwhelming at times when trying to get into the docker codebase.

@goltermann
Copy link
Contributor

ok to test

@lavalamp lavalamp assigned jessfraz and unassigned lavalamp Jul 8, 2016
@lavalamp
Copy link
Member

lavalamp commented Jul 8, 2016

please reassign when it's ready for another look.

@jessfraz jessfraz force-pushed the start-enable-golint branch from 81b1654 to 9e8f357 Compare July 19, 2016 18:40
@jessfraz
Copy link
Contributor Author

ok now I think it should be what you suggested @lavalamp :)

@jessfraz jessfraz assigned lavalamp and unassigned jessfraz Jul 19, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 19, 2016
@jessfraz jessfraz force-pushed the start-enable-golint branch 3 times, most recently from 4b786b9 to 2cae366 Compare July 20, 2016 00:24
@jessfraz jessfraz assigned jessfraz and unassigned lavalamp Jul 20, 2016
@jessfraz jessfraz force-pushed the start-enable-golint branch from 2cae366 to e89a7bd Compare July 20, 2016 13:39
@k8s-github-robot
Copy link

@jfrazelle
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@k8s-github-robot k8s-github-robot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. retest-not-required-docs-only and removed retest-not-required-docs-only labels Jul 28, 2016
@lavalamp
Copy link
Member

lavalamp commented Aug 4, 2016

Ping @jfrazelle if you could respond to comments I think we can get this merged?

@lavalamp lavalamp removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 4, 2016
@jessfraz jessfraz force-pushed the start-enable-golint branch from a51f7bd to d983cc0 Compare August 4, 2016 22:43
@jessfraz jessfraz force-pushed the start-enable-golint branch 3 times, most recently from ff1358e to 8efc7fd Compare August 5, 2016 18:25
@jessfraz
add golint to test dockerfile
818cdaf 
Signed-off-by: Jess Frazelle <me@jessfraz.com>
@jessfraz
enable golint for pkg/security/podsecuritypolicy/capabilities
c0f4bd3 
Signed-off-by: Jess Frazelle <me@jessfraz.com>
@jessfraz
Start verifying golint on a per-package basis as packages are fixed
70d860f 
Signed-off-by: Jess Frazelle <me@jessfraz.com>
@jessfraz jessfraz force-pushed the start-enable-golint branch from 8efc7fd to 70d860f Compare August 11, 2016 00:40
@k8s-bot
Copy link

k8s-bot commented Aug 11, 2016

GCE e2e build/test passed for commit 70d860f.

@jessfraz jessfraz assigned lavalamp and unassigned dchen1107 Aug 11, 2016
@jessfraz
Copy link
Contributor Author

it's green and comments are addressed now :)

@lavalamp lavalamp added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 11, 2016
@lavalamp
Copy link
Member

LGTM, thanks!

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 11, 2016

GCE e2e build/test passed for commit 70d860f.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d4691a7 into kubernetes:master Aug 11, 2016
@jessfraz jessfraz deleted the start-enable-golint branch August 11, 2016 22:52
@MHBauer MHBauer mentioned this pull request Aug 17, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jessfraz @k8s-bot @lavalamp @MHBauer @goltermann @k8s-github-robot @googlebot @dchen1107