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

[skip changelog] Automate license headers checks in CI #2177

Conversation

alessio-perugini
Copy link
Contributor

@alessio-perugini alessio-perugini commented May 15, 2023

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

This adds a new CI check for new go files missing the license headers.
We have to put in the root of the folder a template called: license_header.tpl. This file will be used by addlicense to add any missing license header in the go file.

What is the current behavior?

If we create a new go file and forget to add a license header we can spot it only by a manual CR which might be error-prone if the PR is large.

What is the new behavior?

Every time a new golang file is pushed with a missing license header the CI will fail and warn the user.

Does this PR introduce a breaking change, and is titled accordingly?

Other information

Any suggestion are welcomed

@alessio-perugini alessio-perugini linked an issue May 15, 2023 that may be closed by this pull request
3 tasks
@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (ca79383) 62.83% compared to head (cf53299) 62.83%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2177   +/-   ##
=======================================
  Coverage   62.83%   62.83%           
=======================================
  Files         220      220           
  Lines       19477    19477           
=======================================
  Hits        12239    12239           
  Misses       6150     6150           
  Partials     1088     1088           
Flag Coverage Δ
unit 62.83% <ø> (ø)

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

Impacted Files Coverage Δ
internal/cli/arguments/completion.go 87.67% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alessio-perugini alessio-perugini changed the title Automate license headers checks in CI [skip changelog] Automate license headers checks in CI May 17, 2023
@alessio-perugini alessio-perugini force-pushed the 2165-automate-license-headers-handling-add-checks-in-ci-to-enforce-it branch 4 times, most recently from 1b8105f to b7ce834 Compare May 17, 2023 11:01
@alessio-perugini alessio-perugini marked this pull request as ready for review May 17, 2023 14:14
@alessio-perugini alessio-perugini self-assigned this May 17, 2023
@alessio-perugini alessio-perugini added the type: enhancement Proposed improvement label May 17, 2023
@cmaglie
Copy link
Member

cmaglie commented May 18, 2023

I tried to change the license in main.go, but running the check would not detect it. I guess it only generates the license header when completely missing?

Can this be changed to detect also if a license is not the correct one?

@alessio-perugini alessio-perugini force-pushed the 2165-automate-license-headers-handling-add-checks-in-ci-to-enforce-it branch 4 times, most recently from 9720ca1 to 1401806 Compare May 22, 2023 16:18
docsgen/main.go Outdated Show resolved Hide resolved
); do
# We read only the license header, removing all the comments symbols and changing the date to be 2000. This will
# help us understand if the other part of the license are exactly the same as the template.
HEADER=$(head -n$N_LINES $file | sed -r 's| [0-9]{4} | 2000 |' | sed 's|// ||; s|^//||' )
Copy link
Member

Choose a reason for hiding this comment

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

We must keep the year range 2020-2023. I did some research and found that:

  1. the starting year must be kept to indicate when the copyright started (otherwise any previous work may not be considered under copyright)
  2. the ending year must be kept to indicate when the copyright expires (it happens after the last year + some decades actually...)
  3. we cannot use 2020-present...
  4. old headers should be updated every time a file is touched (for example if a file made in 2020 is modified in 2023 then the ending year in the header must be updated with 2020-2023)

At this point, maybe the simplest solution is to just put the year range in the template file and update the template + all source files every year?

Comment on lines 264 to 269
if [ "$LICENSE" != "$HEADER" ]; then
STATUS=1
echo "$file"
fi
done
exit $STATUS
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this check because it just tells you that a file is wrong but didn't tell you what's wrong, and we don't have any way to re-run it locally. Also running task go:add-license-headers won't fix the wrong file.

Ideally, the action should just run task go:add-license-header and check if there are changes with git diff.
(more or less the same as the check for protoc files https://github.com/arduino/arduino-cli/blob/master/.github/workflows/check-protobuf-task.yml#L130).

In other words task go:add-license-header should write the files as they should be before pushing the PR.

@alessio-perugini alessio-perugini force-pushed the 2165-automate-license-headers-handling-add-checks-in-ci-to-enforce-it branch 3 times, most recently from b65d97c to 86ad5e5 Compare May 24, 2023 13:39
@alessio-perugini
Copy link
Contributor Author

Ideally, we'd like to have a tool that can recognize and fix unwanted changes in the license header without being only limited to checking the presence of a license header. We tried to implement something quick with bash, but it's more tricky than expected. For example, do we need to provide a date range for our license headers? Or it's okay to keep the year when the file was created? In case we have to opt for a year range, should we always put 2020-CURRENT_YEAR?

We agreed that the most common mistake is to forget to add the license for a newly created file. Without overcomplicating this, the addlicense should be enough for now.

@umbynos umbynos force-pushed the 2165-automate-license-headers-handling-add-checks-in-ci-to-enforce-it branch from 86ad5e5 to cf53299 Compare May 26, 2023 13:50
@alessio-perugini alessio-perugini merged commit 15a5e88 into master May 26, 2023
197 checks passed
@alessio-perugini alessio-perugini deleted the 2165-automate-license-headers-handling-add-checks-in-ci-to-enforce-it branch May 26, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate license headers handling / add checks in CI to enforce it
2 participants