Skip to content

ci: Refactor stable.yml #163

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

Merged
merged 3 commits into from
May 23, 2025
Merged

ci: Refactor stable.yml #163

merged 3 commits into from
May 23, 2025

Conversation

polarathene
Copy link
Contributor

@polarathene polarathene commented May 23, 2025

For the most part this just pulls the stable check script to the start of the workflow to bail early instead of littering the same conditional in many steps that follow (including build and tests).

The other big change is the final step to assign more tags to the image when it is published. The logic should be the same, I've just rearranged it to be a bit nicer to grok.

NOTE: Given that you've centralized to a single Dockerfile now, and it can be built successfully with multiple platforms (without a build arg for the arch necessary), you could avoid all this dance with the tag management.

  • Only the extraction of the RUST_VER is a little tricky, but you should be able to just append that tag in a post publish step?
  • You've also got the option to use the ARM64 runner for aarch64 image builds instead of the slower QEMU process. For that you'd need to retain the merge workflow if you need to speed up the CI, otherwise it's easier with QEMU.

PLATFORM_PAIR appears unused? Introduced here as part of this PR that introduced ARM64 platform images in Feb 2024.

Basically it turns linux/x86_64 to linux-x86_64, presumably it was intended at some point to be used in part of the image tag naming?:

platform=${{ matrix.platform }}
echo "PLATFORM_PAIR=${platform//\//-}" >> $GITHUB_ENV

Removed it as it doesn't seem to be serving a purpose.


That multi-arch PR also chose to associate the latest tag to the nightly builds for some reason which seems odd? I could switch that over to the stable workflow instead?


I also noticed scheduled builds at 10am (Nightly) and 12pm (Stable), no context was provided for the offset if intentional? (introduced from this commit)

For the most part the stable and nightly workflows are effectively identical. A good portion could be split out and shared with an input of stable / nightly to distinguish the two builds (_which the last step with adding extra tags could use to differentiate assigning latest.

Copy link
Owner

@clux clux left a comment

Choose a reason for hiding this comment

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

Ah, this is nice. I agree that the if conditions are often hard to reason about, and factoring out like this is better for mental overhead.

Comment on lines -55 to -59

- name: Prepare
run: |
platform=${{ matrix.platform }}
echo "PLATFORM_PAIR=${platform//\//-}" >> $GITHUB_ENV
Copy link
Owner

Choose a reason for hiding this comment

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

yeah, this was probably from some older experiments iirc.

@clux
Copy link
Owner

clux commented May 23, 2025

That #133 also chose to associate the latest tag to the nightly builds for some reason which seems odd? I could switch that over to the stable workflow instead?

That was basically to dissuade people from tailing latest (unless you absolutely wanted something unstable) and softly force a choice between other tags. I don't have super strong opinions on it, but I also don't think latest is super useful as a semantic identifier.

I also noticed scheduled builds at 10am (Nightly) and 12pm (Stable), no context was provided for the offset if intentional?

uhh. possibly i wanted to spread out the load. but it doesn't really matter on github runners. 🤷 feel free to change it.

For the most part the stable and nightly workflows are effectively identical. A good portion could be split out and shared with an input of stable / nightly to distinguish the two builds

yeah, maybe your workflow dispatch idea with parameters might make it smaller. feel free to play around!

i'll merge this in the meantime :-)

@clux clux merged commit 451d86c into clux:main May 23, 2025
4 of 6 checks passed
@polarathene
Copy link
Contributor Author

That #133 also chose to associate the latest tag to the nightly builds for some reason which seems odd? I could switch that over to the stable workflow instead?

That was basically to dissuade people from tailing latest (unless you absolutely wanted something unstable) and softly force a choice between other tags. I don't have super strong opinions on it, but I also don't think latest is super useful as a semantic identifier.

Then don't publish a :latest tag. Some image registries may have something in place to implicitly alias that (not sure), but otherwise a CLI will attempt that tag when none is provided and just fail.

Personally :latest is a sane default for trying an image out quickly and not thinking about the tag. Anyone using containers in production should know better than to rely on this tag as that's an anti-pattern.

It's not too different from semver tags or similar to the variants you already offer that can be reassigned over time. A tag is associated to some meaning for which image digest it'll point to, more specific ones aren't likely to be updated, but tags like :latest or :nightly are clearly dynamic.

For this image, there is a difference in behaviour when running nightly vs stable toolchains, so personally I'd expect :latest to point to :stable.

@clux
Copy link
Owner

clux commented May 24, 2025

Yeah, those are all good arguments.

I am a bit skeptical about removing it now given the amount of unqualified uses on github search. It's probable that most of those users expect a stable version, so maybe changing it over is the better option. Then if it's actually a problem they'll either complain or figure out they need to pick :nightly explicitly.

@polarathene
Copy link
Contributor Author

All good, I've got that already handled in a WIP branch (no commits pushed yet). I'll return to that in 12 hours or so.

Since you can switch back to a simpler build process in the CI now for publishing, I'm going to PR that over the manifest management. It'll unify the stable/nightly workflows too, and fix up other tag issues I noticed.

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