Skip to content
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.

Sign up for GitHub

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

Feat: deploy for pre-release or release #329

Merged
merged 9 commits into from
Oct 2, 2023
Merged

Feat: deploy for pre-release or release #329

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
00d39d0
feat: add event trigger for when pre-releases or releases are published
angela-tran Sep 26, 2023
ade17c6
refactor: make pre-releases/releases use the correct deploy environment
angela-tran Sep 26, 2023
d613425
refactor: move concurrency config to workflow level instead of job level
angela-tran Sep 26, 2023
9218a01
feat: run tests before deploying if triggered by pre-release/release
angela-tran Sep 27, 2023
07a2e2e
refactor: remove unneeded conditions from test if expression
angela-tran Sep 29, 2023
1ec8099
refactor: extract out deploy-environment name as environment variable
angela-tran Sep 29, 2023
f36ca19
feat: create Docker tag with deployment environment name
angela-tran Sep 29, 2023
659c5c8
refactor: remove unneeded and redundant usage of always()
angela-tran Sep 29, 2023
0586d73
chore: add comment explaining 'deploy' job condition
angela-tran Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions .github/workflows/docker-publish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,32 @@ on:
- dev
- test
- prod
release:
types:
- published

defaults:
run:
shell: bash

env:
# this expression works like a ternary operation (see https://github.com/actions/runner/issues/409#issuecomment-727565588)
- DEPLOYMENT_ENVIRONMENT: ${{ github.event_name != 'release' && github.ref_name || github.event.release.prerelease && 'test' || 'prod' }}
thekaveman marked this conversation as resolved.
Show resolved Hide resolved

concurrency:
group: ${{ env.DEPLOYMENT_ENVIRONMENT }}
cancel-in-progress: true

jobs:
test:
uses: ./.github/workflows/run-tests.yml
if: github.event_name == 'release'

deploy:
needs: test
if: always() && !cancelled() && (github.event_name != 'release' || needs.test.result == 'success')
Copy link
Member Author

@angela-tran angela-tran Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following the need for always() -- isn't that like saying true AND x which evaluates to whatever x is?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right that it is redundant! I tried it out myself to verify.

Removed it in 659c5c8

runs-on: ubuntu-latest
environment: ${{ github.ref_name }}
concurrency: ${{ github.ref_name }}
environment: ${{ env.DEPLOYMENT_ENVIRONMENT }}

steps:
- name: Checkout
Expand Down Expand Up @@ -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 }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@angela-tran angela-tran Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

1 change: 1 addition & 0 deletions .github/workflows/run-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
name: Run pytests

on:
workflow_call:
pull_request:
branches: ["*"]

Expand Down