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 GHA stuff #563

Merged
merged 4 commits into from
Nov 27, 2024
Merged

Fix GHA stuff #563

merged 4 commits into from
Nov 27, 2024

Conversation

penelopeysm
Copy link
Member

This PR makes a few changes to GitHub Actions workflows:

  1. Separates the preview workflow from the remove-preview workflow.

    Whenever a PR was merged to master, the remove-preview workflow would fire. However, the publish workflow on master would also fire. The changes in Add concurrency key to publish/preview workflows #527 meant that the publish workflow would cancel the remove-preview workflow. The result is that we have a buildup of PR previews.

    This fixes it by making the remove-preview workflow a separate one entirely, so that its running is unaffected by other workflows.

  2. Modifies the version check workflow to not use continue-on-error: true. This causes the workflow to always report a green tick, even if the step did have an error. Instead I use always() on the subsequent steps.

  3. Slightly improves the version checking script.

Instead of calling ]up, it calls ]add Turing@<desired_version>, which
more closely matches our intentions
@penelopeysm
Copy link
Member Author

penelopeysm commented Nov 26, 2024

The effects of (2) are already visible 😄 it is failing because it can't resolve an env with Turing v0.35.2, as expected, see #559 (comment).

Unfortunately, (1) is not visible because it uses the pull_request_target event rather than pull_request. That means that GitHub runs the workflow on the base branch (i.e. master) rather than the workflow in the PR (i.e. the modified one).

Copy link
Contributor

Preview the changes: https://turinglang.org/docs/pr-previews/563
Please avoid using the search feature and navigation bar in PR previews!

Copy link
Member

@shravanngoswamii shravanngoswamii left a comment

Choose a reason for hiding this comment

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

Looks good, thank you, @penelopeysm!

Fixing these issues in GHA is really great. I was aware of the PR-preview removal issue, but we decided to ignore it when merging this repo into Quarto. At that time, we were mainly focused on addressing internal documentation problems, so the GHA workflows were written to just work, but were not fully optimized or cleaned up. It's really helpful to have this addressed now—thanks for your efforts!

@penelopeysm
Copy link
Member Author

Thanks @shravanngoswamii! To be honest, I'm pretty sure that this is still not quite right. There's probably some weird behaviour that I haven't figured out. But with GHA I always think it's fine to merge something imperfect since it can't really cause any bugs in the actual code 😅

@penelopeysm penelopeysm merged commit b7d983f into master Nov 27, 2024
3 of 4 checks passed
@penelopeysm penelopeysm deleted the py/fix-actions branch November 27, 2024 13:02
github-actions bot added a commit that referenced this pull request Nov 27, 2024
@shravanngoswamii
Copy link
Member

To be honest, I'm pretty sure that this is still not quite right. There's probably some weird behaviour that I haven't figured out. But with GHA I always think it's fine to merge something imperfect since it can't really cause any bugs in the actual code 😅

Let's see, we can always change it if it creates any issue, experiments takes the product near perfection😅!

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