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

Build all branches, delete image versions when branches/tags are deleted #14

Merged
merged 9 commits into from
Jul 9, 2024

Conversation

nwiltsie
Copy link
Member

Description

This is a major overhaul of this Action, intended to be released as v2.0.0 (see plan from #12). The gist of these changes is that the Action manages the full lifecycle of images in GHCR: it will now build and deploy images for all branches of the repository, as well as all SemVer tags. When a branch or tag is deleted from GitHub, the corresponding image version is deleted from the container registry. (That is #13, but I'm not going to close that issue until the refactor-base branch is merged back into main).

The git SemVer tag v1.2.3 will result in the docker version 1.2.3 (note the lack of v).

The git branch foobar will result in the docker version branch-foobar. The one exception to this behavior is that the main branch will map to dev - that's in line with the current behavior of this Action.

Note

The default branch name is hard-coded as main. docker/metadata-action has an {{is_default_branch}} expression, but unfortunately there is currently no way to negate it.

Testing

I tested this Action with a fresh repository - the assorted runs can be seen here. The example workflow in the README sets the run-name to a useful value:

Screenshot 2024-06-24 at 12 00 26 PM

Individual workflow runs will emit annotations linking to the newly-created or deleted image versions:

Screenshot 2024-06-24 at 12 00 43 PM Screenshot 2024-06-24 at 12 00 58 PM

Errata

  • The image deletion logic only works for GHCR, so our default usage of <ghcr.io> is now a required usage. I replaced the registry input (default ghcr.io/uclahs-cds) with an organization input (defaults to the organization of the calling repository, usually uclahs-cds for us).
  • The run-name for tags (e.g. "Update v0.0.8 docker tag") incorrectly includes the leading v. GitHub Expressions don't have a substring function, and I couldn't figure out any way to abuse the existing functions to replicate it.

Checklist

  • This PR does NOT contain PHI or germline genetic data. A repo may need to be deleted if such data is uploaded. Disclosing PHI is a major problem.
  • This PR does NOT contain molecular files, compressed files, output files such as images (e.g. .png, .jpeg), .pdf, .RData, .xlsx, .doc, .ppt, or other non-plain-text files. To automatically exclude such files using a .gitignore file, see here for example.
  • I have read the code review guidelines and the code review best practice on GitHub check-list.
  • I have set up or verified the main branch protection rule following the github standards before opening this pull request.
  • The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)]-[brief_description_of_branch].
  • I have added the major changes included in this pull request to the CHANGELOG.md under the next release version or unreleased, and updated the date.

@yashpatel6
Copy link
Collaborator

I'll get to this in the next couple of days!

Copy link
Collaborator

@yashpatel6 yashpatel6 left a comment

Choose a reason for hiding this comment

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

Generally looks good! I'm not as familiar with JavaScript so leaving any specific details in those scripts to @dan-knight or @aholmes

README.md Show resolved Hide resolved
action.yml Show resolved Hide resolved
action.yml Show resolved Hide resolved
@aholmes
Copy link
Member

aholmes commented Jul 9, 2024

thought (non-blocking):

The git branch foobar will result in the docker version branch-foobar. The one exception to this behavior is that the main branch will map to dev - that's in line with the current behavior of this Action.

Cool - though not directly relevant to this PR, dev also fits the Python version specifier format which can make this flow nicely with related PyPI packages.

note (non-blocking):

Note

The default branch name is hard-coded as main. docker/metadata-action has an {{is_default_branch}} expression, but unfortunately there is currently no way to negate it.

I didn't check to see if you're doing something to get around this, but here's a suggestion. In BL_Python there are several outputs from jobs that are used in later jobs. For example, here's how I create the cache key in the initial checkout that is then referenced by all subsequent jobs. You can similarly negate the value of is_default_branch in a script and output that value for later use.

@aholmes
Copy link
Member

aholmes commented Jul 9, 2024

question (non-blocking):

The one exception to this behavior is that the main branch will map to dev

Though we have typically avoided using it, thoughts on updating the latest tag (in addition?) ?

@nwiltsie
Copy link
Member Author

nwiltsie commented Jul 9, 2024

You can similarly negate the value of is_default_branch in a script and output that value for later use.

Unfortunately it's not a variable is_default_branch, it's a Handlebars template {{is_default_branch}} that is only evaluated within the docker tags action, so I don't think it can be cached anywhere. The logic isn't too complicated but it felt like too much to replicate for this PR.

Though we have typically avoided using it, thoughts on updating the latest tag (in addition?) ?

I don't think we shouldn't have both dev and latest pointing to the same version. Given that, and assuming we keep dev, I guess latest would point to the highest official release? I don't like latest in general for exactly this ambiguity, but I'm open to the discussion.

Copy link
Member

@aholmes aholmes left a comment

Choose a reason for hiding this comment

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

Two blocking comments. We can discuss in more detail; I know the Dependabot one is not fully fleshed out at the moment, and the one about variable expansion only applies if it actually matters in that context.

action.yml Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
action.yml Show resolved Hide resolved
action.yml Show resolved Hide resolved
delete-tags.js Show resolved Hide resolved
delete-tags.js Show resolved Hide resolved
delete-tags.js Show resolved Hide resolved
delete-tags.js Show resolved Hide resolved
@aholmes
Copy link
Member

aholmes commented Jul 9, 2024

Though we have typically avoided using it, thoughts on updating the latest tag (in addition?) ?

I don't think we shouldn't have both dev and latest pointing to the same version. Given that, and assuming we keep dev, I guess latest would point to the highest official release? I don't like latest in general for exactly this ambiguity, but I'm open to the discussion.

I share your concern, and see the utility in latest. However, given that we have typically avoided its use, and coming SOPs are stricter about dependencies, we should probably continue to not use latest.

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.

4 participants