chore(ci): Fix pre-commit/action, as v2.x not work anymore
#878
Conversation
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe GitHub Actions workflow for pre-commit checks was updated to streamline the process of handling autofix commits on pull requests. The workflow now includes a second checkout step with full history and SSH authentication to allow pushing autofix commits back to the PR branch. The pre-commit action was upgraded, and redundant steps were removed. If pre-commit detects issues, the workflow now automatically commits and pushes fixes using a dedicated action, triggering the workflow to re-run on the updated commit. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Repo (PR Branch)
participant Pre-commit
participant EndBug/add-and-commit
GitHub Actions->>Repo (PR Branch): Checkout PR branch (full history, SSH)
GitHub Actions->>Pre-commit: Run pre-commit with autofix enabled
alt Pre-commit passes
Pre-commit-->>GitHub Actions: Success
else Pre-commit fails (autofixes made)
Pre-commit-->>GitHub Actions: Failure (autofixes staged)
GitHub Actions->>EndBug/add-and-commit: Commit and push autofixes to PR branch
EndBug/add-and-commit->>Repo (PR Branch): Push autofix commit
Note right of Repo (PR Branch): Workflow re-triggers on new commit
end
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates the GitHub Actions workflow to fix issues with the legacy pre-commit action and migrate to the supported version, ensuring that automated fixes are pushed correctly.
- Update pre-commit/action from v2.0.3 to v3.0.1
- Add a checkout step with full history and SSH key support for autofix commits
- Introduce a push fixes step using EndBug/add-and-commit to apply autofix changes
Comments suppressed due to low confidence (1)
.github/workflows/pre-commit.yaml:81
- Using 'if: failure()' for the 'Push fixes' step will only trigger the push on failure; verify that this condition meets the intended workflow behavior.
if: failure()
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/pre-commit.yaml (5)
69-78: Guard pre-commit execution and pin to a version tag.
- Pin the action to a clear tag (e.g.,
pre-commit/action@v3.0.1) rather than a raw commit SHA for readability.- Protect against an empty file list (when
steps.file_changes.outputs.filesis blank) to avoid invocation errors. For example:- name: Execute pre-commit if: steps.file_changes.outputs.files != '' uses: pre-commit/action@v3.0.1 env: SKIP: no-commit-to-branch with: extra_args: >- --color=always --show-diff-on-failure --files ${{ steps.file_changes.outputs.files }}Or fall back to
--allwhen no files are detected.
80-94: Enhance Add-and-Commit configuration.
- Pin the action to a tag (e.g.,
EndBug/add-and-commit@v9.1.4) instead of a SHA for easier upgrades.- Explicitly set the
branchinput to ensure fixes land on the PR branch:- uses: EndBug/add-and-commit@a94899bca583c204427a224a7af87c02f9b325d5 + uses: EndBug/add-and-commit@v9.1.4 with: + branch: ${{ github.event.pull_request.head.ref }} default_author: github_actor message: '[pre-commit] Autofix violations'This prevents accidental pushes to unintended refs and makes the intent explicit.
55-55: Remove outdated comment about Terraform.The comment
# Skip terraform_tflint which interferes to commit pre-commit auto-fixesno longer reflects any logic here. Please delete or update it to match the current workflow.
51-58: Reorder checkout and setup steps for clarity.Having an
actions/setup-pythonstep placed between two checkouts can be confusing. Consider this sequence:
- Checkout repo (with fetch-depth and SSH or HTTP configured).
- Setup Python environment.
- Run pre-commit and push fixes.
Grouping related actions improves readability and reduces cognitive load.
1-9: Consider concurrency and forked PR handling.
- Add a
concurrencygroup keyed by the PR number to cancel stale runs when new commits arrive.- If you need to commit autofixes on PRs from forks, secrets (SSH key, GITHUB_TOKEN) won’t be available under a
pull_requesttrigger. You may need to switch topull_request_targetand lock down paths to mitigate security risks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pre-commit.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: 🧪 Tests / pytest@🐍3.9@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.13@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.12@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.11@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.10@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-13
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
🔇 Additional comments (1)
.github/workflows/pre-commit.yaml (1)
72-73: Review theSKIPenvironment variable scope.Currently SKIP is set to only
no-commit-to-branch. Confirm that all desired hooks (e.g., hadolint) remain enabled or explicitly skipped as intended. Adjust the comma-separated list if more hooks need to be bypassed during the autofix run.
| fetch-depth: 0 | ||
| ref: ${{ github.event.pull_request.head.ref }} | ||
| # Need to trigger pre-commit workflow on autofix commit. Guide: | ||
| # https://web.archive.org/web/20210731173012/https://github.community/t/required-check-is-expected-after-automated-push/187545/ |
There was a problem hiding this comment.
Thoughts aloud: a bit odd to link discussion that doesn't exist anymore (was there a reason to remove it from public access? 🤔)
There was a problem hiding this comment.
The other guy on the thread explicitly discourages against this approach 🤔
You really really don’t want to do this.
I guess I'll take my approval back and leave this to @antonbabenko as a repo owner.
There was a problem hiding this comment.
On the other hand it shouldn't be dangerous as default Deploy Key permission is readonly, but in the same time GitHub and the EndBug/add-and-commit recommend to use PAT instead: https://github.com/EndBug/add-and-commit#the-commit-from-the-action-is-not-triggering-ci
@MaxymVlasov Why did you choose to use Deploy Key rather than PAT? 🤔
There was a problem hiding this comment.
Because PAT is Personal Access Token, which can expire, rotate or removed during usual routine of key management by user.
When ssh key is fully incapsulated inside repo.
Whatever, we can swith to pre-commit light GHA, I just don't want spend time to setup and support stuff that I will not be able to use anywhere except public repos
There was a problem hiding this comment.
I got your point. Thanks for details.
We anyways need an action from @antonbabenko to proceed with this, right?
There was a problem hiding this comment.
I have just added the required keys to the GH repository secrets. Let me know if there is anything else you need from me.
| default_author: github_actor | ||
| # The message for the commit. | ||
| # Default: 'Commit from GitHub Actions (name of the workflow)' | ||
| message: '[pre-commit] Autofix violations' |
There was a problem hiding this comment.
It is not essential, but just a bit that made me re-read the message twice: the "Autofix violations" can be taken as "violations related to autofix", so I'd suggest to eliminate this confusion by rewording it a tiny bit:
| message: '[pre-commit] Autofix violations' | |
| message: '[pre-commit] Autofix rule violations' |
Maybe replace "rule" with "policy". Or something similar.
Up to you though
There was a problem hiding this comment.
| message: '[pre-commit] Autofix violations' | |
| message: '[pre-commit] Fix violations' |
There was a problem hiding this comment.
It's still "Порушення виправлень" або "Виправлення порушень".
I'm going to approve this PR as a whole, though the "Autofix violations" is better for clarity. <ITEM> can be "coding style", "coding standards", "coding rules", whatever matches best with what kind of fixes this is applied to.
I also think "Auto" prefix is good enough to designate the commit is done by automation rather than by a human.
Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/pre-commit.yaml (4)
56-59: Use a semantic version tag foractions/setup-python
Pinning to a specific commit hash can make upgrades and audits harder. Consider using the official version tag (e.g.@v5.4.0) instead of a SHA to benefit from patch fixes within the same major/minor release.Proposed diff:
- - uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5.4.0 + - uses: actions/setup-python@v5.4.0 with: python-version: '3.13'
75-78: Guard against empty file list in--filesargument
Ifsteps.file_changes.outputs.filesis empty (e.g., no diffable files),--fileswill error. Consider adding a safeguard or conditional skip:extra_args: >- - --color=always \ - --show-diff-on-failure \ - --files ${{ steps.file_changes.outputs.files }} + --color=always \ + --show-diff-on-failure \ + ${{ steps.file_changes.outputs.files && format('--files {0}', steps.file_changes.outputs.files) }}Or add a preceding step to bail out when no files have changed.
79-79: Refine comment wording for clarity
The comment reads# Needed to trigger pre-commit workflow on autofix commitbut could be misinterpreted. For example:# Enable this checkout for the Add-and-Commit step to re-trigger CI on autofix commits
93-93: Clarify commit message template
The message'[pre-commit] Fix violations'is concise but might blend into other fixes. Consider something like:[pre-commit] Autofix lint violations in changed filesto distinguish automated fixes in your history.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pre-commit.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.13@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.10@windows-2025
🔇 Additional comments (2)
.github/workflows/pre-commit.yaml (2)
61-68: Ensure correct credential persistence when using SSH checkout
When using thessh-keyinput, it’s generally recommended to disable persisted HTTP credentials (persist-credentials: false) to avoid mixing auth methods. If you rely solely on SSH for pushes, explicitly set:- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: - fetch-depth: 0 + fetch-depth: 0 + persist-credentials: false ref: ${{ github.event.pull_request.head.ref }} # Needed to trigger pre-commit workflow on autofix commit. Guide: # https://web.archive.org/web/20210731173012/https://github.community/t/required-check-is-expected-after-automated-push/187545/ ssh-key: ${{ secrets.GHA_AUTOFIX_COMMIT_KEY }}If you need HTTP credentials for subsequent steps, please confirm that this checkout step still behaves as expected.
72-73: Validate theSKIPenvironment variable usage
TheSKIP: no-commit-to-branchsetting skips theno-commit-to-branchhook, but confirm that:
- The hook ID matches your
.pre-commit-config.yaml.- The correct env var name is used (
SKIPvsPRE_COMMIT_SKIP) according to the pre-commit action docs.
|
This PR is included in version 1.99.1 🎉 |
Description of your changes
Deal with
https://github.com/antonbabenko/pre-commit-terraform/actions/runs/14755939444/job/41424052063?pr=589
How can we test changes
@antonbabenko please,
ssh-keygen -t ed25519locallyGHA_AUTOFIX_COMMIT_KEYat https://github.com/antonbabenko/pre-commit-terraform/settings/secrets/actionsGHA_AUTOFIXat https://github.com/antonbabenko/pre-commit-terraform/settings/keysAnd then we will be able to test it this PR