Test centralized GitHub actions config #112
Conversation
There was a problem hiding this comment.
I really like this interface, and I think it's 95% there, with a remaining open question around the way that local pre-commit should be handled. I think this could come in immediately if need be, but am curious to hear what Dan thinks about the local pre-commit issue first!
.gitignore
Outdated
| @@ -2,8 +2,10 @@ venv/ | |||
| __pycache__/ | |||
| ipynb_checkpoints | |||
| glue/schema/*.parquet | |||
| .pre-commit-config.yaml | |||
There was a problem hiding this comment.
[Thought, non-blocking] After thinking about it for a bit, I think that I actually prefer the other side of the tradeoff described in the PR body: It strikes me as slightly friendlier to keep a local copy of the pre-commit config under version control, in spite of the duplication with the ccao-data/actions/pre-commit action, so that developers can run pre-commit locally with minimal overhead. Having to occasionally resolve conflicts between the local pre-commit config and the remote config in ccao-data/actions/pre-commit will sometimes be annoying, but I expect it will be a less common annoyance than requiring every developer to copy the remote config to their local repo before they start development (and then also update their local copy whenever the remote changes).
That being said, this is a weakly-held opinion; what do you two think?
There was a problem hiding this comment.
I agree with the sentiment. A question for me is: Given that we would include both in each repository, how valuable is it to transition to composite actions across all of our repositories? It doesn't seem clear to me that we would be significantly reducing lines of code or reducing the total number of files.
What would positives from this be? I suppose we can have a single source of truth for which to look for all of our hook versions in the actions repo, and that at least upon PRs they are all standardized. Maybe this would be a good start for other actions functionalities I'm not aware of.
There was a problem hiding this comment.
Good point, there's definitely a tradeoff between DRY and developer experience here. I'm comfortable moving forward with the gitignore and just making sure we explain to the rest of the team how they'll be expected to manage local pre-commit configs going forward.
There was a problem hiding this comment.
See my response on the actions PR
Co-authored-by: Jean Cochrane <[email protected]>
…ub.com/ccao-data/model-sales-val into Test-centralized-github-actions-config Merge changes
| run: pre-commit run --show-diff-on-failure --color=always --all-files | ||
| shell: bash | ||
| - name: Run pre-commit checks | ||
| uses: ccao-data/actions/pre-commit@create-composite-pre-commit-action |
There was a problem hiding this comment.
Eventually this branch name will be subbed for main when we merge.
Dual PR with ccao-data/actions#22.
This PR factors out a number of steps in the pre-commit workflow.