-
Notifications
You must be signed in to change notification settings - Fork 19
chore(dev): adopt pre-commit hooks #144
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
base: main
Are you sure you want to change the base?
Conversation
c9a1290 to
9de708c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
tekktrik
left a comment
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.
Some changes and requests for clarification but this is great that were almost there.
| # SPDX-FileCopyrightText: 2017 Scott Shawcroft, written for Adafruit Industries | ||
| # | ||
| # SPDX-License-Identifier: Unlicense |
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 pre-commit configuration should have the following licensing:
| # SPDX-FileCopyrightText: 2017 Scott Shawcroft, written for Adafruit Industries | |
| # | |
| # SPDX-License-Identifier: Unlicense | |
| # SPDX-FileCopyrightText: 2024 Justin Myers for Adafruit Industries | |
| # | |
| # SPDX-License-Identifier: Unlicense |
| with: | ||
| persist-credentials: false |
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 don't often see this argument. Looking into it I don't think this is an issue, but I would comment why it's here so others are confused in the future (especially if authenticated git commands are required for any reason).
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.
See rationale for zizmor's audit "artipacked" rule.
IIRC, you were the one tasked with writing Adafruit's reusable workflows, right? If so, you might want to check out zizmor.
| push: | ||
| branches: [main] | ||
|
|
||
| permissions: {} |
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.
Default permissions are fine and this doesn't need to be specified, since they are typically read-only anyways. If permissions will be specifically revoked, please provide a short comment explaining - this will save someone time in the future (e.g., the default token has permissions that would otherwise be available here that a developer is expecting in some future iteration of this workflow)
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.
See the rationale for zizmor audit "excessive permissions" rule.
This is especially important if you're using an unverified action (directly or from a reusable/composite workflow) that automatically appropriates your env.GITHUB_TOKEN...
FYI, this aligns changes I've made to the other CI workflows in this project.
since they are typically read-only anyways
This depends on the repo settings. Forks can have different settings from upstream's settings.
please provide a short comment explaining
This is simply a best practice approach. So, the comment would be:
| permissions: {} | |
| # Do not inherit GITHUB_TOKEN permissions, for security purposes. | |
| # These will be explicitly granted per job (if/when needed) | |
| permissions: {} |
this will save someone time in the future (e.g., the default token has permissions that would otherwise be available here that a developer is expecting in some future iteration of this workflow)
I'd rather be safe than sorry. If someone alters the CI with a malicious action, there's very little chance to prevent or identify the attack.
FYI, if permissions are needed but not granted (because of this line), then the runner will issue an error stating what step/action is requesting what exact permissions. This will help highlight what sacrifices you're making in the name of convenience.
| on: | ||
| pull_request: | ||
| branches: [main] | ||
| push: | ||
| branches: [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.
I would remove this - it's fine (and desirable) to have this run on development branches.
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 follows the other CI workflows' triggers in this project. Personally I perceive any failure notifications (via email or otherwise) as spam when the branch is not actually ready for review.
What you propose is very annoying for contributors who's forks have CI enabled.
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: '3.12' |
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.
Is there any reason you specifically chose 3.12? You could just specify as "3.X" and it will select the latest version.
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 think it was because that is what is used in the other workflows for the project. I have no objection to using '3.x'.
It would probably make for sense to set a public variable that each workflow can use instead of updating multiple instances of with: { python-version: '3.12' }:
| python-version: '3.12' | |
| python-version: ${{ vars.python-version }} |
| @@ -0,0 +1,31 @@ | |||
| # SPDX-FileCopyrightText: 2017 Scott Shawcroft, written for Adafruit Industries | |||
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.
It's your file - change yourself to the author! 😄
|
Regarding some similar workflow, there is a much larger one that handles things for libraries that are not applicable here (e.g., sphinx documentation and running |
resolves #138
Requires the following PRs merged before review:
__init__.pyfiles #141The pre-commit config was adapted from adafruit/Adafruit_CircuitPython_PCA9554 project's config, but with version upgrades for the specified hooks.
The added CI workflow is just something I whipped up. Maybe there's a Adafruit-maintained reusable workflow of which I'm unaware.