-
Notifications
You must be signed in to change notification settings - Fork 8
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
#3 Added aiSSEMBLE GitHub Action release job #332
Conversation
3138cff
to
362fe93
Compare
|
||
runs-on: arc-runner-set-aissemble | ||
env: | ||
DOCKER_CONFIG: /home/runner/.docker |
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.
Using env variables to easily switch out using workflow_dispatch input values and hardcoded values for testing!
.github/workflows/release.yml
Outdated
description: "OPTIONAL: The existing aiSSEMBLE version. Defaults to patchVersion-SNAPSHOT" | ||
required: false | ||
type: string | ||
dryRun: |
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.
Allows us to toggle running the release job in dryRun mode.
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.
Looks good to me, but I haven't done a release on aiSSEMBLE so be sure you get a second opinion from baseline.
Also, be sure to sign all commits.
362fe93
to
66651af
Compare
with: | ||
username: ${{ secrets.DOCKERHUB_USERNAME }} | ||
password: ${{ secrets.DOCKERHUB_TOKEN }} | ||
- uses: actions/checkout@v4 |
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.
Q: - uses: actions/checkout@v4
this is used twice, is that needed? Also on line 106.
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.
Yes it is needed, it is a bug with using custom actions. You have to checkout the current branch your running on for them to show up. The other checkout
checks out the release branch.
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.
Recommend grouping them together (if possible) and leaving a comment explaining that.
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 cant group them unfortunately but I can leave a comment
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 don't think you'll need a double checkout once this is merged to dev. Do you have a link to the info you found saying you would need that?
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.
Ok this is actually not a bug. It looks like if you dont do a initial checkout, the local aissemble repository is actually not present in the runner. By doing an initial checkout, the repository is present and the branch being ran on is checked out.
66651af
to
17f4ce4
Compare
description: 'Existing version' | ||
required: true | ||
outputs: | ||
minor-version: |
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.
S: Would update this variable to be major-minor-version
, sort of misleading currently.
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.
Changed
17f4ce4
to
fd43a8e
Compare
description: "Is pre-release version" | ||
value: ${{ steps.split.outputs.release-branch }} | ||
existing-version: | ||
description: "Is pre-release version" |
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.
S: Update descriptions
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.
Updated
echo "minor-version=$minor_version" >> $GITHUB_OUTPUT | ||
echo "patch-version=$patch_version" >> $GITHUB_OUTPUT | ||
echo "is-pre-release=$( [ "${{ inputs.version }}" != "$patch_version" ] && echo true || echo false)" >> $GITHUB_OUTPUT | ||
echo "release-branch=$( [ "${{ inputs.releaseBranch }}" != "" ] && echo "${{ inputs.releaseBranch }}" || echo "aissemble-release-$minor_version")" >> $GITHUB_OUTPUT |
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.
Q: Have you verified this works having the echo
and part of the &&
conditional?
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.
Yes this works!
fd43a8e
to
e5917fd
Compare
.github/workflows/release.yml
Outdated
custom-facet-new-version: ${{ env.RELEASE_VERSION }} | ||
- name: Create settings.xml | ||
run: | | ||
echo "<settings><servers><server><id>ossrh</id><username>${{ secrets.SONATYPE_CENTRAL_REPO_TOKEN_USER }}</username><password>${{ secrets.SONATYPE_CENTRAL_REPO_TOKEN_KEY }}</password></server><server><id>ghcr.io</id><username>${{ secrets.GHCR_IO_USERNAME }}</username><password>${{ secrets.GHCR_IO_TOKEN }}</password></server><server><id>pypi</id><username>${{ secrets.PYPI_USERNAME }}</username><password>${{ secrets.PYPI_TOKEN }}</password></server><server><id>dev-pypi</id><username>${{ secrets.TEST_PYPI_USERNAME }}</username><password>${{ secrets.TEST_PYPI_TOKEN }}</password></server> </servers></settings>" > $HOME/.m2/settings.xml |
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.
Q: Should we be using a differ pypi server for the release? I believe test pypi is only for our snapshots.
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.
Its being uploaded to Pypi not test pypi, I verified this. We add in credentials to Pypi and test Pypi
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.
Safe to remove this then? <id>dev-pypi</id><username>${{ secrets.TEST_PYPI_USERNAME }}</username><password>${{ secrets.TEST_PYPI_TOKEN }}</password>
?
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 am not sure if it is safe to remove!
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.
yes it should be
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.
Removed
minor=$(echo ${{ inputs.version }} | cut -d "." -f 2) | ||
patch=$(echo ${{ inputs.version }} | cut -d "." -f 3 | cut -d "-" -f 1) | ||
minor_version=$major.$minor | ||
patch_version=$major.$minor.$patch |
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.
A: Would be helpful to have comments explaining what these vars are given inputs, for example from the old release script:
//grab second group (first embedded capture) in first match: 1.4.2-rc.3 -> 1.4.2
//grab second group (first embedded capture) in first match: 1.4.2 -> 1.4
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.
Added comments to better explain
.github/workflows/release.yml
Outdated
run: | | ||
echo "Releasing with the following parameters" | ||
echo "Release version: ${{ env.RELEASE_VERSION }}" | ||
echo "Development version: ${{ env.DEVELOPMENT_VERSION }}" |
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.
Optional: Change Development version
-> Next Development Version
and env.DEVELOPMENT_VERSION
-> env.NEXT_DEVELOPMENT_VERSION
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.
Changed
98aae12
to
0b93383
Compare
.github/workflows/release.yml
Outdated
default: false | ||
type: boolean | ||
push: | ||
branches: [ "3-add-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.
This will be removed before merge? Does it need a different value?
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.
C: This needs to be removed before merging. This workflow should be on dispatch only.
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.
Yes, this is only for testing purposes. It will be removed before being merged.
7866381
to
d7eb15d
Compare
description: "Is pre-release version" | ||
value: ${{ steps.split.outputs.is-pre-release }} | ||
release-branch: | ||
description: "Is pre-release version" |
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.
A: Update description
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.
Removed descriptions
required: true | ||
outputs: | ||
major-minor-version: | ||
description: "Major-minor version" |
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.
S: In general these descriptions are just restating the name of the output which doesn't seem helpful. We should either make them more explanatory or just omit them if we can.
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.
Removed descriptions
release-branch: | ||
description: 'Release branch' | ||
required: true | ||
custom-facet-old-version: |
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.
Q: Why are these inputs instead of just being derived from version
?
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.
When updating the custom facet schemas, what we change differs between the pre release Antora docs update and the post release.
git config --global user.name "aiSSEMBLE Team" | ||
git diff | ||
git add . | ||
git commit -S -m "Updating scripts and templates to ${{ inputs.version }}" |
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.
A: I'd make the commit message more accurate. This is likely a carryover from when we updated more stuff manually instead of through the build process.
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.
Changed to "Updating Antora documentation to version ..."
.github/workflows/release.yml
Outdated
NEXT_DEVELOPMENT_VERSION: ${{ inputs.developmentVersion || '1.10.0-SNAPSHOT' }} | ||
RELEASE_BRANCH: ${{ inputs.releaseBranch }} | ||
EXISTING_VERSION: ${{ inputs.existingVersion }} | ||
DRY_RUN: ${{ inputs.dryRun || true }} |
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.
C: We should not be including these defaults at all. It just introduces risk/confusion. If this script is updated/worked on in the future and such mechanisms are helpful, the author can add this stuff in. However, I would argue that we should be testing by filling in the parameters manually anyway.
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.
This is only for testing purposes. This will be deleted before being merged.
.github/workflows/release.yml
Outdated
curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | ||
chmod 700 get_helm.sh | ||
./get_helm.sh | ||
- name: Install Helm Unittest Plugin |
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.
S: This is just a copy of what we're doing in build.yaml
right? Strongly suggest we pull these out into a common place if so. We're already introducing the complexity of composite actions, and this is the core use case for them.
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.
Added instal dependencies composite action.
.github/workflows/release.yml
Outdated
echo "<settings><servers><server><id>ossrh</id><username>${{ secrets.SONATYPE_CENTRAL_REPO_TOKEN_USER }}</username><password>${{ secrets.SONATYPE_CENTRAL_REPO_TOKEN_KEY }}</password></server><server><id>ghcr.io</id><username>${{ secrets.GHCR_IO_USERNAME }}</username><password>${{ secrets.GHCR_IO_TOKEN }}</password></server><server><id>pypi</id><username>${{ secrets.PYPI_USERNAME }}</username><password>${{ secrets.PYPI_TOKEN }}</password></server><server><id>dev-pypi</id><username>${{ secrets.TEST_PYPI_USERNAME }}</username><password>${{ secrets.TEST_PYPI_TOKEN }}</password></server> </servers></settings>" > $HOME/.m2/settings.xml | ||
- name: Release aiSSEMBLE | ||
run: | | ||
./mvnw release:clean release:prepare release:perform -s $HOME/.m2/settings.xml -U -B -Pci,gh-build -DdryRun=${{ env.DRY_RUN }} -DpushChanges=true -DreleaseVersion=${{ env.RELEASE_VERSION }} -DdevelopmentVersion=${{ env.NEXT_DEVELOPMENT_VERSION }} [email protected] -DsignTag=true -Dmaven.build.cache.enabled=false |
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.
S: IMO the dry-run isn't going to be used enough to warrant having it checked in to dev and it just adds noise to the parameters. Esp since we aren't using it for things like the git commits at earlier steps.
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.
Removed
d7eb15d
to
48cd4f7
Compare
- Update documentation to live under supporting components - #219 improve manual action message keys
36b7053
to
180d90d
Compare
- Update documentation to live under supporting components - #219 improve manual action message keys
- Update documentation to live under supporting components - #219 improve manual action message keys
- Update documentation to live under supporting components - #219 improve manual action message keys
- Update documentation to live under supporting components - #219 improve manual action message keys
- Update documentation to live under supporting components - #219 improve manual action message keys
- Update documentation to live under supporting components - #219 improve manual action message keys
- Update documentation to live under supporting components - #219 improve manual action message keys
This is a PR to add the aiSSEMBLE GitHub Action release job. This action mimics the Jenkins release job found here https://github.boozallencsn.com/aissemble/aissemble/blob/fix-ci/devops/JenkinsfileRelease.groovy
The release job aims to do the following