feat(pathogen-repo-ci): allow configuration of workflow root #57
Conversation
| @@ -63,6 +63,14 @@ on: | |||
| - conda | |||
| required: false | |||
|
|
|||
| workflow-root: | |||
There was a problem hiding this comment.
"Workflow" is an overloaded term in this context (GitHub workflow vs. pathogen workflow). What about a simpler name like path, root, or directory? That would look like:
jobs:
pathogen-ci:
uses: nextstrain/.github/.github/workflows/pathogen-repo-ci.yaml@master
with:
path: phylogenetic/There was a problem hiding this comment.
Hmm, I'm not sure the simpler path is as clear. Yes there's ambiguity in workflow-root - but path is even more ambiguous.
There was a problem hiding this comment.
Ok, also fine for this to be non-blocking. It works, I don't have any better suggestions, and this is something we can change easily if it's only used internally.
There was a problem hiding this comment.
build-dir, i.e. "the path for nextstrain build"
There was a problem hiding this comment.
I'd be happy with build-dir or build-root
| $workdir/auspice/ | ||
| $workdir/results/ | ||
| $workdir/benchmarks/ | ||
| $workdir/logs/ | ||
| $workdir/.snakemake/log/ |
There was a problem hiding this comment.
This currently only accepts paths without a trailing slash. I think it'd be good to accept both with and without, because even if we document support for only one in the input description, the error that happens here might not be very informative and only happens after everything else is done running.
I don't think there's an easy way to do something like Python's os.path.join inline here, so the solution might be to normalize as a first step before using the value.
There was a problem hiding this comment.
This sounds like a non-blocking feature request to me :)
There was a problem hiding this comment.
Yeah it works, so this can be non-blocking.
There was a problem hiding this comment.
This sounds like a non-blocking feature request to me :)
This sounds like a flippant response to a reasonable request for robustness.
The trailing slash is not even a documented requirement in the input description. But even if it was, requiring a slash is an easily-avoided tripping hazard that we don't need to subject ourselves to.
Co-authored-by: Victor Lin <13424970+victorlin@users.noreply.github.com>
| $workdir/auspice/ | ||
| $workdir/results/ | ||
| $workdir/benchmarks/ | ||
| $workdir/logs/ | ||
| $workdir/.snakemake/log/ |
There was a problem hiding this comment.
This sounds like a non-blocking feature request to me :)
This sounds like a flippant response to a reasonable request for robustness.
The trailing slash is not even a documented requirement in the input description. But even if it was, requiring a slash is an easily-avoided tripping hazard that we don't need to subject ourselves to.
| - if: always() | ||
| uses: actions/upload-artifact@v3 | ||
| env: | ||
| workdir: ${{ inputs.workflow-root }} | ||
| with: | ||
| name: ${{ inputs.artifact-name }}-${{ matrix.runtime }} | ||
| path: | | ||
| auspice/ | ||
| results/ | ||
| benchmarks/ | ||
| logs/ | ||
| .snakemake/log/ | ||
| $workdir/auspice/ | ||
| $workdir/results/ | ||
| $workdir/benchmarks/ | ||
| $workdir/logs/ | ||
| $workdir/.snakemake/log/ |
There was a problem hiding this comment.
This doesn't work. The interpolation does not work like that. If you look at the "successful" test, you'll see it didn't find any files and uploaded no artifacts.
) * Move phylogenetic workflow from top-level to folder `phylogenetic` * wip: use the experimental workflow from nextstrain/.github#57
wip: use the experimental workflow from nextstrain/.github#57 wip wip Add general precommit lints Add codespell to precommit Add shellcheck to precommit Add tomlsort to precommit chore: lint/formatting
Current
pathogen-repo-ciseems to assume that we don't need to customize workdir innextstrain build ., i.e. we can't setnextstrain build foobar.Our nextstrain/pathogen-repo-template, however, puts the main workflow in a folder. So we should add that feature.
See https://bedfordlab.slack.com/archives/C01LCTT7JNN/p1695664810319929 for discussion
Testing
It works: https://github.com/nextstrain/monkeypox/actions/runs/6303654582/job/17113366921