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

verify_boilerplate.py: auto skip YEAR in generated files #233

Merged
merged 1 commit into from
Aug 25, 2021
Merged

verify_boilerplate.py: auto skip YEAR in generated files #233

merged 1 commit into from
Aug 25, 2021

Conversation

saschagrunert
Copy link
Member

We should not skip generated files entirely, but only skip checking for
the YEAR. This patch changes the current behavior to only strip the
year from the ref.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 25, 2021
@saschagrunert saschagrunert force-pushed the genereated-skip-year branch 2 times, most recently from 6f26b2f to 055b24c Compare March 25, 2021 09:44
cpanato
cpanato approved these changes Jun 9, 2021
View reviewed changes
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2021
@cpanato
Copy link
Member

cpanato commented Jun 9, 2021

/assign @mikedanese

@dims
Copy link
Member

dims commented Jun 9, 2021

/assign

@dims
Copy link
Member

dims commented Jun 9, 2021

@cpanato will take a look at this in a bit. i have opinions :)

dims
dims reviewed Jun 10, 2021
View reviewed changes
hack/verify_boilerplate.py
@@ -135,6 +133,13 @@ def file_passes(filename, refs, regexs): # pylint: disable=too-many-locals
# trim our file to the same number of lines as the reference file
data = data[:len(ref)]

# remove the 'YEAR' placeholder from the ref if the file is generated
if is_generated(file_data):
Copy link
Member

Choose a reason for hiding this comment

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

looks like we are updating very specific files ... from this list:
https://github.com/kubernetes/repo-infra/blob/master/hack/verify_boilerplate.py#L76-L93

Also looks like the year is not needed in CNCF guidance:
https://github.com/cncf/foundation/blob/master/copyright-notices.md

So for this specific set of generated files, +1 to drop the year. Let's just not do this for any other files.

thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Should we instead display an error if YEAR is used in the copyright header for generated files (similar to k/k)? This would help ensure consistency across generated files.

https://github.com/kubernetes/kubernetes/blob/046f93b1133dadc6358b656d4000006b0953499b/hack/boilerplate/boilerplate.py#L124-L126

Copy link
Member Author

Choose a reason for hiding this comment

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

@nikhita yes

I fear that we will break existing licence files if we merge a change like this.

Copy link
Member

Choose a reason for hiding this comment

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

though repo-infra is not immediately upgraded anywhere and it shouldn't block go upgrades anymore ™️ now that k/k doesn't use bazel

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to return an error if it contains the YEAR placeholder and is generated.

@saschagrunert saschagrunert force-pushed the genereated-skip-year branch from 055b24c to 0da6e3a Compare June 10, 2021 07:33
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2021
@saschagrunert saschagrunert force-pushed the genereated-skip-year branch 4 times, most recently from 4fe875a to a339547 Compare June 10, 2021 08:01
@saschagrunert
verify_boilerplate.py: auto skip YEAR in generated files
80d4b7e 
We should not skip generated files entirely, but only skip checking for
the `YEAR`. This patch changes the current behavior to only strip the
year from the ref.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@saschagrunert saschagrunert force-pushed the genereated-skip-year branch from a339547 to 80d4b7e Compare June 21, 2021 07:02
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 21, 2021
@justaugustus
Copy link
Member

@dims @nikhita @BenTheElder @saschagrunert -- Have all of the reviews been addressed here?

@saschagrunert
Copy link
Member Author

@dims @nikhita @BenTheElder @saschagrunert -- Have all of the reviews been addressed here?

Yes, from my point of view.

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, justaugustus, saschagrunert

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 25, 2021
@k8s-ci-robot k8s-ci-robot merged commit 72a6f5d into kubernetes:master Aug 25, 2021
@saschagrunert saschagrunert deleted the genereated-skip-year branch August 25, 2021 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikhita nikhita nikhita left review comments

@BenTheElder BenTheElder BenTheElder left review comments

@dims dims dims left review comments

@cpanato cpanato cpanato approved these changes

@justaugustus justaugustus justaugustus approved these changes

Assignees

@dims dims

@justaugustus justaugustus

@mikedanese mikedanese

@cpanato cpanato

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. sig/release Categorizes an issue or PR as relevant to SIG Release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@saschagrunert @cpanato @dims @justaugustus @k8s-ci-robot @BenTheElder @nikhita @mikedanese