ci: Test pathogen repo CI builds with the final image #148
Conversation
|
Ah, it's as I feared and jobs which call reusable workflows can't use the I'll figure out how to rework this to have ~equivalent behaviour without that key, but first I'm going to repush without it to see if the rest of the changes work. |
tsibley
force-pushed
the
trs/test-pathogen-repo-ci
branch
from
0122876 to
54fde05
Compare
|
Oh, hmm. To pull from ghcr.io the job needs to be authenticated. That'll require threading some changes thru the shared pathogen-repo-ci workflow. Alternatively, we can switch to the docker.io registry, but then the new test jobs need to move after the push-{build,branch} jobs, and that's potentially annoying for workflow DAG (conditional execution) reasons. And in any case, the pulls from docker.io should ideally still use authentication so we don't hit the low rate limits. |
For docker.io, this lifts low rate limits on image pulls. For ghcr.io, this allows the use of docker-base images we transiently stage there before publishing to docker.io. DOCKER_TOKEN_PUBLIC_READ_ONLY is an org-level secret available to all our public GitHub repos. On Docker Hub, it's granted "public read-only" access as nextstrainbot. Related-to: <nextstrain/docker-base#148>
For docker.io, this lifts low rate limits on image pulls. For ghcr.io, this allows the use of docker-base images we transiently stage there before publishing to docker.io. DOCKER_TOKEN_PUBLIC_READ_ONLY is an org-level secret available to all our public GitHub repos. On Docker Hub, it's granted "public read-only" access as nextstrainbot. Related-to: <nextstrain/docker-base#148>
For docker.io, this lifts low rate limits on image pulls. For ghcr.io, this allows the use of docker-base images we transiently stage there before publishing to docker.io. A new "permissions:" block with "packages: read" restricts the ghcr.io access to read-only. This addition requires explicitly enumerating the rest of the required permissions too, which is only "contents: read". Related-to: <nextstrain/docker-base#148>
This lifts low rate limits on image pulls. However, calling workflows must explicitly opt in with "secrets: inherit" in order for this reusable workflow to be able to see the org-level secret containing the token. Related-to: <nextstrain/docker-base#148>
This allows the use of docker-base images we transiently stage at ghcr.io before publishing to docker.io. A new "permissions:" block with "packages: read" restricts the ghcr.io access to read-only. This addition requires explicitly enumerating the rest of the required permissions too, which is only "contents: read" for actions/checkout. Related-to: <nextstrain/docker-base#148>
tsibley
force-pushed
the
trs/test-pathogen-repo-ci
branch
from
54fde05 to
4471bfc
Compare
|
It works!
Before merge:
|
| test-pathogen-repo-ci: | ||
| needs: build | ||
| strategy: | ||
| # XXX TODO: Test on multiple platforms via the matrix too, as above? |
There was a problem hiding this comment.
Yeah that sounds reasonable!
There was a problem hiding this comment.
To do this we'd first need to add a platform input to the pathogen-repo-ci.yaml shared workflow and update that workflow to DTRT with it, namely: set DOCKER_DEFAULT_PLATFORM and run docker/setup-qemu-action@v2 if the desired platform is not the native platform.
I'm going to leave that for a separate PR, and I may not do it myself right away. Someone else should feel free to beat me to it. :-)
| - { pathogen: lassa } | ||
| - { pathogen: monkeypox } | ||
| - { pathogen: mumps } | ||
| - { pathogen: ncov, build-args: all_regions -j 2 --profile nextstrain_profiles/nextstrain-ci } |
There was a problem hiding this comment.
Really nice config with the build-args, brilliant!
|
Would it make sense to also validate new conda-base environments like this? This could potentially be simple as all we need to do is tell the cli to install and use |
tsibley
force-pushed
the
trs/test-pathogen-repo-ci
branch
from
4471bfc to
f351e4c
Compare
Yep. I'd probably do this with two additional supporting changes in
|
A useful check for if new images will break our pathogen builds. I included all pathogen repos that already use our pathogen-repo-ci reusable workflow. It should be minimal effort to maintain this list over time—I expect it to only grow—but perhaps in the future we will want to abstract it out into a shared list of known pathogen repos. I don't like that we have to copy the build-args for a few of the repos here since it'll be easy for this copy to diverge from the repo's authoritative build-args, but it's necessary for now. Over time as we work towards increased automation of pathogen builds, I think we can get rid of this build-args copy by further standardizing how each repo configures itself for automation. For example, instead of specifying build-args in a repo's CI workflow, the args for CI could be stored in a broader workflow metadata file (e.g. nextstrain-workflow.yaml) read by pathogen-repo-ci, or defined by some other convention. An alternative to directly running pathogen-repo-ci against each repo here would be instead triggering the CI workflows themselves within each repo. The downside to that is it would divorce the outcomes of those workflows from this one and render them not visible from PRs in this repo. It would also require updates to each repo to support triggering and passing in of additional parameters (i.e. for the image). And finally those CI workflows sometimes run other jobs, like linting and other integration tests (e.g. with Cram), that aren't always necessary to run with a new image. Resolves <#147>.
tsibley
force-pushed
the
trs/test-pathogen-repo-ci
branch
from
f351e4c to
12000a2
Compare
|
This passed CI for the PR but then failed CI after the merge to master. The reason why comes down to the difference in The and it ends up trying to enumerate the |
Allows calling workflows to succeed even if the job in this called workflow fails. This is a workaround for the calling working being unable to specify continue-on-error itself.¹ Related-to: <nextstrain/docker-base#148> ¹ <https://docs.github.com/en/actions/using-workflows/reusing-workflows#supported-keywords-for-jobs-that-call-a-reusable-workflow>
This makes them advisory-only, as they're currently broken.¹ We may also opt to leave them advisory-only longer term, as they might be noisy and not fit for stopping an image release. Idea for the pass thru this uses from @joverlee521.² Related-to: <#148> ¹ <#148 (comment)> ² <#148 (comment)>
Allows calling workflows to succeed even if the job in this called workflow fails. This is a workaround for the calling working being unable to specify continue-on-error itself.¹ Related-to: <nextstrain/docker-base#148> ¹ <https://docs.github.com/en/actions/using-workflows/reusing-workflows#supported-keywords-for-jobs-that-call-a-reusable-workflow>
|
Nextstrain CLI's handling here should be improved, but I'll also address the issue separately in this repo. |
|
#928 should fix the broken CI by ignoring errors in the |
… ghcr.io This should fix the currently-broken test-pathogen-repo-ci jobs when our CI runs on master² (and they should continue to work on branches too). It requires a little rearranging of jobs in the workflow, with a little additional and unfortunate complexity due to conditionals. This works around a Nextstrain CLI bug with registries other than docker.io¹ during `nextstrain update docker` (and `nextstrain setup docker`), which is run as part of our setup-nextstrain-cli action used by this workflow. Ideally we'll fix that bug and then be able to revert this, especially since we might actually want to condition the pushing to docker.io on the outcome of these test jobs in the future. ¹ <nextstrain/cli#279> ² <#148 (comment)>
I assume you meant nextstrain/.github#40 + #150 ? |
|
Indeed. … I don't know where #928 came from! |
[ Commit message based on that of 12000a20 in nextstrain/docker-base.¹ Code changes also based on that commit, plus subsequent commits.² ] A useful check for if new packages will break our pathogen builds. I included all pathogen repos that already use our pathogen-repo-ci reusable workflow. It should be minimal effort to maintain this list over time—I expect it to only grow—but perhaps in the future we will want to abstract it out into a shared list of known pathogen repos. I don't like that we have to copy the build-args for a few of the repos here since it'll be easy for this copy to diverge from the repo's authoritative build-args, but it's necessary for now. Over time as we work towards increased automation of pathogen builds, I think we can get rid of this build-args copy by further standardizing how each repo configures itself for automation. For example, instead of specifying build-args in a repo's CI workflow, the args for CI could be stored in a broader workflow metadata file (e.g. nextstrain-workflow.yaml) read by pathogen-repo-ci, or defined by some other convention. An alternative to directly running pathogen-repo-ci against each repo here would be instead triggering the CI workflows themselves within each repo. The downside to that is it would divorce the outcomes of those workflows from this one and render them not visible from PRs in this repo. It would also require updates to each repo to support triggering and passing in of additional parameters (i.e. for the package). And finally those CI workflows sometimes run other jobs, like linting and other integration tests (e.g. with Cram), that aren't always necessary to run with a new package. Related-to: <nextstrain/docker-base#148> Related-to: <nextstrain/docker-base#150> Related-to: <nextstrain/docker-base#151> ¹ <nextstrain/docker-base@12000a20> ² <nextstrain/docker-base@bc22a0bc> <nextstrain/docker-base@0a20a474> <nextstrain/docker-base@75254e92>
|
I wrote:
Both supporting changes done in nextstrain/.github#42. Corresponding updates for docker-base in #154. Testing for conda-base in nextstrain/conda-base#27. |
[ Commit message based on that of 12000a20 in nextstrain/docker-base.¹ Code changes also based on that commit, plus subsequent commits.² ] A useful check for if new packages will break our pathogen builds. I included all pathogen repos that already use our pathogen-repo-ci reusable workflow. It should be minimal effort to maintain this list over time—I expect it to only grow—but perhaps in the future we will want to abstract it out into a shared list of known pathogen repos. I don't like that we have to copy the build-args for a few of the repos here since it'll be easy for this copy to diverge from the repo's authoritative build-args, but it's necessary for now. Over time as we work towards increased automation of pathogen builds, I think we can get rid of this build-args copy by further standardizing how each repo configures itself for automation. For example, instead of specifying build-args in a repo's CI workflow, the args for CI could be stored in a broader workflow metadata file (e.g. nextstrain-workflow.yaml) read by pathogen-repo-ci, or defined by some other convention. An alternative to directly running pathogen-repo-ci against each repo here would be instead triggering the CI workflows themselves within each repo. The downside to that is it would divorce the outcomes of those workflows from this one and render them not visible from PRs in this repo. It would also require updates to each repo to support triggering and passing in of additional parameters (i.e. for the package). And finally those CI workflows sometimes run other jobs, like linting and other integration tests (e.g. with Cram), that aren't always necessary to run with a new package. Related-to: <nextstrain/docker-base#148> Related-to: <nextstrain/docker-base#150> Related-to: <nextstrain/docker-base#151> Related-to: <nextstrain/docker-base#154> ¹ <nextstrain/docker-base@12000a20> ² <nextstrain/docker-base@bc22a0bc> <nextstrain/docker-base@0a20a474> <nextstrain/docker-base@75254e92>
A useful check for if new images will break our pathogen builds.
I included all pathogen repos that already use our pathogen-repo-ci reusable workflow. It should be minimal effort to maintain this list over time—I expect it to only grow—but perhaps in the future we will want to abstract it out into a shared list of known pathogen repos.
I don't like that we have to copy the build-args for a few of the repos here since it'll be easy for this copy to diverge from the repo's authoritative build-args, but it's necessary for now. Over time as we work towards increased automation of pathogen builds, I think we can get rid of this build-args copy by further standardizing how each repo configures itself for automation. For example, instead of specifying build-args in a repo's CI workflow, the args for CI could be stored in a broader workflow metadata file (e.g. nextstrain-workflow.yaml) read by pathogen-repo-ci, or defined by some other convention.
An alternative to directly running pathogen-repo-ci against each repo here would be instead triggering the CI workflows themselves within each repo. The downside to that is it would divorce the outcomes of those workflows from this one and render them not visible from PRs in this repo. It would also require updates to each repo to support triggering and passing in of additional parameters (i.e. for the image). And finally those CI workflows sometimes run other jobs, like linting and other integration tests (e.g. with Cram), that aren't always necessary to run with a new image.
Resolves #147.
Testing