Feat: deploy for pre-release or release #329
Conversation
github.ref_name covers getting either branch _or tag_ name, so with this commit, a run of this workflow would publish a Docker tag with the git tag name
also set 'cancel-in-progress' to true so that pending runs will cancel if a newer run is queued for a given concurrency group
angela-tran
force-pushed
the
feat/deploy-for-pre-release-or-release
branch
from
d35f139 to
017a1eb
Compare
the workflow itself is already filtered to only listen to release publishes
angela-tran
force-pushed
the
feat/deploy-for-pre-release-or-release
branch
from
017a1eb to
f36ca19
Compare
| @@ -44,5 +60,6 @@ jobs: | |||
| file: Dockerfile | |||
| push: true | |||
| tags: | | |||
| ghcr.io/${{ github.repository }}:${{ env.DEPLOYMENT_ENVIRONMENT }} | |||
| ghcr.io/${{ github.repository }}:${{ github.ref_name }} | |||
| ghcr.io/${{ github.repository }}:${{ github.sha }} | |||
There was a problem hiding this comment.
Because we are in an interim state of still doing merge-based deployments, the first two values in this list will be the same... I'm not sure if the docker/build-push-action@v5 action will allow that.
There was a problem hiding this comment.
There's not a really easy way to test this without just merging this PR and trying a merge-based deployment.
If we really do want to test before merging though, it could be set up in a smaller experimental repo. It just takes some time to get it all set up.
.github/workflows/docker-publish.yml
Outdated
| deploy: | ||
| needs: test | ||
| if: always() && !cancelled() && (github.event_name != 'release' || needs.test.result == 'success') |
There was a problem hiding this comment.
always() is needed here because even if test didn't run, we still want this to run.
!cancelled() is needed because if the whole workflow was cancelled, we don't want this to run.
(github.event_name != 'release' || needs.test.result == 'success') is needed because if test did run, we only want this to run if test succeeded.
There was a problem hiding this comment.
I'm not following the need for always() -- isn't that like saying true AND x which evaluates to whatever x is?
There was a problem hiding this comment.
I think you are right that it is redundant! I tried it out myself to verify.
Removed it in 659c5c8
|
This is ready for review, but note that this is a stretch item for the sprint so it's ok if this doesn't get reviewed for a while |
There was a problem hiding this comment.
LGTM!
I just ran this in my terminal, which makes me think the duplicate tag here won't be a problem:
$ docker build --no-cache -t eligibility_server:main -t eligibility_server:main .
[+] Building 0.0s (0/0) docker:default
[+] Building 46.0s (11/11) FINISHED docker:default
=> [internal] load build definition from Dockerfile 6.0s
=> => transferring dockerfile: 404B 0.0s
=> [internal] load .dockerignore 6.4s
=> => transferring context: 81B 0.0s
=> [internal] load metadata for ghcr.io/cal-itp/docker-python-web:main 1.9s
=> CACHED [1/6] FROM ghcr.io/cal-itp/docker-python-web:main@sha256:7a8a6bc350c5ebffd04b3855912abeb3fb8a1b23c8578ad47c4a463a16a6070c 0.0s
=> [internal] load build context 0.8s
=> => transferring context: 674B 0.0s
=> [2/6] RUN python -m pip install --upgrade pip 7.0s
=> [3/6] COPY bin bin 0.4s
=> [4/6] COPY eligibility_server/ eligibility_server/ 0.3s
=> [5/6] COPY pyproject.toml pyproject.toml 0.2s
=> [6/6] RUN pip install -e . 28.1s
=> exporting to image 0.8s
=> => exporting layers 0.7s
=> => writing image sha256:97398d05669bda9f05d9a655dbd1f1e0b6c95aa5c22ba61e5e45a0d11df5c09e 0.0s
=> => naming to docker.io/library/eligibility_server:main
Ooh, good idea to try that 👍 |
Part of #286
This PR adds to our
DeployGitHub workflow so that it can be triggered by a GitHub release (either pre- or full release will work).These are all the code changes needed to implement #286; the last two tasks on the issue are changes to settings for the GitHub repo.