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

Introduce reusable workflow to update pre-commit hooks #6

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Jan 29, 2023

Changes

  • Add workflow to update and run pre-commit hooks.

🔴 Required Updates 🔴

  • The BRUTUS_PAT_TOKEN token needs to added as a "normal" organizational secret as well.
    • Currently, this token may only be a "Dependabot secret".

Workflow Implementation

  • Install pre-commit (from the repo's dev extra by default or using the input)
  • Set up pre-commit and run its autoupdate
  • Run pre-commit with the updated hooks
  • Via peter-evans/create-pull-request action, commit all changed files to a branch called autoupdates/pre-commit and add to a PR
  • Adds a changenote

It probably makes the most sense for repos to run this alongside dependabot on Sunday nights. Each time the workflow run, it effectively resets the autoupdates/pre-commit branch....so beware making changes to one of these PRs and letting it sit over the weekend.

The author of the PR that's created is controlled by the user behind the token used (I think).

Example PR: https://github.com/rmartin16/briefcase-test-1/pull/42
Example pre-commit run: https://github.com/rmartin16/briefcase-test-1/actions/runs/4060193182/jobs/6989040382
Example with nothing to update: https://github.com/rmartin16/briefcase--test-2/actions/runs/4060240083/jobs/6989131369
Example demonstrating inputs: https://github.com/rmartin16/briefcase--test-2/actions/runs/4060271416/jobs/6989193718

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16 rmartin16 force-pushed the update-pre-commit branch 13 times, most recently from 98a07e7 to 4878154 Compare January 29, 2023 17:30
@rmartin16 rmartin16 changed the title Introduce reusable workflow to update pre-commit dependenices Introduce reusable workflow to update pre-commit hooks Jan 29, 2023
@rmartin16 rmartin16 force-pushed the update-pre-commit branch 3 times, most recently from 0b9d028 to 77c7ca6 Compare January 29, 2023 18:23
@rmartin16 rmartin16 marked this pull request as ready for review January 29, 2023 18:40
@rmartin16 rmartin16 force-pushed the update-pre-commit branch 2 times, most recently from f666f4e to 7459b0f Compare January 29, 2023 22:01
@rmartin16
Copy link
Member Author

rmartin16 commented Jan 30, 2023

Ok....so, experimenting with the updates to the pre-commit hooks for Briefcase is proving non-trivial to get working.

So, I'm going to create PRs just to accommodate the rename of the dependabot changenote workflow.

This PR probably should not be merged until those PRs are ready to be merged.....or we're gonna break all the dependabot change note workflows.

@rmartin16
Copy link
Member Author

I created the related PRs to handle the rename of the changenote workflow.

This PR (and all those) can now be safely merged.

The effort to introduce pre-commit updating is ongoing. The primary issue is docformatter==1.6.0 (which technically only has RCs released)....it is doing strange things...

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This is some dark magicks... nice work :-)

A couple of high level questions/ideas:

  • Should the pre-commit-update workflow include the change note, rather than requiring a second pass of the changenote workflow to add one (possibly controlled by an input if there are any repos that have precommit but don't have towncrier)? That would seem to avoid the duplicate pass problem, as the dependabot change note would only trigger on commits authored by dependabot, and the pre-commit workflow would be authored by brutus and include a changenote.

  • I presume this has been triggered manually as a result of the workflow_dispatch registration in the briefcase-test-1? In practice, this would be replaced with a schedule-cron workflow, correct?

  • It looks like the if the pre-commit update doesn't discover any changes (i.e., the normal case), the workflow fails - won't this result in a weekly "nothing changed" email because of the failed workflow? Could this be replaced with some logic that catches the $? return value, and sets a "needed=False" flag that is used by the "Create PR" step?

@rmartin16
Copy link
Member Author

This is some dark magicks... nice work :-)

A couple of high level questions/ideas:

  • Should the pre-commit-update workflow include the change note, rather than requiring a second pass of the changenote workflow to add one (possibly controlled by an input if there are any repos that have precommit but don't have towncrier)? That would seem to avoid the duplicate pass problem, as the dependabot change note would only trigger on commits authored by dependabot, and the pre-commit workflow would be authored by brutus and include a changenote.

But then we won't have workflow after workflow triggering each other ;) this definitely makes more sense.

  • I presume this has been triggered manually as a result of the workflow_dispatch registration in the briefcase-test-1? In practice, this would be replaced with a schedule-cron workflow, correct?

Yep; the schedule-cron will need to be configured in the workflow set up in each repo (like in this example); otherwise, the workflow would just run for this repo.

  • It looks like the if the pre-commit update doesn't discover any changes (i.e., the normal case), the workflow fails - won't this result in a weekly "nothing changed" email because of the failed workflow? Could this be replaced with some logic that catches the $? return value, and sets a "needed=False" flag that is used by the "Create PR" step?

I actually tested the case where nothing changes and therefore there's nothing to commit. In that case, the peter-evans/create-pull-request action simply quietly finishes without creating a branch or pushing anything up to GitHub. Although, an explicit handling of this case probably makes more sense.

@rmartin16 rmartin16 force-pushed the update-pre-commit branch 6 times, most recently from 1f35ff7 to 3c16f33 Compare February 1, 2023 01:57
@rmartin16
Copy link
Member Author

rmartin16 commented Feb 1, 2023

Removed depenabot changenote workflow changes from scope and updated pre-commit workflow to add changenote itself.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I think this all makes sense, and the demo repo seems to indicate it works in practice, so I guess we'll go with it!

I've also added the BRUTUS_PAT_TOKEN as an organization-wide secret.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants