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

Refine workflow for generating test-ubuntu-git #1617

Merged
merged 2 commits into from Feb 21, 2024
Merged

Conversation

jww3
Copy link
Contributor

@jww3 jww3 commented Feb 20, 2024

The previous implementation used env.GITHUB_SHA to tag the generated container image, but it was flawed. The actual SHA was not embedded in the container image tag list.

The problem stems from this nuance of Github Actions.

The default environment variables that GitHub sets are available to every step in a workflow.

Because default environment variables are set by GitHub and not defined in a workflow, 
they are not accessible through the env context. However, most of the default variables 
have a corresponding, and similarly named, context property.

For example, the value of the GITHUB_REF variable can be read during workflow processing
using the ${{ github.ref }} context property.

Two possible fixes are:

  1. use inline variable expansion (i.e. $GITHUB_SHA)
  2. retrieve the SHA via the github context (i.e. github.sha)

However, this flaw caused me to rethink the tagging strategy altogether. SHAs are useful, but don't communicate much to the consumer. The consumer has to walk back through the history to understand when the SHA was in play. In other words, it's impossible to get a sense of recency when looking at a SHA in isolation.

Semantic Versioning (e.g. v1.0) is typically the solution, but actions/checkout already employs semantic versioning to communicate versions of the consumable action it represents. I worried that having two semantic versioning systems in play within the same repo could lead to confusion. Moreover, implementing semantic versioning correctly for container images (commonly via docker/metadata-action) also has its own complexity and gotchas.

Above all, I expect that updates to the test-ubuntu-git container image will be very infrequent (about once per year), so I decided to go with a simpler approach here and tag the container images with a UTC timestamp.

The format is inspired by ISO 8601 (sortable date/time) and has the following format:
<branch>.<year><month><day>.<hour><minute><second>.<millisecond>Z (where the trailing Z emphasizes that this is a UTC timestamp)

Example

main.20240221.105506.339Z

Also in this PR

  • Gave the workflow a more concise name.
  • Restricted the publish (push to ghcr.io) opt-in behavior to the main branch.
  • Updated the publish input parameter description to communicate that it's only valid on the main branch.

@jww3 jww3 requested a review from a team as a code owner February 20, 2024 19:19
# labels: ${{ steps.meta.outputs.labels }}
# For now, attempts to push to ghcr.io must target the `main` branch.
# In the future, consider also allowing attempts from `releases/*` branches.
push: ${{ inputs.publish && github.ref_name == 'main' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also pushes :latest right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I haven't included a :latest tag in this implementation.

There's only one known use case (test-proxy), so I'm OK to pinning that use case to an explicit timestamp.

Copy link
Contributor

@fhammerl fhammerl left a comment

Choose a reason for hiding this comment

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

LGTM

@jww3 jww3 merged commit df0bcdd into main Feb 21, 2024
11 of 12 checks passed
@jww3 jww3 deleted the jww3-refine-ubuntu-image branch February 21, 2024 11:46
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.

None yet

2 participants