Build, test, and deploy a Heroku slug instead of re-building on Heroku #857
Conversation
tsibley
force-pushed
the
trs/give-me-a-build-ci-one-build-only-please
branch
from
a6a8a3a
to
2ab1460
Compare
| --env SOURCE_VERSION \ | ||
| --env SOURCE_DESCRIPTION \ | ||
| --env NODE_MODULES_CACHE=false \ | ||
| heroku/heroku:20-build bash -c ' |
There was a problem hiding this comment.
note
heroku/heroku:20-build (and 22-build) are single-platform linux/amd64 - no linux/arm64 variant. This means scripts/heroku-build runs slower on Apple silicon, but not terribly slow - 7m17s on my M1 just now.
heroku/heroku:24-build is multi-platform with a linux/arm64 variant. Just noting a reason to upgrade in the future.
There was a problem hiding this comment.
Good to note! Upgrading should be pretty painless, but I wanted to save that for another PR.
…Heroku
Building the Heroku app slug locally avoids the need to re-build on
Heroku. Heroku build dynos are much slower than GitHub Action runners
and building twice is a waste of time regardless. This also means that
we deploy the exact thing that we test instead of a new build, making
this now a true CI flow.
The CI jobs are now:
┌─────────────┐
│ lint (~40s) │
└─────────────┘
┌─────────────┐ ┌───────────────┐ ┌───────────────┐
│ build (~4m) │────▶│ test (~1m15s) │────▶│ deploy (~30s) │
└─────────────┘ └───────────────┘ └───────────────┘
│ ▲
└─────────────────────────────────────────────┘
Lint is moved out of test, because test now happens after build and it
doesn't make sense to lint _after_ build (since, as a previous comment
noted, the build will fail if linting fails).
Review apps are still built twice—once on GitHub Actions and once on
Heroku—and deployed from the Heroku build, but we can address that in
future work if desired.
For troubleshooting and debugging, the Heroku app slug can be built
locally as well with `./scripts/heroku-build`. Docker is required.
Resolves: <#850>
… the Heroku runtime
This is handy for testing the "deploy" job and potentially for deploying a larger work-in-progress change to dev.nextstrain.org (i.e. before its ready for next.nextstrain.org). The "deploy" job now references a GitHub environment¹ named after the Heroku app to access Heroku API credentials and record the deployment. I've provisioned the "nextstrain-canary"² and "nextstrain-dev"³ environments in the repo settings. Each has the same Heroku API token attached to bot@nextstrain.org, scoped down to "write-protected" (instead of the default of "global").⁴ The "nextstrain-canary" environment can only be used when running on master and so is only usable by those who can push to master. The "nextstrain-dev" environment, however, can be used from any branch, and thus as a safeguard requires manual approval of the deployment by @nextstrain/core. Unfortunately the Heroku API tokens can't be scoped down to a single app instead; they always allow access to all apps the bot@ account can manage. ¹ <https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment> ² <https://github.com/nextstrain/nextstrain.org/settings/environments/440351004/edit> ³ <https://github.com/nextstrain/nextstrain.org/settings/environments/13941227/edit> ⁴ <https://devcenter.heroku.com/articles/oauth#scopes>
No need to be a secret, and makes it clearer what token we're working with. I've adjusted the "nextstrain-canary" and "nextstrain-dev" environments to account for this.
Yeah! For CI runs that deploy, it'll mean faster times. For CI runs that don't deploy, it may be a bit slower. Need to look into that more closely. OTOH, if we're going to start deploying review apps ourselves (and I think we probably should?) then all CI runs will do deploys. |
@victorlin noted in review that the previous construct threw an "unbound variable" error on macOS's system Bash 3.2.57.¹ This is because after 3.2.57 and at least by 4.4.20 (but maybe earlier), Bash stopped treating the "@" and "*" parameters as unset under -u when empty.² Use an array slice³ (i.e. substring expansion, ${parameter:offset}, applied to an array) as a workaround. Note that testing if "docker_args" is empty, e.g. "${docker_args:+${docker_args[@]}}", won't work because it leaves us with an empty string argument when unset. ¹ <#857 (comment)> ² <nextstrain/ingest#17 (comment)> ³ <https://www.gnu.org/software/bash/manual/bash.html#Shell-Parameter-Expansion>
tsibley
force-pushed
the
trs/give-me-a-build-ci-one-build-only-please
branch
from
2ab1460
to
0f76b4b
Compare
One thing we could do here (even if we're going to move review apps into CI and make all CI runs do deploys) is run tests against both the "deployment-style" build as is done in this PR currently and a "development-style" build as we used to. They could run concurrently and wouldn't change the overall duration of the CI run while still giving us faster deploys. Put another way, we'd make the current |
…instead of a potentially not-yet-released version. "latest" is a tag that Heroku updates to keep pointed at the latest numbered release tag, e.g. it points to "v247" currently.
There was a problem hiding this comment.
run tests against both the "deployment-style" build as is done in this PR currently and a "development-style" build as we used to
Is the only benefit to have a slightly faster build result? If so, I don't think it's worth the added complexity.
We could always merge as-is and reconsider later if build results seem painfully slower.
Slightly faster time-to-test-failure for failing CI runs. Yeah, wasn't suggesting I do it now in this PR but as an option for the future. |
There was a problem hiding this comment.
Hooray for faster deploys!
Will need to read more on buildspacks...I'm assuming this will inform the implementation details for creating Heroku review apps.
|
First deploy to canary with this PR went smoothly. Note the lack of a Heroku build before the Heroku deploy:
GitHub CI run and GitHub deployment record.
I've done the post-merge tidy ups I'd previously identified. |
…ort options The long options are from GNU coreutils, but macOS ships a BSD tty and du. Only one of the swapped options is POSIX. Noted by @joverlee521 in post-merge review.¹ ¹ <#857 (review)>
| environment: | ||
| name: heroku | ||
| url: https://next.nextstrain.org | ||
| name: ${{ inputs.heroku-app || 'nextstrain-canary' }} | ||
| url: ${{ inputs.heroku-app && format('https://{0}.herokuapp.com', inputs.heroku-app) || 'https://next.nextstrain.org' }} |
There was a problem hiding this comment.
On the deployments page, nextstrain-canary maps to next.nextstrain.org and nextstrain-dev to nextstrain-dev.herokuapp.com (as opposed to dev.nextstrain.org). I thought that was weird, but it makes sense after looking back at this.
There was a problem hiding this comment.
Yeah, we could improve that various ways later.
See commit messages.
Resolves: #850
Checklist