AN-354 Initial checkin of Release Community Cromwell GHA #7691
Conversation
jgainerdewar
changed the title
| - name: Find Cromwell release number | ||
| run: | | ||
| set -e | ||
| previous_version=$(curl -X GET https://api.github.com/repos/broadinstitute/cromwell/releases/latest | jq .tag_name | xargs) |
There was a problem hiding this comment.
There might be a more elegant way to do this now, but this way works.
| name: Release Community Cromwell | ||
|
|
||
| on: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Nit, but do you want to set up a manual trigger to be able to run this workflow on any branch in case you need to further debug once this has been merged? Something like this should do it: https://github.com/DataBiosphere/terra-docker/blob/a0cf5769e6b469ba5813c810c119b10f26915422/.github/workflows/publish.yml#L2
There was a problem hiding this comment.
I do, but it looks like I get that for free - this is what I see when I run the action in my fork:
Maybe I'm misunderstanding though, I haven't actually tried with a different branch.
There was a problem hiding this comment.
I guess you only care about the workflow file, and if that's the case you should be all set. In the example I shared, the branch input is to chose the code from your PR branch, in addition to selecting the workflow from that branch too
There was a problem hiding this comment.
Ah, got it - for this specific workflow I think we do always want to use develop and master, and use a fork if we want to test without creating real releases. Feels like a footgun to give users a dropdown to create a real release from an arbitrary branch.
| steps: | ||
| - uses: sbt/setup-sbt@v1 | ||
|
|
||
| # `DSDEJENKINS_PASSWORD` auto syncs from vault with https://github.com/broadinstitute/terraform-ap-deployments/pull/614 |
There was a problem hiding this comment.
Do we know if this secret will stay put in vault, or if devops has plans to move it to GSM instead?
There was a problem hiding this comment.
No idea, it may already be coming from GSM - I can look and update the comment. This was copied and pasted from a different GHA we have.
There was a problem hiding this comment.
Looks like it's still in Vault.
There was a problem hiding this comment.
Copy-pasted could be a candidate for making a reusable ("composite") action.
There was a problem hiding this comment.
True, though I don't know that that would result in many fewer lines copied and pasted, this is pretty minimal.
| if ! [[ "${previous_version}" =~ ^[0-9][0-9]+$ ]]; then | ||
| exit 1 | ||
| fi | ||
| echo "RELEASE_VERSION=$((previous_version + 1))" >> $GITHUB_ENV |
There was a problem hiding this comment.
Are cromwell versions always incremented by 1? Would it make sense to have a manual override using workflow inputs in case this changes?
There was a problem hiding this comment.
In the past they have always been incremented by 1, and there are a few fiddly things in here that would go wrong if we started using arbitrary versions. We'll need to address this as part of switching to semantic versioning, which we should do, but that's a conversation for a different PR - are you okay waiting until then to do custom version support?
There was a problem hiding this comment.
This logic does replicate the old release script.
| token: ${{ secrets.BROADBOT_GITHUB_TOKEN }} | ||
| path: cromwell | ||
| ref: develop | ||
| fetch-depth: 0 # <-- important for branch shenanigans below |
There was a problem hiding this comment.
Could you provide a bit more details about this? I could not pin point exactly where this is used below
There was a problem hiding this comment.
Sure, will expand comment - basically this tells the action to check out all the past commits, which is important for git to understand how to merge branches together.
There was a problem hiding this comment.
I think in this case 0 is a sentinel for infinity.
Description
Initial checkin of this GHA. I've done as much testing as I could in my fork, you can see the release created there and my recent runs of this action.
This requires a further round of testing in this repo with real secrets, but need to do an initial merge first.
This action will result in:
developmerged tomasterand upgraded to the next versionbroadinstitute/cromwell:88Release Notes Confirmation
CHANGELOG.mdCHANGELOG.mdin this PRCHANGELOG.mdbecause it doesn't impact community usersTerra Release Notes