-
Notifications
You must be signed in to change notification settings - Fork 0
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 CI #11
Conversation
Noting that with these first set of changes, running |
be90a1a
to
7ea8972
Compare
Hurdle: The The one thing that jumps out at me is updating the base workflow so that the build target can be passed in (defaulting to |
211edd3
to
629df84
Compare
Ah, sorry if this wasn't clear! I've been using the |
06e2890
to
137a7a5
Compare
I made a local copy-fork of Have it working for the ingest build, currently working on extending to download ingest results before the phylogenetic build. |
d886ecf
to
4081ebd
Compare
This adds the following features to the `pathogen-repo-ci` workflow: * defines a `build-target` argument that will get passed to `nextstrain build` as a target directory — this allows the targeting of sub-dir based workflows, which was the main original issue in #63 * defines arguments (`download-previous-artifact`, `previous-artifact-name`) and a conditional step (`Dowload previous step artifact`) to support the downloading of the build artifact from a previous step to allow testing of later steps in a complete pathogen repo build This combination of features was all that was needed to support [a CI workflow for `seasonal-cov`](nextstrain/seasonal-cov#11) that: 1. runs an ingest workflow for a subset of the regular build data 2. uploads an artifact with the ingest output 3. downloads that artifact into a phylogenetic workflow 4. runs the phylogenetic workflow for the same subset of data
Note: changes to the core |
.github/workflows/ci.yaml
Outdated
uses: nextstrain/.github/.github/workflows/pathogen-repo-ci.yaml@master | ||
name: nextstrain build ingest | ||
uses: ./.github/workflows/pathogen-repo-ci.yaml |
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.
FWIW, instead of a local fork, you can point to the centralized workflow on an alternative branch, e.g.
uses: nextstrain/.github/.github/workflows/pathogen-repo-ci.yaml@master | |
name: nextstrain build ingest | |
uses: ./.github/workflows/pathogen-repo-ci.yaml | |
uses: nextstrain/.github/.github/workflows/pathogen-repo-ci.yaml@update-pathogen-repo-ci-63 |
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 — I was iterating pretty rapidly on both the underlying "base" workflow and the ci.yaml in this repo so it was faster to just be able to keep amending the commit and force pushing the one repo. (I'm not particularly happy about that but I think if I was force pushing branches in 2 repos at the same time where one was dependent on the other, I would have tripped over my own feet a lot ...more.)
4081ebd
to
95b63d2
Compare
Noting that this is going to stall briefly behind the changes discussed in today's lab meeting around standardizing ETA: specifically, this PR needs to land and then the local fork in this PR should be cleaned up. |
[Slack context](https://bedfordlab.slack.com/archives/C0K3GS3J8/p1716315967004439) with discussion of different ways to accomplish this. I think overall this approach is better; it does cause an extremely tight coupling between the two parts of the config, but that coupling was already present in the old `VIRUSES` global in the Snakemake files; this mainly changes where that list lives.
Blocked on nextstrain/.github#89 |
aff4512
to
fdfc343
Compare
This has been pointed at the It is ready for another look. |
No description provided.