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

Add verify_boilerplate step to check for copyright header #442

Merged
merged 10 commits into from Dec 6, 2018
Merged

Add verify_boilerplate step to check for copyright header #442

merged 10 commits into from Dec 6, 2018

Conversation

ashish-amarnath
Copy link
Contributor

@ashish-amarnath ashish-amarnath commented Dec 4, 2018 •
edited

What this PR does / why we need it:
Adds a new script to verify that all newly added files have copyright header

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #441

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 4, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @ashish-amarnath. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 4, 2018
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2018
@ashish-amarnath ashish-amarnath changed the title [WIP] Add a script to verify copyright header in all newly added files Add verify_boilerplate step to check for copyright header Dec 5, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2018
@dims
Copy link
Member

dims commented Dec 5, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 5, 2018
@chuckha
Copy link
Contributor

chuckha commented Dec 5, 2018

/ok-to-test

@ashish-amarnath
ignore code generated by Mockgen
4fe89d2 
Signed-off-by: Ashish Amarnath <ashish.amarnath@gmail.com>
@ashish-amarnath
Copy link
Contributor Author

ashish-amarnath commented Dec 5, 2018 •
edited

The PR is super large in terms of the number of files changed. But, the change is around a few main files:

  • New files under hack/boilerplate/boilerplate.*.txt. These are stolen as is from the upstream cluster-api repo that drive what the boilerplate should look like for various file extensions.
  • New hack/verify_boilerplate.py file which is copied from the upstream cluster-api rep with modifications to ignore auto-generated files by bazel, MockGen, and go-bindata.
  • Modify build/go_mock.bzl to remove the dependency of a hardcoded boilerplate.go.txt
  • Deleting the hardcoded boilerplate file at hack/boilerplage.go.txt
  • In the Makefile:
    ** adds a new verify target to invoke the boilerplate verification. This will help us avoid a change in the test-infra repo to change the pre-submit job config but still automate this verification
    ** adds verify as a make dependency on the test target.

All the other changes are to fix the copyright headers in files that were in violation with either missing or non-conformant headers.

Happy to walk through the changes 🙂 ping me on slack @ ashisha

@ashish-amarnath
Copy link
Contributor Author

ashish-amarnath commented Dec 5, 2018 •
edited

/assign @detiber

cc @dims

@vincepri
Copy link
Member

vincepri commented Dec 5, 2018

This looks great!
/approve

Waiting on @detiber for lgtm.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashish-amarnath, vincepri

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 Dec 5, 2018
detiber
detiber reviewed Dec 6, 2018
View reviewed changes
build/asm_shim/c.go Show resolved Hide resolved
hack/boilerplate/boilerplate.go.txt Show resolved Hide resolved
hack/print-workspace-status.sh Show resolved Hide resolved
pkg/cloud/aws/actuators/clients.go Show resolved Hide resolved
pkg/cloud/aws/converters/tags.go Show resolved Hide resolved
pkg/cloud/aws/services/ec2/mock_ec2iface/ec2api.go Outdated Show resolved Hide resolved
@ashish-amarnath
fix headers for autogenerated files
0dff169 
remove boilerplate for kubebuilder
pass standard boilerplate to deepcopygen and kubebuilder

Signed-off-by: Ashish Amarnath <ashish.amarnath@gmail.com>
detiber
detiber reviewed Dec 6, 2018
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for getting this sorted out!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2018
@k8s-ci-robot k8s-ci-robot merged commit da39a22 into kubernetes-sigs:master Dec 6, 2018
@ashish-amarnath
Copy link
Contributor Author

kubernetes/test-infra#10370

@ashish-amarnath ashish-amarnath deleted the banner-verify branch December 6, 2018 20:03
@ashish-amarnath
Copy link
Contributor Author

sample test failure: #451

@ashish-amarnath ashish-amarnath mentioned this pull request Feb 11, 2019
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@detiber detiber detiber left review comments

@randomvariable randomvariable randomvariable left review comments

@vincepri vincepri vincepri left review comments

@liztio liztio liztio left review comments

@roberthbailey roberthbailey Awaiting requested review from roberthbailey

Assignees

@detiber detiber

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automate "Copyright" header/banner check for new files
8 participants
@ashish-amarnath @k8s-ci-robot @dims @chuckha @vincepri @detiber @randomvariable @liztio