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

Adding automation to add boilerplate license/copyrights headers #4077

Closed
wants to merge 2 commits into from
Closed

Adding automation to add boilerplate license/copyrights headers #4077

wants to merge 2 commits into from

Conversation

mohitsharma-in
Copy link

#4073 Adding support to automatically add license headers

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/github-management Issues or PRs related to GitHub Management subproject labels Mar 7, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @mohitsharma-in!

It looks like this is your first PR to kubernetes/org 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/org has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@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 Mar 7, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @mohitsharma-in. Thanks for your PR.

I'm waiting for a 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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 7, 2023
@nikhita
Copy link
Member

nikhita commented Mar 8, 2023

/cc @MadhavJivrajani @Priyankasaggu11929

@nikhita
Copy link
Member

nikhita commented Mar 8, 2023

/assign

Assigning to myself to approve. Will let @MadhavJivrajani and @Priyankasaggu11929 take an initial review pass at this.

@MadhavJivrajani
Copy link
Contributor

/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 Mar 8, 2023
@mohitsharma-in
Copy link
Author

pull-org-test-all test is failing since --config is being passed which is not expected in this test case that would require changes in makefile to run test

Priyankasaggu11929
Priyankasaggu11929 reviewed Mar 10, 2023
View reviewed changes
Copy link
Member

@Priyankasaggu11929 Priyankasaggu11929 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and the contribution @mohitsharma-in. I've left a few comments above.

hack/k-license/k-license.go Outdated Show resolved Hide resolved
hack/k-license/k-license.go Outdated Show resolved Hide resolved
hack/k-license/k-license.go Outdated Show resolved Hide resolved
hack/k-license/k-license.go Outdated Show resolved Hide resolved
hack/k-license/k-license.go Outdated Show resolved Hide resolved
hack/k-license/k-license.go Outdated Show resolved Hide resolved
@palnabarun
Copy link
Member

Apart from what @Priyankasaggu11929 suggested, k-license is a better candidate for the cmd directory than hack.

@mohitsharma-in
Copy link
Author

/cc @MadhavJivrajani @palnabarun

MadhavJivrajani
MadhavJivrajani reviewed Jun 27, 2023
View reviewed changes
cmd/k-license/k-license.go Outdated Show resolved Hide resolved
cmd/k-license/k-license_test.go Outdated Show resolved Hide resolved
@mohitsharma-in
Copy link
Author

@MadhavJivrajani Can you please review it.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mohitsharma-in
Once this PR has been reviewed and has the lgtm label, please ask for approval from madhavjivrajani. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 7, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2023
mohitsharma-in and others added 2 commits September 4, 2023 10:58
@mohitsharma-in
Adding automation to add boilerplate license/copyrights headers
b5b0afa 
Signed-off-by: Mohit Sharma <[email protected]>

Adding license info

Fixing some flags and test

Signed-off-by: Mohit Sharma <[email protected]>

Fixing boilerplate

Making few optimistaion

Signed-off-by: Mohit Sharma <[email protected]>

Modification and resolving review Comments

fixes

Adding recommendded fixes and resolving comments

Adding recommendded fixes

Minor Modifications

Fixing CI test

Resolving  Suggestions

Fixing unused imports from removing config_test

Adding Docs

Adding license Data

addding skip in existing boilerplate script

adding test case validation
updating testing to check if files actaully got modified
61f247c
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed 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. labels Sep 4, 2023
@mohitsharma-in
Copy link
Author

@MadhavJivrajani Please review it once again

MadhavJivrajani
MadhavJivrajani reviewed Sep 5, 2023
View reviewed changes
Copy link
Contributor

@MadhavJivrajani MadhavJivrajani left a comment

Choose a reason for hiding this comment

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

Just 2 small comments, otherwise LGTM.
Thank you so much for persisting through reviews here, and I apologise this has taken so long.

cmd/k-license/k-license_test.go
Comment on lines +71 to +82
hasLic, err := hasLicense(path)
if !hasLic {
if optTest.confirm {
fmt.Printf("Modified %s file\n", path)
return nil
}
}
if err != nil {

return fmt.Errorf("Error Adding Header/verifying Files")
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the if err != nil check first here.

Also:

          if !hasLic {
				if optTest.confirm {
					fmt.Printf("Modified %s file\n", path)
					return nil
				}
			}

Shouldn't this return an error? If it does not have a license, but we had the confirm flag to set, it should add a license, which is what we want to check.

cmd/k-license/k-license_test.go Show resolved Hide resolved
justaugustus
justaugustus suggested changes Sep 5, 2023
View reviewed changes
Copy link
Member

@justaugustus justaugustus left a comment

Choose a reason for hiding this comment

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

Please strongly consider using addlicense instead: https://github.com/google/addlicense

There may also be some useful content here:

@mohitsharma-in
Copy link
Author

@MadhavJivrajani / @Priyankasaggu11929
Do you think we should not invest much time on this but rather setup our jobs to use
addlicense instead: https://github.com/google/addlicense as suggested by @justaugustus above
#4077 (review)

@Priyankasaggu11929
Copy link
Member

Hey @mohitsharma-in, just checked out the discussion on kubernetes/repo-infra#82.

It seems like we do originally wanted to use verify-boilerplate.py from k/repo-infra, but back then, we were looking for examples of how to use it and vendor it efficiently into other repositories.

Now, we have a working example in k-sigs/release-utils (boilerplate.go), which uses the above script to achieve what we need.

So, yea, I'm +1 on reusing this existing tooling to avoid duplication.

@mohitsharma-in
Copy link
Author

@Priyankasaggu11929 / @MadhavJivrajani Please suggest the appropriate here. If we Don't need this one I'll mark it close.

@Priyankasaggu11929
Copy link
Member

@Priyankasaggu11929 / @MadhavJivrajani Please suggest the appropriate here. If we Don't need this one I'll mark it close.

@mohitsharma-in, yes, let's close this PR and use the existing tooling (discussed in #4077 (comment)) wherever needed.

@Priyankasaggu11929
Copy link
Member

@mohitsharma-in, thank you so much for all your work on this PR.

This discussion resurfaced a lot of useful resources!

@mohitsharma-in
Copy link
Author

/close

@k8s-ci-robot
Copy link
Contributor

@mohitsharma-in: Closed this PR.

In response to this:

/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
Reviewers

@MadhavJivrajani MadhavJivrajani MadhavJivrajani requested changes

@justaugustus justaugustus justaugustus requested changes

@Priyankasaggu11929 Priyankasaggu11929 Awaiting requested review from Priyankasaggu11929

Assignees

@MadhavJivrajani MadhavJivrajani

@Priyankasaggu11929 Priyankasaggu11929

Labels
area/github-management Issues or PRs related to GitHub Management subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.
7 participants
@mohitsharma-in @k8s-ci-robot @nikhita @MadhavJivrajani @palnabarun @Priyankasaggu11929 @justaugustus