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

Script to automatically add licence headers #82

Closed
wants to merge 14 commits into from
Closed

Script to automatically add licence headers #82

wants to merge 14 commits into from

Conversation

mariantalla
Copy link

@mariantalla mariantalla commented Sep 7, 2018

This PR adds a ensure-boilerplate.sh script; when run, it will add copyright/licence headers to applicable files.

Where a valid licence header is already present, the script will not update or otherwise change it. Similarly for files that it is configured to skip.

As discussed with sig-contributor-experience this is copied from pivotal-k8s/kubernetes-contrib-tools - we'll deprecate the latter once this PR is merged.

/cc @hoegaarden

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 7, 2018
@mariantalla mariantalla changed the title WIP - Script to automatically add licence headers Script to automatically add licence headers Sep 7, 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 Sep 7, 2018
@hoegaarden
Copy link
Contributor

/cc @ixdy @mikedanese

@@ -1,6 +1,6 @@
#!/usr/bin/env python

# Copyright 2015 The Kubernetes Authors.
# Copyright 2018 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

The year is not modified on updates. It reflects the year the file was added in.

Copy link
Contributor

@hoegaarden hoegaarden Sep 12, 2018

Choose a reason for hiding this comment

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

Sorry ... this came from testing the boilerplate code on the boilerplate code itself [1] :) Will revert that.

@@ -1,3 +1,5 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

👍


# Copyright 2014 The Kubernetes Authors.
# Copyright 2018 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

should remain as 2014 only, since that is the year the file was created

Copy link
Contributor

Choose a reason for hiding this comment

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

[1]

"pkg/generated/bindata.go"]

# list all the files that contain 'DO NOT EDIT', but are not generated
default_skipped_not_generated = ['hack/build-ui.sh', 'hack/lib/swagger.sh',
Copy link
Member

Choose a reason for hiding this comment

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

I might be wrong here but my understanding was that repo-infra is for all repositories under the kubernetes org and not just k/k. Wouldn't this hard core it to k/k only?

(I understand that this was present even before but just curious :))

Copy link
Contributor

@hoegaarden hoegaarden Sep 12, 2018

Choose a reason for hiding this comment

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

As of now, we are not sure where repo-infra is actually used. We'd like to see it being used everywhere, also in k/k.

As we started off using the script from k/k we kept the defaults as they were in the k/k repo. However, we introduced an option to override those via a config file in other repos. An example of a config file can be found on this PR for this repo here.

Maybe we can add more (clearer) docs about exactly that. There is some documentation about that here.

Another related question is: Where should this config file be located. Right now we expect it to be in ${REPO_ROOT}/boilerplate.json. If you think that is a too prominent location, we are happy to switch to e.g. ${REPO_ROOT}/.boilerplate.json or something else -- just let us know.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to .boilerplate.json instead of boilerplate.json.

Copy link
Member

Choose a reason for hiding this comment

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

and also I agree with @nikhita that having k/k defaults here seems weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • We will remove the k/k defaults and come up with a set of defaults that are reasonable for a broad range of repos
  • We will switch to using .boilerplate.json for the config file

@@ -0,0 +1,16 @@
/*
Copyright The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@nikhita
Copy link
Member

nikhita commented Sep 12, 2018

To follow the same conventions as scripts in hack/, can we rename this to update-boilerplate.sh instead of ensure-boilerplate.sh?

Also, I am a little confused about how repo-infra is being used for k/k right now. k/k has it's own boilerplate verification: https://github.com/kubernetes/kubernetes/blob/master/hack/verify-boilerplate.sh and https://github.com/kubernetes/kubernetes/tree/master/hack/boilerplate.

/cc @cblecker @spiffxp

@hoegaarden
Copy link
Contributor

hoegaarden commented Sep 12, 2018

  • As said, we are not sure where repo-infra is used and where it is not. The boilerplate stuff is different between in k/k and in repo-infra (before our change). That might be, because it has not been updated in a while or because k/k never had used repo-infra. But ideally, at least from our point of view, k/k should use repo-infra and should have some mechanism to automatically update to new versions of repo-infra.
  • We went for ensure-boilerplate.sh instead of update-boilerplate.sh because it does not really update anything if the boilerplate is already there, it only adds the boilerplate on files which do not have it. Having said that, if you think update-boilerplate.sh is a better name we are happy to rename the thing.

@nikhita
Copy link
Member

nikhita commented Sep 12, 2018

We went for ensure-boilerplate,sh instead of update-boilerplate.sh because it does not really update anything if the boilerplate is already there, it only adds the boilerplate on files which do not have it. Having said that, if you think update-boilerplate.sh is a better name we are happy to rename the thing.

No strong opinions. ensure-boilerplate sgtm.

I'm not aware of the history between repo-infra and k/k. I'll defer to @ixdy @spiffxp @cblecker on that one.

…stead.

Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
@hoegaarden
Copy link
Contributor

Hello @nikhita @ixdy @spiffxp @cblecker .

Do you have any input on that? We'd like to move that forward and see if we can even use that in k/k. Any thoughts?

@cblecker
Copy link
Member

cblecker commented Oct 3, 2018

I like the idea, but python isn't my area of expertise.

WDTY @ixdy @spiffxp? Maybe we could rope @fejta into helping review if you're good with the idea :)

with open(conf_path) as json_data_file:
return json.load(json_data_file)
except ValueError:
raise
Copy link
Member

@tao12345666333 tao12345666333 Oct 5, 2018

Choose a reason for hiding this comment

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

👍

@nikhita
Copy link
Member

nikhita commented Oct 30, 2018

cc @justaugustus @dims @swinslow

This relates to our licensing efforts in Kubernetes.

@hoegaarden
Copy link
Contributor

hoegaarden commented Dec 10, 2018

Is there anything we can do to get this merged?

Or even more importantly: is there a way to track which repos actually use this repo and therefore assess if it's even relevant?

@nikhita
Copy link
Member

nikhita commented Dec 10, 2018

is there a way to track which repos actually use this repo and therefore assess if it's even relevant?

I believe the verify script for license headers is not used anywhere currently. k/k has its own verify script but the plan is to start using this repo in the future.

@ixdy
Copy link
Member

ixdy commented Jan 9, 2019

As said, we are not sure where repo-infra is used and where it is not. The boilerplate stuff is different between in k/k and in repo-infra (before our change). That might be, because it has not been updated in a while or because k/k never had used repo-infra. But ideally, at least from our point of view, k/k should use repo-infra and should have some mechanism to automatically update to new versions of repo-infra.

Right, I don't have a good answer for how we can easily vendor scripts from repo-infra and keep them up to date; just copying the scripts from repo-infra into the various other repos is not great. Anyone have any good suggestions?

@hoegaarden
Copy link
Contributor

hoegaarden commented Jan 9, 2019

What if this repo had a script which would update itself via git subtree merge that could be called from within the parent repo (k/k for example).

I'd be up for experimenting with that in kubernetes-sigs/testing_frameworks.

@spiffxp
Copy link
Member

spiffxp commented Mar 5, 2019

... automation should really be removing the needs-rebase label automatically on push

@spiffxp spiffxp removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2019
@fejta
Copy link
Contributor

fejta commented Mar 29, 2019

/lifecycle stale

Any chance you're going to come back to this? It looks cool and super useful!

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 29, 2019
@hoegaarden
Copy link
Contributor

@fejta I think all the issues and comments are addressed. So I think it is done from our perspective.

@fejta
Copy link
Contributor

fejta commented Mar 29, 2019

Cool! Meaning you're waiting on a review?

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 29, 2019
@hoegaarden
Copy link
Contributor

Yes, that'd be great.

@nikhita
Copy link
Member

nikhita commented Apr 28, 2019

Bumping this. Looks like this needs more reviewers (I'm way out of touch with python)

@nikhita
Copy link
Member

nikhita commented Jun 7, 2019

Can we merge and iterate here?

We require donated repos to have this boilerplate header for all their files. Adding the header to each file in a repo is extremely cumbersome, this script would be immensely helpful in such cases.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 5, 2019
@nikhita
Copy link
Member

nikhita commented Sep 5, 2019

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 5, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 4, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 3, 2020
@k8s-ci-robot
Copy link
Contributor

@mariantalla: PR needs rebase.

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.

@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants