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

Fix verify license lint error #5984

Conversation

XiShanYongYe-Chang
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang commented Dec 26, 2024

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

I found that some files I did not add licenses, but lint CI did not report errors, so I checked the hack/verify-license.sh script.

When I run it, it report:

# hack/verify-license.sh
Usage: addlicense [flags] pattern [pattern ...]

The program ensures source code files have copyright license headers
by scanning directory patterns recursively.

It modifies all source files in place and avoids adding a license header
to any file that already has one.

The pattern argument can be provided multiple times, and may also refer
to single files.

Flags:

  -c string
        copyright holder (default "Google LLC")
  -check
        check only mode: verify presence of license headers and exit with non-zero code if missing
  -f string
        license file
  -ignore value
        file patterns to ignore, for example: -ignore **/*.go -ignore vendor/**
  -l string
        license type: apache, bsd, mit, mpl (default "apache")
  -s    Include SPDX identifier in license header. Set -s=only to only include SPDX identifier.
  -skip value
        [deprecated: see -ignore] file extensions to skip, for example: -skip rb -skip go
  -v    verbose mode: print the name of the files that are modified or were skipped
  -y string
        copyright year(s) (default "2024")
hack/verify-license.sh: line 45: .: filename argument required
.: usage: . filename [arguments]

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Dec 26, 2024
@XiShanYongYe-Chang
Copy link
Member Author

/hold

@karmada-bot karmada-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 26, 2024
@RainbowMango
Copy link
Member

Refer to #5357 (comment).

We need to confirm if we really need to ignore .git path.

@codecov-commenter
Copy link

codecov-commenter commented Dec 26, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.24%. Comparing base (e8a6ab3) to head (e93da66).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5984      +/-   ##
==========================================
- Coverage   48.26%   48.24%   -0.02%     
==========================================
  Files         665      665              
  Lines       54788    54788              
==========================================
- Hits        26442    26434       -8     
- Misses      26632    26639       +7     
- Partials     1714     1715       +1     
Flag Coverage Δ
unittests 48.24% <ø> (-0.02%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tiansuo114
Copy link
Contributor

Refer to #5357 (comment).  请参阅 #5357 (comment)

We need to confirm if we really need to ignore .git path.我们需要确认是否真的需要忽略 .git 路径。

When I initially modified the code in hack/verify-license.sh, the issue I encountered was related to lint test errors regarding the missing license certificate content in files under the .git path. At that time, I thought that files under the .git path, similar to files in paths like .github/** or others in the script, should not be checked, so I added an ignore rule for those paths.

As for the current issue in the lint tests for verify license in this PR, I noticed that it seems to be related to missing certificate content in some files. I attempted to add the missing certificates to those files and pushed the changes, which seems to have passed the tests. It might not be related to the issue mentioned in #5357. I can try to identify the problem and fix it based on the additional information you provided.

@RainbowMango
Copy link
Member

I checked the #5357 after revert the changes, it doesn't need to ignore the .git:

-bash-5.0# hack/verify-license.sh 
Congratulations! All files have passed license header check.
-bash-5.0# 
--- a/hack/verify-license.sh
+++ b/hack/verify-license.sh
@@ -40,7 +40,6 @@ missing_license_header_files="$($ADDLICENSE_BIN \
   -ignore "**/*.yml" \
   -ignore "**/*.json" \
   -ignore ".idea/**" \
-  -ignore ".git/**"
   .)" || true
 
 if [[ "$missing_license_header_files" ]]; then

@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the fix-verify-license-lint-error branch from bf014e4 to e93da66 Compare December 26, 2024 11:40
@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 26, 2024
@XiShanYongYe-Chang
Copy link
Member Author

Hi @tiansuo114 I modified the missing license file by the way.

@XiShanYongYe-Chang
Copy link
Member Author

/hold-cancel

@XiShanYongYe-Chang
Copy link
Member Author

I checked the #5357 after revert the changes, it doesn't need to ignore the .git:

I had the same test results.

@RainbowMango
Copy link
Member

RainbowMango commented Dec 26, 2024

Any chance to improve the script to distinguish the error from addlicense?:

-bash-5.0# hack/verify-license.sh 
Usage: addlicense [flags] pattern [pattern ...]

The program ensures source code files have copyright license headers
by scanning directory patterns recursively.

It modifies all source files in place and avoids adding a license header
to any file that already has one.

The pattern argument can be provided multiple times, and may also refer
to single files.

Flags:

  -c string
    	copyright holder (default "Google LLC")
  -check
    	check only mode: verify presence of license headers and exit with non-zero code if missing
  -f string
    	license file
  -ignore value
    	file patterns to ignore, for example: -ignore **/*.go -ignore vendor/**
  -l string
    	license type: apache, bsd, mit, mpl (default "apache")
  -s	Include SPDX identifier in license header. Set -s=only to only include SPDX identifier.
  -skip value
    	[deprecated: see -ignore] file extensions to skip, for example: -skip rb -skip go
  -v	verbose mode: print the name of the files that are modified or were skipped
  -y string
    	copyright year(s) (default "2024")
hack/verify-license.sh: line 45: .: filename argument required
.: usage: . filename [arguments]
Congratulations! All files have passed license header check.

@XiShanYongYe-Chang
Copy link
Member Author

XiShanYongYe-Chang commented Dec 27, 2024

Any chance to improve the script to distinguish the error from addlicense?:

Remove the || true at the command end can help find the err quickly:

--- a/hack/verify-license.sh
+++ b/hack/verify-license.sh
@@ -41,7 +41,7 @@ missing_license_header_files="$($ADDLICENSE_BIN \
   -ignore "**/*.json" \
   -ignore ".idea/**" \
   -ignore ".git/**"
-  .)" || true
+  .)

Test it on my own repo:

Run hack/verify-license.sh
  hack/verify-license.sh
  shell: /usr/bin/bash -e {0}
go: downloading github.com/google/addlicense v1.1.1
go: downloading github.com/bmatcuk/doublestar/v4 v4.0.[2](https://github.com/XiShanYongYe-Chang/karmada/actions/runs/12510379262/job/34901054228#step:4:2)
go: downloading golang.org/x/sync v0.0.0-20190911185100-cd5d95a4[3](https://github.com/XiShanYongYe-Chang/karmada/actions/runs/12510379262/job/34901054228#step:4:3)a6e
Usage: addlicense [flags] pattern [pattern ...]

The program ensures source code files have copyright license headers
by scanning directory patterns recursively.

It modifies all source files in place and avoids adding a license header
to any file that already has one.

The pattern argument can be provided multiple times, and may also refer
to single files.

Flags:

  -c string
    	copyright holder (default "Google LLC")
  -check
    	check only mode: verify presence of license headers and exit with non-zero code if missing
  -f string
    	license file
  -ignore value
    	file patterns to ignore, for example: -ignore **/*.go -ignore vendor/**
  -l string
    	license type: apache, bsd, mit, mpl (default "apache")
  -s	Include SPDX identifier in license header. Set -s=only to only include SPDX identifier.
  -skip value
    	[deprecated: see -ignore] file extensions to skip, for example: -skip rb -skip go
  -v	verbose mode: print the name of the files that are modified or were skipped
  -y string
    	copyright year(s) (default "202[4](https://github.com/XiShanYongYe-Chang/karmada/actions/runs/12510379262/job/34901054228#step:4:5)")
hack/verify-license.sh: line 45: .: filename argument required
.: usage: . filename [arguments]
Error: Process completed with exit code 2.

ref: https://github.com/XiShanYongYe-Chang/karmada/actions/runs/12510379262/job/34901054228

@XiShanYongYe-Chang
Copy link
Member Author

/hold cancel

@karmada-bot karmada-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 27, 2024
@XiShanYongYe-Chang
Copy link
Member Author

It is not easy to distinguish whether the addlicense command executes an error or a file is checked, because their exit status is 1:

# /root/go/bin//addlicense -check -ignore 'vendor/**' -ignore '_output/**' -ignore 'samples/**' -ignore 'docs/**' -ignore '.github/**' -ignore 'third_party/**' -ignore '**/*.md' -ignore '**/*.yaml' -ignore '**/*.yml' -ignore '**/*.json' -ignore '.idea/**' -ignore '.git/**' .
pkg/resourceinterpreter/customized/webhook/serviceresolvers_test.go
pkg/resourceinterpreter/customized/webhook/serviceresolvers.go
pkg/util/helper/index.go
pkg/util/helper/index_test.go
# echo $?
1

# /root/go/bin//addlicense -check -ignore 'vendor/**' -ignore '_output/**' -ignore 'samples/**' -ignore 'docs/**' -ignore '.github/**' -ignore 'third_party/**' -ignore '**/*.md' -ignore '**/*.yaml' -ignore '**/*.yml' -ignore '**/*.json' -ignore '.idea/**' -ignore '.git/**'
Usage: addlicense [flags] pattern [pattern ...]
...

# echo $?
1

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

ok
/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 27, 2024
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 27, 2024
@RainbowMango RainbowMango added this to the v1.13 milestone Dec 27, 2024
@karmada-bot karmada-bot merged commit 8fa7c2c into karmada-io:master Dec 27, 2024
21 checks passed
@RainbowMango
Copy link
Member

@tiansuo114 would you like to backport this to release-1.12?

@tiansuo114
Copy link
Contributor

@tiansuo114 would you like to backport this to release-1.12?

I’m not entirely sure about the meaning of backporting. If you’re referring to synchronizing this change to the release-1.12 branch to address potential issues with the test script, I would support this proposal. Is there anything I can do to help with this process?

@RainbowMango
Copy link
Member

I mean cherry-pick this to branch release-1.12. Here is the guidance.

@tiansuo114
Copy link
Contributor

I mean cherry-pick this to branch release-1.12. Here is the guidance.

OK, I tried to create a cherry-pick PR here. How does it look?

@RainbowMango
Copy link
Member

Looks good, thanks.

karmada-bot added a commit that referenced this pull request Jan 2, 2025
…84-remotes-upstream-release-1.12

Automated cherry pick of #5984: fix verify license lint error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants