Add license automatically #3122

bangqipropel · 2021-12-10T23:07:43Z

add make add-copyright and make check-copyright in Makefile to add and check copyright for files automatically
check if all needed copyright is added in workflow
add copyright to needed files

This is based on #3100

Makefile

luolanzone · 2021-12-13T02:06:03Z

@bangqipropel add necessary info to make your commit and PR summary clear. and link it to the right issue. thanks.

bangqipropel · 2021-12-13T08:12:08Z

@bangqipropel add necessary info to make your commit and PR summary clear. and link it to the right issue. thanks.

@luolanzone Hi, I see, but it seems like the "Linked issues", "Assignees" or something like that is not editable for me, seems like no permission for me, for the antrea main fork, you have any idea whom I should ask the permission for?

luolanzone · 2021-12-13T08:48:44Z

@bangqipropel , you don't need such permission, please update the description to link the issue, you can take a look at this help doc: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
btw, your DCO build failed, please check this doc to add your commit and PR sign-off. https://github.com/antrea-io/antrea/blob/main/CONTRIBUTING.md#sign-off-your-work

codecov-commenter · 2021-12-16T04:33:33Z

Codecov Report

Merging #3122 (1546a8d) into main (70fac83) will decrease coverage by 12.06%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #3122       +/-   ##
===========================================
- Coverage   59.08%   47.02%   -12.07%     
===========================================
  Files         331      397       +66     
  Lines       28444    42069    +13625     
===========================================
+ Hits        16806    19781     +2975     
- Misses       9804    20134    +10330     
- Partials     1834     2154      +320

Flag	Coverage Δ
integration-tests	`33.98% <ø> (?)`
kind-e2e-tests	`35.20% <ø> (-11.54%)`	⬇️
unit-tests	`41.60% <ø> (-0.24%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...uster/controllers/multicluster/controller_utils.go	`44.18% <ø> (ø)`
pkg/agent/proxy/metrics/metrics.go	`100.00% <ø> (ø)`
pkg/ovs/openflow/ofctrl_action.go	`65.41% <ø> (-2.50%)`	⬇️
pkg/ovs/openflow/ofctrl_bridge.go	`55.58% <ø> (ø)`
pkg/ovs/openflow/ofctrl_flow.go	`39.39% <ø> (-1.52%)`	⬇️
pkg/agent/controller/networkpolicy/reject.go	`0.00% <0.00%> (-87.91%)`	⬇️
...g/agent/apiserver/handlers/featuregates/handler.go	`0.00% <0.00%> (-82.36%)`	⬇️
pkg/apis/controlplane/v1beta2/helper.go	`33.33% <0.00%> (-66.67%)`	⬇️
pkg/agent/types/networkpolicy.go	`16.66% <0.00%> (-66.67%)`	⬇️
pkg/controller/networkpolicy/mutate.go	`0.00% <0.00%> (-62.07%)`	⬇️
... and 279 more

ci/check-copyright.sh

hack/add-license.sh

luolanzone · 2021-12-21T01:31:03Z

Makefile

+	$(CURDIR)/hack/add-license.sh 
+else
+	@echo "===> Adding License for files <==="
+	$(CURDIR)/hack/add-license.sh --add


I feel it's unecessary to add add-license.sh, Could you just add the CLI here? we can already tell if it's check or add action in Makefile.

luolanzone · 2021-12-21T01:31:31Z

.github/workflows/go.yml

@@ -84,6 +84,8 @@ jobs:
    - name: Run golangci-lint for netpol
      working-directory: hack/netpol
      run: make golangci
+    - name: Check copyright
+      run: ./ci/check-copyright.sh


this should be a make target, not a shell script.

@luolanzone I think we need a check step here? So we just need a shell script to generate git diff and return some err message like manifest? And we just check this in one step, so we don't need two shell scripts within one make target like test-tidy?

I didn't mean there is unnecessary for check step, I mean you can add a check target in Makefile instead of a separate shell file considering you only have one parameter to determine if it's check or not. I personally think it would be simpler if it's just a few lines.

I don't think a Makefile target is strictly necessary (we don't have one for codegen, manifests). However I don't know why the CI step is part of the golangci-lint job and not part of the tidy-codegen-manifest job.

yes, I talked with bangqi offline, after check the codes, I feel it's OK to keep them in a shell script.

will move to the tidy-codegen-manifest, I think this job makes more sense

pkg/agent/config/traffic_encap_mode_test.go

luolanzone · 2022-01-04T09:57:43Z

.github/workflows/go.yml

@@ -84,6 +84,8 @@ jobs:
    - name: Run golangci-lint for netpol
      working-directory: hack/netpol
      run: make golangci
+    - name: Check copyright
+      run: ./ci/check-copyright.sh


I didn't mean there is unnecessary for check step, I mean you can add a check target in Makefile instead of a separate shell file considering you only have one parameter to determine if it's check or not. I personally think it would be simpler if it's just a few lines.

Makefile

antoninbas · 2022-01-04T22:23:36Z

.github/workflows/go.yml

@@ -84,6 +84,8 @@ jobs:
    - name: Run golangci-lint for netpol
      working-directory: hack/netpol
      run: make golangci
+    - name: Check copyright
+      run: ./ci/check-copyright.sh


I don't think a Makefile target is strictly necessary (we don't have one for codegen, manifests). However I don't know why the CI step is part of the golangci-lint job and not part of the tidy-codegen-manifest job.

antoninbas · 2022-01-04T22:24:26Z

Makefile

+add-copyright: 
+	@echo "==> Installing addlicense <=="
+	@$(GO) install github.com/google/addlicense@latest
+ifeq ($(check), true)


I don't think this is very user-friendly (having to define the check variable)

maybe we can define two targets, one is check-copyright, one is add-copyright? or @bangqipropel can change back to original code which was using check-copyright --add to add copy right, then developer can use check-copyright for checking only.

I think check-copyright and add-copyright is better one, for now.
I think the key is here: we don't want to set var by the format "XX=YY" but the format "--XX" in the make command, but dealing with arg with dash is complicated in Makefile command, (like here). It will make the code much more complicated or let the command as make add-copyright -- --check, two double-dash also seems like unfriendly to user, either.

I am OK to add two targets.

bangqipropel · 2022-01-06T08:09:31Z

why is this being merged into feature/multi-cluster instead of main?

changed

antoninbas · 2022-01-07T23:57:14Z

Makefile

+	@echo "==> Installing addlicense <=="
+	@GO=$(GO) $(CURDIR)/hack/add-license.sh
+
+.PHONY: add-copyright
+add-copyright: 
+	@echo "==> Installing addlicense <=="


I don't think the "Installing addlicense" is necessary or even makes sense here

moved to the shell script before the "go install" line

antoninbas · 2022-01-08T00:02:52Z

hack/add-license.sh

+else
+    echo "===> Checking License for files <==="
+    addlicense -c "Antrea Authors." -check `find . -type f -name "*.go"` `find . -type f -name "*.sh"`
+    echo "Have checked out files without copyright, if you want to add them, please run 'make add-copyright'"


If the previous command fails the script exists immediately and the message is never printed, so it's not right
you can do something like this if you want

rc=0 addlicense -c "Antrea Authors." -check `find . -type f -name "*.go"` `find . -type f -name "*.sh"` || rc=1 if [[ $rc -ne 0 ]]; then echo "Some files do not have a correct copyright" exit $rc fi

or you can omit the message altogether. The current message ("if you want to add them, please run 'make add-copyright'") is not ideal in my opinion because there is no reason to refer to a make target from this script.

removed the message

antoninbas · 2022-01-10T18:28:58Z

/skip-all

luolanzone · 2022-02-09T00:33:41Z

@bangqipropel Could you help to resolve the conflicts? thanks.

luolanzone · 2022-02-15T05:54:04Z

Hi @antoninbas @tnqn Could you help to review this issue again, @bangqipropel has fixed the code conflicts. thanks.

antoninbas · 2022-02-15T22:05:52Z

/skip-all

antoninbas · 2022-02-16T00:23:42Z

I approved bu looks like the CI checks are not running correctly. You may need to close and re-open the PR?

bangqipropel · 2022-02-16T00:40:22Z

I approved bu looks like the CI checks are not running correctly. You may need to close and re-open the PR?
@antoninbas
Have done this but some of the Jenkins still report errors, I also have this error on other PRs, is there something wrong with Jenkins?

antoninbas · 2022-02-16T00:51:49Z

.github/workflows/go.yml

@@ -164,6 +164,8 @@ jobs:
      run: ./ci/check-codegen.sh
    - name: Check manifest
      run: ./ci/check-manifest.sh
+    - name: Check copyright
+    run: ./ci/check-copyright.sh


I notice that there is a formatting issue here. Could it be why the CI jobs are not running for your PR (I notice that the jobs not running are the ones defined in this file)?

antoninbas · 2022-02-16T00:53:09Z

Most Jenkins jobs a re marked as failed when they do not run. /skip-all should make then "green" but maybe the command doesn't cover all jobs (only the required ones).

1. add make check-copyright and add-copyright in Makefile to check and add copyright for files automatically 2. check if all needed copyright is added in workflow 3. add copyright for files Closes antrea-io#3100 Signed-off-by: zbangqi <zbangqi@vmware.com>

antoninbas · 2022-02-18T23:04:40Z

/skip-all

antoninbas

Looks like CI issues have been resolved

1. add make check-copyright and add-copyright in Makefile to check and add copyright for files automatically 2. check if all needed copyright headers have been added with workflow 3. add missing copyright headers Closes antrea-io#3100 Signed-off-by: zbangqi <zbangqi@vmware.com>

bangqipropel changed the base branch from main to feature/multi-cluster December 10, 2021 23:08

bangqipropel marked this pull request as draft December 10, 2021 23:12

bangqipropel marked this pull request as ready for review December 10, 2021 23:13

luolanzone reviewed Dec 13, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

luolanzone force-pushed the feature/multi-cluster branch 2 times, most recently from 14f9a10 to f55dbad Compare December 13, 2021 07:16

bangqipropel force-pushed the addlicense branch 7 times, most recently from d6d9b72 to 2c88847 Compare December 16, 2021 04:28

bangqipropel force-pushed the addlicense branch from 2c88847 to 86be0df Compare December 16, 2021 04:56

luolanzone reviewed Dec 21, 2021

View reviewed changes

luolanzone force-pushed the feature/multi-cluster branch from f9e5f48 to fb5ddfc Compare December 22, 2021 01:08

bangqipropel force-pushed the addlicense branch 2 times, most recently from 9878a9e to 34a67c9 Compare January 4, 2022 09:55

luolanzone reviewed Jan 4, 2022

View reviewed changes

bangqipropel force-pushed the addlicense branch from 34a67c9 to 4bcb0da Compare January 4, 2022 10:08

luolanzone requested review from antoninbas and jianjuns January 4, 2022 10:19

bangqipropel force-pushed the addlicense branch from 4bcb0da to 6d9d741 Compare January 4, 2022 10:20

luolanzone reviewed Jan 4, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

bangqipropel force-pushed the addlicense branch from 6d9d741 to b9cdf2e Compare January 4, 2022 10:25

antoninbas reviewed Jan 4, 2022

View reviewed changes

bangqipropel force-pushed the addlicense branch from a6d6387 to 144d8d6 Compare January 6, 2022 08:07

bangqipropel changed the base branch from feature/multi-cluster to main January 6, 2022 08:09

bangqipropel force-pushed the addlicense branch 2 times, most recently from d1cfbec to 4d68a96 Compare January 6, 2022 22:38

antoninbas reviewed Jan 8, 2022

View reviewed changes

bangqipropel force-pushed the addlicense branch from 4d68a96 to 6ea168f Compare January 10, 2022 08:28

antoninbas previously approved these changes Jan 10, 2022

View reviewed changes

bangqipropel dismissed antoninbas’s stale review via 17bd71f February 9, 2022 02:41

bangqipropel force-pushed the addlicense branch 2 times, most recently from 17bd71f to b6fb678 Compare February 9, 2022 02:48

antoninbas previously approved these changes Feb 15, 2022

View reviewed changes

bangqipropel closed this Feb 16, 2022

bangqipropel reopened this Feb 16, 2022

antoninbas reviewed Feb 16, 2022

View reviewed changes

addlicense

1546a8d

1. add make check-copyright and add-copyright in Makefile to check and add copyright for files automatically 2. check if all needed copyright is added in workflow 3. add copyright for files Closes antrea-io#3100 Signed-off-by: zbangqi <zbangqi@vmware.com>

bangqipropel dismissed antoninbas’s stale review via 1546a8d February 18, 2022 21:27

bangqipropel force-pushed the addlicense branch from b6fb678 to 1546a8d Compare February 18, 2022 21:27

antoninbas approved these changes Feb 18, 2022

View reviewed changes

antoninbas merged commit 3f30d84 into antrea-io:main Feb 18, 2022

luolanzone mentioned this pull request Jun 13, 2022

Add copy right info for shell scripts and Dockerfiles #3887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add license automatically #3122

Add license automatically #3122

bangqipropel commented Dec 10, 2021 •

edited

Loading

luolanzone commented Dec 13, 2021

bangqipropel commented Dec 13, 2021

luolanzone commented Dec 13, 2021 •

edited

Loading

codecov-commenter commented Dec 16, 2021 •

edited

Loading

luolanzone Dec 21, 2021

luolanzone Dec 21, 2021

bangqipropel Jan 4, 2022

luolanzone Jan 4, 2022 •

edited

Loading

antoninbas Jan 4, 2022

luolanzone Jan 5, 2022

bangqipropel Jan 5, 2022

luolanzone Jan 4, 2022 •

edited

Loading

antoninbas Jan 4, 2022

antoninbas Jan 4, 2022

luolanzone Jan 5, 2022

bangqipropel Jan 5, 2022 •

edited

Loading

luolanzone Jan 5, 2022 •

edited

Loading

bangqipropel commented Jan 6, 2022

antoninbas Jan 7, 2022

bangqipropel Jan 10, 2022

antoninbas Jan 8, 2022 •

edited

Loading

bangqipropel Jan 10, 2022

antoninbas commented Jan 10, 2022

luolanzone commented Feb 9, 2022

luolanzone commented Feb 15, 2022

antoninbas commented Feb 15, 2022

antoninbas commented Feb 16, 2022

bangqipropel commented Feb 16, 2022

antoninbas Feb 16, 2022

antoninbas commented Feb 16, 2022

antoninbas commented Feb 18, 2022

antoninbas left a comment

Add license automatically #3122

Add license automatically #3122

Conversation

bangqipropel commented Dec 10, 2021 • edited Loading

luolanzone commented Dec 13, 2021

bangqipropel commented Dec 13, 2021

luolanzone commented Dec 13, 2021 • edited Loading

codecov-commenter commented Dec 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bangqipropel Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

luolanzone Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

bangqipropel commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas commented Jan 10, 2022

luolanzone commented Feb 9, 2022

luolanzone commented Feb 15, 2022

antoninbas commented Feb 15, 2022

antoninbas commented Feb 16, 2022

bangqipropel commented Feb 16, 2022

Choose a reason for hiding this comment

antoninbas commented Feb 16, 2022

antoninbas commented Feb 18, 2022

antoninbas left a comment

Choose a reason for hiding this comment

bangqipropel commented Dec 10, 2021 •

edited

Loading

luolanzone commented Dec 13, 2021 •

edited

Loading

codecov-commenter commented Dec 16, 2021 •

edited

Loading

luolanzone Jan 4, 2022 •

edited

Loading

luolanzone Jan 4, 2022 •

edited

Loading

bangqipropel Jan 5, 2022 •

edited

Loading

luolanzone Jan 5, 2022 •

edited

Loading

antoninbas Jan 8, 2022 •

edited

Loading