-
Notifications
You must be signed in to change notification settings - Fork 11
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
Update pathogen-repo-ci #90
Conversation
This should be handled within the build-config of the consuming repo if it is required.
4f06c08
to
fe768d3
Compare
…per PR feedback * ignore/don't warn when no files found * include `auspcice` directory in upload set * sort lines
…89] * Pass `runtime` as an input, rather than accessing `matrix.runtime` directly * Update action description to reflect dependency on having Nextstrain CLI already set up via other action; add warning about the purpose of this action
Okay, I think that I've addressed everything; ready for re-review. |
Not quite. The point was to test that the (phylo) build ran even when example data was omitted (because it's optional before this PR). But because |
...and still nothing ran in ETA: actually, it's not yet because that work is unmerged and blocks on this work. sigh Gonna add |
...and it still didn't run because the config file isn't named as expected sigh |
|
* Update `artifact-name` description in `pathogen-repo-ci.yaml` to make it clear build directory will be appended to provided value, and to update default value to make clear these are CI results * Pass `artifact-name` down into `run-nextstrain-ci-build` steps ni `pathogen-repo-ci` workflow * Add `artifact-name` input to `run-nextstrain-ci-build` action and use it in `upload-artifact` action step
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 to merge, I think.
FWIW, the commits on this branch are of the "all the messy details and false starts" style instead of "tells a story" style. I have a strong preference for the latter, but I won't try to insist on it.
To me, there is a story here — the story is, "here's what I originally thought would satisfy the rquirements, and here's all the stuff people told me in code review that I should change". If you have specific things to point to and say (e.g.), "instead of adding this commit, I would have squashed it back here" or "I would combine all the PR feedback into a single commit at the end", or something like that, I'm happy to listen to that feedback and try to take it into account in the future — but I assure you, I am trying to tell a story with my branches, although perhaps more of a "lab notebook" version than a "journal article" recounting. |
Got it. I didn't give specific feedback here because I didn't intend to insist on it. I'll try to give more specific feedback in the future. |
Description of proposed changes
Adds "smarts" to
pathogen-repo-ci
.Also switches it over to using the "modern" style for building Nextstrain runtimes, using a sidecar repo to ensure everything runs from the same commit in the
nextstrain/.github
repo.Related issue(s)
#89 #63
Checklist