-
-
Notifications
You must be signed in to change notification settings - Fork 182
Add deployment options #5958
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
base: main
Are you sure you want to change the base?
Add deployment options #5958
Conversation
This fixes #5868 by adding a number of conditional steps to the deployment. We can now skip various pieces if they do not apply to make the deployment faster, or to skip it altogether.
for more information, see https://pre-commit.ci
Added all three of you as reviewers here in case y'all have thoughts, but I don't imagine the code will need more than one review if somebody wants to claim that part of it. Thanks all! I hope this will help! |
After using it, I noticed a few things that could be tweaked
…s-2025-07-13' into issue-5868-streamline-deployments-2025-07-13
for more information, see https://pre-commit.ci
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.
Thanks for tagging! I left a suggestion for the template, and job changes look good to me, but I haven't used github actions that much so I would take my input here with a grain of salt. I only wonder if there's anything that could potentially error out some other pipeline not triggered by a PR, like direct pushes to main?
.github/workflows/docker-build.yml
Outdated
|
||
const labels = pr.labels.map(l => l.name); | ||
core.setOutput("labels", labels.join(",")); | ||
core.setOutput("pr_number", pr.number); |
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.
are we using this output for anything?
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.
Good call. Removed.
Co-authored-by: Elisa <[email protected]>
${{ | ||
!contains(needs.get-pr-labels.outputs.labels, 'skip-deploy') && | ||
!contains(needs.get-pr-labels.outputs.labels, 'skip-web-deploy') && | ||
!contains(needs.get-pr-labels.outputs.labels, 'skip-celery-deploy') && | ||
!contains(needs.get-pr-labels.outputs.labels, 'skip-cronjob-deploy') | ||
}} |
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.
Would this skip the image build and push if any of the skip labels is applied?
Is that correct? For instance, if the author adds the skip-web-deploy
label, we’d still need the image to be built and pushed so that the Celery and cronjob deployments receive the changes, right?
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.
So if any of these labels are applied, it won't run this step, and I'm pretty sure that means the deploy won't run since it needs
the build
step. I'm not positive, but I think that's how it will work.
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.
Yeah that block of code means that if any skip-
label is present, the build process is skipped, which is my concern.
For example, if the user only passes skip-celery-deploy
, it should still deploy to the web service and the cron job, right? But because there’s no build, the deploy action is completely omitted?
Shouldn’t we only skip the build
step when the entire deploy is skipped with skip-deploy?
Fixes
Fixes: #5868
Summary
This PR adds deployment options to pull requests so that if the PRs have particular labels, certain parts of the deployment will be skipped. This should make deployments faster and more streamlined.
I developed these tweaks in a personal repo. You can see an example run here:
https://github.com/mlissner/actions-test/actions/runs/16248353854
One thing that could be nicer, in theory, is that I'm checking using
contains()
instead of equality. Doing the latter seems to be really hard and not worth it, so I went with this compromise. It should be fine, so long a we don't make a label calledskip
or something like that.This PR also adds a PR template to encourage the use of these labels. You're witnessing the first use of the template now.
Deployment
The following labels control the deploymnet of this PR if they are applied. Please choose which should be applied, then apply them to this PR:
skip-deploy — The entire deployment can be skipped.
This might be the case for a small fix, a tweak to documentation or something like that.
skip-web-deploy — The web tier can be skipped.
This is the case if you're working on code that doesn't affect the front end, like management commands, tasks, or documentation.
skip-celery-deploy — Deployment to celery can be skipped.
This is the case if you make no changes to tasks.py or the code that tasks rely on.
skip-cronjob-deploy — Deployment to cron jobs can be skipped.
This is the case if no changes are made that affect cronjobs.
What extra steps are needed to deploy this beyond the standard deploy?
None.
Screenshots
None