Create composite pre commit action #22
Conversation
There was a problem hiding this comment.
Looking great! One blocking suggestion: How about we refactor the pre-commit workflow in this repo to use this new action as well? That way we can always be sure that any changes we make to this action don't introduce any obvious bugs (although there's always the possibility of introducing breaking changes that affect repos that call this action).
pre-commit/action.yaml
Outdated
| - name: Run pre-commit | ||
| run: | | ||
| # Initialize an array to store the parsed hook IDs | ||
| parsed_hooks=() | ||
|
|
||
| # Parse the hook IDs from a space-separated | ||
| # string into an array of strings | ||
| while read -r hook; do | ||
| parsed_hooks+=("$hook") | ||
| done <<< "${{ inputs.hooks }}" # Pass string as input to the while loop | ||
|
|
||
| # Use the array of parsed hooks to run a specific set of hooks | ||
| for hook in ${parsed_hooks[@]}; | ||
| do | ||
| pre-commit run \ | ||
| --show-diff-on-failure \ | ||
| --color=always \ | ||
| --all-files \ | ||
| --config "${{ github.action_path }}/.pre-commit-config.yaml" \ | ||
| "$hook" | ||
| done | ||
| shell: bash |
There was a problem hiding this comment.
Note that I believe we are protected from remote code injection due to the read loop, which parses each token of the hooks input into a string and so protects us from input like hooks: "|| echo helloworld ; exit" (except with something more malicious than echo helloworld). It shouldn't really be a problem if our Actions permissions are configured correctly on the calling repos, but I wanted to make extra sure so I tested this code locally and confirmed that the above string throws an error instead of printing helloworld.
There was a problem hiding this comment.
I should've been more explicit in my issue definition/ask here. The purpose of the issue here is just to codify our existing pre-commit workflow as a composite action, since the code is used across all repos.
We don't really need a way to define the hooks across repos. I'd much prefer to leave that to the local .pre-commit-config.yaml.
The three issues we're trying to solve with the composite action are:
- Standardizing our own pre-commit action rather than the deprecated official one
- Reducing code duplication across repos
- Updating and debugging the caching logic to support our common hooks. The R caching works most of the time but has intermittent, seemingly random failures (that are resolved by re-running the workflow).
Apologies, I should have clarified this issue more.
pre-commit/action.yaml
Outdated
| name: 'pre-commit' | ||
| description: 'Runs pre-commit hooks' |
There was a problem hiding this comment.
super-nitpick: The inconsistency of string usage here kinda bugs me. I would just nix the single quotes unless they're really needed.
pre-commit/action.yaml
Outdated
| - name: Run pre-commit | ||
| run: | | ||
| # Initialize an array to store the parsed hook IDs | ||
| parsed_hooks=() | ||
|
|
||
| # Parse the hook IDs from a space-separated | ||
| # string into an array of strings | ||
| while read -r hook; do | ||
| parsed_hooks+=("$hook") | ||
| done <<< "${{ inputs.hooks }}" # Pass string as input to the while loop | ||
|
|
||
| # Use the array of parsed hooks to run a specific set of hooks | ||
| for hook in ${parsed_hooks[@]}; | ||
| do | ||
| pre-commit run \ | ||
| --show-diff-on-failure \ | ||
| --color=always \ | ||
| --all-files \ | ||
| --config "${{ github.action_path }}/.pre-commit-config.yaml" \ | ||
| "$hook" | ||
| done | ||
| shell: bash |
There was a problem hiding this comment.
praise: Nice thinking @jeancochrane
Co-authored-by: Jean Cochrane <[email protected]>
Co-authored-by: Jean Cochrane <[email protected]>
Co-authored-by: Jean Cochrane <[email protected]>
| - name: Run pre-commit checks | ||
| uses: ccao-data/actions/pre-commit@create-composite-pre-commit-action |
There was a problem hiding this comment.
[Suggestion, non-blocking] Nice! Note that there will be a bit of a catch-22 when merging this -- the new action won't be on the main branch until we merge the PR, so CI checks will fail if you try to adjust the version before merging, but after you merge the branch will be deleted, so checks will also fail. There are two ways to get around this:
- Remove the branch reference before merging, and let someone with admin permissions on this repo (me or Dan) force-merge the PR despite the failing check
- Keep the branch reference while merging, and put up a super simple PR immediately after merging to remove the reference
I'm fine with either, just let me know what your plan is and we'll go from there!
Dual PR with ccao-data/model-sales-val#112.
This PR adds an
action.yamlwhich can be called from other repos. Centering this code will reduce different dependencies and LoC.