Skip to content

chore(ci): Fix pre-commit/action, as v2.x not work anymore #878

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.

Sign up for GitHub

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

Merged
MaxymVlasov merged 3 commits into from
May 2, 2025
Merged

chore(ci): Fix pre-commit/action, as v2.x not work anymore #878

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
afcbc76
chore(ci): Fix pre-commit/action, as v2.x not work anymore
MaxymVlasov May 1, 2025
9589a4e
Apply suggestions from code review
MaxymVlasov May 2, 2025
0756a9d
Apply suggestions from code review
MaxymVlasov May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 28 additions & 29 deletions .github/workflows/pre-commit.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,39 +56,38 @@ jobs:
- uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5.4.0
with:
python-version: '3.13'

# Needed for pre-commit fix push to succeed
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
fetch-depth: 0
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/
Copy link
Copy Markdown
Collaborator

@yermulnik yermulnik May 1, 2025

Choose a reason for hiding this comment

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

Thoughts aloud: a bit odd to link discussion that doesn't exist anymore (was there a reason to remove it from public access? 🤔)

Copy link
Copy Markdown
Collaborator

@yermulnik yermulnik May 1, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

@yermulnik yermulnik May 1, 2025

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Copy Markdown
Collaborator Author

@MaxymVlasov MaxymVlasov May 1, 2025

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

@yermulnik yermulnik May 1, 2025

Choose a reason for hiding this comment

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

I got your point. Thanks for details.
We anyways need an action from @antonbabenko to proceed with this, right?

Copy link
Copy Markdown
Owner

@antonbabenko antonbabenko May 2, 2025

Choose a reason for hiding this comment

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

I have just added the required keys to the GH repository secrets. Let me know if there is anything else you need from me.

ssh-key: ${{ secrets.GHA_AUTOFIX_COMMIT_KEY }}

- name: Execute pre-commit
uses: pre-commit/action@9b88afc9cd57fd75b655d5c71bd38146d07135fe # v2.0.3
uses: pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd # v3.0.1
env:
SKIP: no-commit-to-branch,hadolint
SKIP: no-commit-to-branch
Comment thread
MaxymVlasov marked this conversation as resolved.
with:
token: ${{ secrets.GITHUB_TOKEN }}
extra_args: >-
--color=always
--show-diff-on-failure
--files ${{ steps.file_changes.outputs.files }}
# Run only skipped checks
- name: Execute pre-commit check that have no auto-fixes
if: always()
uses: pre-commit/action@9b88afc9cd57fd75b655d5c71bd38146d07135fe # v2.0.3
env:
SKIP: >-
check-added-large-files,
check-merge-conflict,
check-vcs-permalinks,
forbid-new-submodules,
no-commit-to-branch,
end-of-file-fixer,
trailing-whitespace,
check-yaml,
check-merge-conflict,
check-executables-have-shebangs,
check-case-conflict,mixed-line-ending,
detect-aws-credentials,
detect-private-key,
shfmt,
shellcheck,
--files ${{ steps.file_changes.outputs.files}}

# Needed to trigger pre-commit workflow on autofix commit
- name: Push fixes
if: failure()
uses: EndBug/add-and-commit@a94899bca583c204427a224a7af87c02f9b325d5 # v9.1.4
with:
extra_args: >-
--color=always
--show-diff-on-failure
--files ${{ steps.file_changes.outputs.files }}
# Determines the way the action fills missing author name and email.
# Three options are available:
# - github_actor -> UserName <UserName@users.noreply.github.com>
# - user_info -> Your Display Name <your-actual@email.com>
# - github_actions -> github-actions <email associated with the github logo>
# Default: github_actor
default_author: github_actor
# The message for the commit.
# Default: 'Commit from GitHub Actions (name of the workflow)'
message: '[pre-commit] Autofix violations'
Copy link
Copy Markdown
Collaborator

@yermulnik yermulnik May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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:

Suggested change
message: '[pre-commit] Autofix violations'
message: '[pre-commit] Autofix rule violations'

Maybe replace "rule" with "policy". Or something similar.
Up to you though

Copy link
Copy Markdown
Collaborator Author

@MaxymVlasov MaxymVlasov May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
message: '[pre-commit] Autofix violations'
message: '[pre-commit] Fix violations'

Copy link
Copy Markdown
Collaborator

@yermulnik yermulnik May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Loading