-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add workflow to update major alias tags, templates #13
Conversation
@@ -1,5 +1,5 @@ | |||
--- | |||
name: Prepare new release | |||
name: 📦 Prepare new release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I thought this was bread :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChatGPT told me to do it!
For a workflow that is responsible for creating software releases, the most appropriate emojis to highlight the purpose of the workflow would be:
🚀
:rocket:
- Represents launching or releasing something, making it a popular and intuitive choice for release workflows.
🏷️
:label:
- Represents a tag or label, which aligns well with tagging versions or releases in a repository.
📦
:package:
- Symbolizes packaging or bundling software, often associated with creating and distributing software releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just sad it wasn't bread ):
bumpchanges/alias.py
Outdated
from .utils import dereference_tags, tag_to_semver, get_github_releases, Release | ||
|
||
|
||
class IneligibleAlias(Exception): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: rename to IneligibleAliasError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent suggestion - done with d42c04c.
bumpchanges/alias.py
Outdated
"""Confirm that the collected data is in a reasonable state.""" | ||
# All releases must have corresponding git tags | ||
if ( | ||
unknown_tags := self.tag_to_release_map.keys() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: though not required, usages of :=
can be clarified by grouping, e.g.,
if (
unknown_tags := (
self.tag_to_release_map.keys()
- self.tag_to_commit_map.keys()
)
): ...
This makes the intention clear that unknown_tags is the result of unknown_tags = (a - b)
and not (unknown_tags = a) - b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah what I have here is misleading. Fixed up with fa2cceb.
bumpchanges/alias.py
Outdated
"""Confirm that the collected data is in a reasonable state.""" | ||
# All releases must have corresponding git tags | ||
if ( | ||
unknown_tags := self.tag_to_release_map.keys() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: The various usages of some_var.keys()
might be more readable if the values are realized earlier, and once, e.g.:
release_keys = tag_to_release_map.keys()
commit_keys = tag_to_commit_map.keys()
version_keys = tag_to_version_map.keys()
if (release_keys ... commit_keys): ...
if (version_keys ... release_keys): ...
# etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, this is way more readable: 64f3d3e.
repository: uclahs-cds/tool-create-release | ||
path: reusable | ||
ref: ${{ steps.workflow-parsing.outputs.SHA }} | ||
token: ${{ secrets.UCLAHS_CDS_REPO_READ_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (non-blocking): I guess once this repo is public, this token should also no longer be necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the secret won't be necessary for this purpose.
Separately, I might need to add an input for the token to be used when creating the release (in the release workflow). I ran into that constant issue where the alias workflow would not be triggered unless the release workflow was run with a non-standard token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically this block:
tool-create-release/.github/workflows/wf-finalize-release.yaml
Lines 36 to 45 in 668db63
- id: parse-version | |
uses: actions/github-script@v7 | |
env: | |
INPUT_DRAFT: ${{ inputs.draft }} | |
with: | |
# Use the separate token so that `published` events will be fired | |
github-token: ${{ secrets.UCLAHS_CDS_REPO_READ_TOKEN }} | |
script: | | |
const script = require('./scripts/finalize-release.js') | |
await script({github, context, core}) |
Description
This one is a little more sprawling than I intended, so apologies for the scope creep. Everything is kind of interdependent.
The major changes in this PR are:
published
ordeleted
. If the modified release has a semantic version tag (e.g.v1.2.3
), then the workflow will update the matching alias tagv1
to point at the highest full release with that major version. This follow's GitHub's guidance for Action and workflow release management.prerelease
is now a separate checkbox rather than a bump type choice likemajor
/minor
/patch
. Previously choosingprerelease
would only bump the patch number, so there was no way to create the version2.0.0-rc.1
.templates/
directory (closes Documentation suggestion: Adding YAML files #9)! Those can be copied verbatim into another repository and they'll work.v
-prefix nonsense from spreading.prerelease
flag) and the aliasing.I have one more feature to implement in a different PR (updating hardcoded version numbers in
__version__.py
andnextflow.config
files), and then I believe these workflows will be ready for widespread use. Given that, please do dig into the nitty-gritty details.I've been testing this with the https://github.com/uclahs-cds/docker-internal-tests repository - it is using duplicates of the new template workflows (modified to point to this branch). Please feel free to test the workflows by creating releases, deleting releases, etc. All of this is intended to be robust and handle edge cases gracefully.
Closes #9
Checklist
This PR does NOT contain Protected Health Information (PHI). A repo may need to be deleted if such data is uploaded.
Disclosing PHI is a major problem1 - Even a small leak can be costly2.
This PR does NOT contain germline genetic data3, RNA-Seq, DNA methylation, microbiome or other molecular data4.
.png
, .jpeg
),.pdf
,.RData
,.xlsx
,.doc
,.ppt
, or other output 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.Footnotes
UCLA Health reaches $7.5m settlement over 2015 breach of 4.5m patient records ↩
The average healthcare data breach costs $2.2 million, despite the majority of breaches releasing fewer than 500 records. ↩
Genetic information is considered PHI.
Forensic assays can identify patients with as few as 21 SNPs ↩
RNA-Seq, DNA methylation, microbiome, or other molecular data can be used to predict genotypes (PHI) and reveal a patient's identity. ↩