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.

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

Add input for which version to deploy on Deploy App workflow #137

Closed
wants to merge 6 commits into from

Conversation

doshitan
Copy link
Contributor

@doshitan doshitan commented Sep 18, 2024

Ticket

Resolves navapbc/template-infra#693

Changes

Pass the value along to the other workflows as necessary.

Testing

Deploying with defaults (dev env and main version): https://github.com/navapbc/platform-test/actions/runs/10926380147

Deploying this branch (which is functionally same as main at time of testing) to dev to see the different version get carried through and succeed: https://github.com/navapbc/platform-test/actions/runs/10926808599 outdated

Deploying this branch (which is functionally same as main at time of testing) to dev to see the different version get carried through and succeed
https://github.com/navapbc/platform-test/actions/runs/11000627559
outdated

Deploying this branch (which is functionally same as main at time of testing) to dev to see the different version get carried through and succeed: https://github.com/navapbc/platform-test/actions/runs/11040058211

Preview environment

♻️ Environment destroyed ♻️

Pass the value along to the other workflows as necessary.
@doshitan
Copy link
Contributor Author

Markdown lint failure unrelated to these changes, created navapbc/template-infra#750

Copy link
Collaborator

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

Looks good, just one important change to pass in IMAGE_TAG rather than checking out a different ref

.github/workflows/cd-app.yml Outdated Show resolved Hide resolved
.github/workflows/cd-app.yml Outdated Show resolved Hide resolved
@@ -11,6 +11,10 @@ on:
description: "the name of the application environment (e.g. dev, staging, prod)"
required: true
type: string
version:
description: "git reference to deploy (e.g., a branch, tag, or commit SHA)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: "git reference to deploy (e.g., a branch, tag, or commit SHA)"
description: "git reference to deploy (e.g. branch, tag, or commit SHA)"

.github/workflows/database-migrations.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to create the template-infra PR now.
If you are inclined to you can test a situation where the branch we're running the workflow from is different from the version we're deploying. E.g. you can make a change to the test app in this branch (e.g. change the hello world string), then undo the change, then run a deploy from main branch but pass in the commit SHA of the commit that had the change and deploy to dev.

But I don't feel strongly about that level of test, this feels like a reasonably low risk change to me.

@doshitan
Copy link
Contributor Author

Looks good, just one important change to pass in IMAGE_TAG rather than checking out a different ref

Going over it again, I don't think we want to just pass in version as the IMAGE_TAG since I think we'd want the same branch, deployed multiple times, to always deploy that branches top commit right? So we need to resolve the version to the actual commit and have all the things that build and deploy the image use that for IMAGE_TAG.

Copy link
Collaborator

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

Good point on needing to get the commit hash not just the version. Maybe test it using this branch to make sure it works?

@@ -63,7 +67,7 @@ jobs:
- name: Check if image is already published
id: check-image-published
run: |
is_image_published=$(./bin/is-image-published "${{ inputs.app_name }}" "${{ inputs.ref }}")
is_image_published=$(./bin/is-image-published "${{ inputs.app_name }}" "${{ needs.get-commit-hash.outputs.commit_hash }}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for finding and fixing this bug! I could have sworn I fixed this issue but I was still seeing it happen but didn't have time to look into it

@doshitan
Copy link
Contributor Author

Merged in navapbc/template-infra@4275fd0

@doshitan doshitan closed this Sep 27, 2024
@doshitan doshitan deleted the doshitan/693-deploy-version branch September 27, 2024 21:03
@doshitan doshitan restored the doshitan/693-deploy-version branch September 27, 2024 21:11
@doshitan doshitan reopened this Sep 27, 2024
@doshitan doshitan marked this pull request as ready for review September 27, 2024 21:12
@doshitan doshitan closed this Sep 27, 2024
@doshitan doshitan deleted the doshitan/693-deploy-version branch September 27, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow "deploy app" workflow to deploy specific version
2 participants