Fix sync recipes and add base justfile - #2774
Conversation
Sorry, something went wrong.
Both the justfile and the pre-commit configuration for the `pr-check` sync were broken: * justfiles run recipes one line at a time in a fresh shell, so the venv activation was not working * the pre-commit config was relying on an installed `ruamel.yaml` pakcage, but the default one installable via `apt` on Ubuntu 24.04 is old and generates different output (with formatting differences). Now: * the venv dance is put in a separate bash script * both just and pre-commit will use that same script, so both problems will be fixed As a bonus, a root `justfile` is added exposing the `update-pr-checks` recipes plus a `build` one. Running `just` without arguments will also now call the default `sync` recipes that will call both of the above.
There was a problem hiding this comment.
PR Overview
This PR aims to fix issues related to the sync recipes in both justfiles and the pre-commit configuration by switching from a Python script to a shared bash script for activating the virtual environment and running the recipes.
- Update the pre-commit configuration to use the new bash script (pr-checks/sync.sh)
- Ensure that both just and pre-commit hooks use a unified venv activation mechanism for consistent behavior
Changes
| File | Description |
|---|---|
| .pre-commit-config.yaml | Changed the hook entry from python3 pr-checks/sync.py to pr-checks/sync.sh to use the shared bash script |
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1).pre-commit-config.yaml:19
- Ensure that the pr-checks/sync.sh script is executable and includes a proper shebang so that it runs as intended under the system language.
entry: pr-checks/sync.sh
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
Sorry, something went wrong.
NlightNFotis
left a comment
There was a problem hiding this comment.
Thank you for fixing the justfile.
I seem to recall it working locally when I was last working on this a few months ago, but it might have been incidental?
Sorry, something went wrong.
| @@ -0,0 +1,7 @@ | |||
| sync: build update-pr-checks | |||
There was a problem hiding this comment.
Would it be possible to add a comment on top of the command?
Because we're probably going to forget which one is which after a couple months, so just --list will probably be our fallback then, and it renders the comment as inline help when invoked.
Sorry, something went wrong.
There was a problem hiding this comment.
Good point!
Sorry, something went wrong.
NlightNFotis
left a comment
There was a problem hiding this comment.
(Reapproving after review automatically went stale).
Thanks for your work on this @redsun82 , I appreciate it 👍🏻
Sorry, something went wrong.
@NlightNFotis Yeah, thinking back that was working because:
|
Sorry, something went wrong.
Both the justfile and the pre-commit configuration for the pr-check sync were broken:
Now:
As a bonus, a root justfile is added exposing the update-pr-checks recipes plus a build one. Running just without arguments will also now call the default sync recipes that will call both of the above.
Merge / deployment checklist