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

Fix sync recipes and add base justfile #2774

Merged
merged 2 commits into from
Feb 19, 2025
Merged

Fix sync recipes and add base justfile #2774

merged 2 commits into from
Feb 19, 2025

Conversation

redsun82
Copy link
Contributor

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.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

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.
@Copilot Copilot bot review requested due to automatic review settings February 19, 2025 15:56
@redsun82 redsun82 requested a review from a team as a code owner February 19, 2025 15:56

Choose a reason for hiding this comment

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

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

NlightNFotis
NlightNFotis previously approved these changes Feb 19, 2025
Copy link
Member

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

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

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?

@@ -0,0 +1,7 @@
sync: build update-pr-checks
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Member

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

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

(Reapproving after review automatically went stale).

Thanks for your work on this @redsun82 , I appreciate it 👍🏻

@redsun82
Copy link
Contributor Author

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?

@NlightNFotis Yeah, thinking back that was working because:

  • on macOS, sh is actually an alias of bash (I'm on Linux where that's not the case)
  • you had the proper package available to python3 outside of the venv, so the last command was also working despite being run outside of the venv

@redsun82 redsun82 enabled auto-merge February 19, 2025 16:17
@redsun82 redsun82 merged commit fb3e7cd into main Feb 19, 2025
267 checks passed
@redsun82 redsun82 deleted the redsun82/sync branch February 19, 2025 16:26
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