ci(app): deploy app artifacts to build number prefixed folder #13970
Conversation
shlokamin
added
the
DO NOT MERGE
| echo "folder=${{env._APP_DEPLOY_BUCKET_OT3}}" >> $GITHUB_OUTPUT | ||
| echo "folder=${{env._APP_DEPLOY_FOLDER_OT3}}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
i think this was just straight up wrong before and we never noticed because we were using top level env vars to deploy below.
| @@ -290,7 +290,7 @@ jobs: | |||
| HOST_PYTHON: python | |||
| OPENTRONS_PROJECT: ${{ steps.project.outputs.project }} | |||
| OT_APP_DEPLOY_BUCKET: ${{ steps.project.outputs.bucket }} | |||
| OT_APP_DEPLOY_FOLDER: ${{ steps.project.outputs.folder }} | |||
| OT_APP_DEPLOY_FOLDER: ${{ steps.project.outputs.folder }}/${{OT_BUILD}} | |||
There was a problem hiding this comment.
I don't think this is right because there's two places that need the OT_BUILD to not be included:
- Below (see comment) we upload
releases.jsontoOT_APP_DEPLOY_FOLDER, andreleases.jsonneeds to be in/appnot/app/$OT_BUILD - this environment variable configures the electron-updater's target URL, and we want electron-updater to continue to look at
/app/
There was a problem hiding this comment.
oopsy didnt realize that env var is used by the app shell and app shell odd, thanks!
| @@ -343,11 +343,13 @@ jobs: | |||
| aws configure set source_profile identity --profile deploy | |||
| shell: bash | |||
| - name: 'deploy release builds to s3' | |||
| if: contains(fromJSON(needs.determine-build-type.outputs.variants), 'release') | |||
There was a problem hiding this comment.
this definitely is wrong - internal-release contains release so it will fire on both. I think this won't do anything though because the code above creates one subdir for release artifacts and one subdir for internal-release artifacts and sorts them, so to_upload_release/ would be empty if this was an internal release (this is also why these steps were not if-gated before)
There was a problem hiding this comment.
lol yes that if statement is useless, good catch. i still think its worth correcting the if statement and including it though, even though one of the subdirs would be empty i think it makes things easier to follow
| @@ -453,13 +455,13 @@ jobs: | |||
| - name: 'update internal-releases releases.json' | |||
| if: needs.determine-build-type.outputs.type == 'release' && contains(fromJSON(needs.determine-build-type.outputs.variants), 'internal-release') | |||
| run: | | |||
| aws --profile=deploy s3 cp s3:https://${{ env._APP_DEPLOY_BUCKET_OT3 }}/${{ env._APP_DEPLOY_FOLDER_OT3 }}/releases.json ./to_upload_internal-release/releases.json | |||
| aws --profile=deploy s3 cp s3:https://${{ env._APP_DEPLOY_BUCKET_OT3 }}/${{ env.OT_APP_DEPLOY_FOLDER }}/releases.json ./to_upload_internal-release/releases.json | |||
There was a problem hiding this comment.
releases.json still wants to stay in builds.opentrons.com/app/, not /app/$BUILDNUMBER - this is one of the reasons I don't think we can just append the build id to the environment variable above
There was a problem hiding this comment.
beep boop beep yes releases.json needs to be at the top level. ill remove the build number from the env variable and only use it for app artifact deploys
Overview
DO NOT MERGE UNTIL AFTER 7.1 IS RELEASED
This PR deploys app builds to a folder prefixed by build number. This is so we don't overwrite historical build artifacts so we can route electron updater to previous non revoked builds if the latest build has been revoked.
Related to https://github.com/Opentrons/robot-stack-infra/pull/34
Changelog
Review requests
This one is hard to test without pushing actual internal release/release tags, we'll have to battle test this during the next alpha process after 7.1 gets released to production
Risk assessment
High, this touches critical release artifact infrastructure