-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add automatic copyright notice verification #963
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #963 +/- ##
=======================================
Coverage 94.39% 94.39%
=======================================
Files 93 93
Lines 21060 21060
=======================================
Hits 19880 19880
Misses 1180 1180
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a partial review, I'll do the rest soon.
copyright: | ||
name: Copyright Notices | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout repository | ||
uses: actions/checkout@v2 | ||
- name: Setup Python | ||
uses: actions/setup-python@v2 | ||
- name: Install check-license and dependencies | ||
run: | | ||
pip install scripts/check-license | ||
pip install -r scripts/check-license/requirements.txt | ||
- name: Query files changed | ||
id: files_changed | ||
uses: Ana06/[email protected] | ||
with: | ||
filter: '*.rs$' | ||
- name: Check copyright notices | ||
run: check-license ${{ steps.files_changed.outputs.added_modified }} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script does offer the option to correct those errors, what's your position to automatically adding a commit with the updated license headers and optionally passing the workflow log in a comment in the PR?
- Add a commit: https://github.com/zeitgeistpm/zeitgeist/actions/runs/985953773/workflow#L138-L147
- Comment success/failure + logs: https://github.com/zeitgeistpm/zeitgeist/actions/runs/985953773/workflow#L173-L189
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing would just trigger in the case of a PR that should be merged into main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the commit is triggered by placing a label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with automating the process, but I have to caution everyone to double-check the diff everytime they do this. And just to prove my point, I botched the test for the File.write
function because I forgot how mock
works...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just a quick argument against automating the write: It probably takes just as much time to execute the write locally and then push the changes manually as it takes to trigger the write remotely, but its much safer to make the changes locally and run git diff
once just to check if there's been a mistake.
scripts/check-license/.gitignore
Outdated
# Byte-compiled / optimized / DLL files | ||
__pycache__/ | ||
*.py[cod] | ||
*$py.class | ||
|
||
# C extensions | ||
*.so | ||
|
||
# Distribution / packaging | ||
.Python | ||
build/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the point of adding the .gitignore in that folder and I think its sufficient for almost any case. However, maybe someone decides to initialize a pyenv within the root to cover anything from the root which will create a .python-version
file in the root which is tracked by git. I think it would be better if we include either those rules in the repositories root .gitignore
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't expect such a complex solution, beware copyright notices 😄
This didn't turn out as good as I wanted it to and currently has a couple of limitations (see below). How to use this currently:
scripts/check-license
by runningpip install scripts/check-license
andpip install -r scripts/check-license/requirements.txt
.*.rs
files using https://github.com/Ana06/get-changed-files. This is a fork of https://github.com/jitterbit/get-changed-files which solves this issue and allows us to filter files by their extension: The head commit for this pull_request event is not ahead of the base commit jitterbit/get-changed-files#11. This won't work if any of our files have a whitespace in their name. So... one more reason not to do that, I guess.check-license [files]
. You can also runcheck-license -w [files]
to write the missing copyright notices.Current limitations:
File
object's blob starts with the proper string. It might be helpful to remove the leading"// "
.File
class a little bit. We may want to do that in the future.You can see the workflow fail due to a missing copyright in a Rust file here: https://github.com/zeitgeistpm/zeitgeist/actions/runs/4085940425/jobs/7044586744.
Blocked by #964, which needs to be merged before merging this PR to ensure that we're starting off with a correct state. Closes #925.