Make pathogen-repo-ci fail when config is missing or no builds are attempted
#95
Conversation
genehack
force-pushed
the
bring-tha-noise-92
branch
6 times, most recently
from
82233df
to
6b2a3c0
Compare
|
The one remaining thing here is figuring out what to do with test-pathogen-repo-ci-no-example-data:
uses: ./.github/workflows/pathogen-repo-ci.yaml
with:
repo: nextstrain/zika-tutorial
artifact-name: outputs-test-pathogen-repo-ci-no-example-dataoptions:
@joverlee521 @tsibley could either of you shed insight on the "what is this job for" question? |
Ah, I tried to answer that previously, but sorry there's still a disconnect here. In retrospect, I think I assumed too much context. I think the commit which introduces the job, 9800e33, is a clear and concise explanation with the right context, but if it's still not I can try again. |
So, based on the context in that commit message, I think the right thing to do here is just remove this job from the CI? The thing that it's validating (don't need to set up if example data doesn't exist) is something that the "modern" ETA: it's removed |
|
This is updated and seems to be working; will try to merge this EOD Wednesday (PT), if not earlier. |
There was a problem hiding this comment.
Looks ok by inspection. A few nits.
My biggest suggestion is that we don't actually run the "Run {ingest, phylogenetic, nextclade}" steps if they're not going to do anything. This means that instead of checking for whether to do anything or not inside each step individually, we'd change to checking the conditions for the steps outside first (in one fell swoop). Then we'd add if: … conditionals to the steps. This will make the logs a lot clearer and mean the GitHub workflow metadata stays more in sync with reality. I'll try this out in a separate PR. I don't think it has to block this one.
The advantage I see to doing the file existence checks inside the action is that it can report out exactly which file(s) aren't present. Maybe you can do that with some sort of |
genehack
force-pushed
the
bring-tha-noise-92
branch
from
e028f39
to
422bff1
Compare
|
minor nits have been picked. |
… to do anything Instead of checking for whether to do anything or not _inside_ each build step individually, move the check to the step's condition. This will make the logs a lot clearer and mean the GitHub workflow metadata stays more in sync with reality. I've used hashFiles() to check for file existence—it returns the empty string, a falsey value, when there are no matching files—but we could swap that out for using the output of a prior setup step that runs some shell to determine what to run and what to skip. Doing so might make more sense if the conditional becomes more complicated or we want to do more detailed reporting on _why_ steps were skipped or not. With the changes, the internal-to-this-workflow action, run-nextstrain-ci-build, is no longer that useful and so I've removed it in favor of inlining things. I think this is an improvement for readability. Related-to: <#95 (review)>
… to do anything Instead of checking for whether to do anything or not _inside_ each build step individually, move the check to the step's condition. This will make the logs a lot clearer and mean the GitHub workflow metadata stays more in sync with reality. I've used hashFiles() to check for file existence—it returns the empty string, a falsey value, when there are no matching files—but we could swap that out for using the output of a prior setup step that runs some shell to determine what to run and what to skip. Doing so might make more sense if the conditional becomes more complicated or we want to do more detailed reporting on _why_ steps were skipped or not. With the changes, the internal-to-this-workflow action, run-nextstrain-ci-build, is no longer that useful and so I've removed it in favor of inlining things. I think this is an improvement for readability. Related-to: <#95 (review)>
… to do anything Instead of checking for whether to do anything or not _inside_ each build step individually, move the check to the step's condition. This will make the logs a lot clearer and mean the GitHub workflow metadata stays more in sync with reality. I've used hashFiles() to check for file existence—it returns the empty string, a falsey value, when there are no matching files—but we could swap that out for using the output of a prior setup step that runs some shell to determine what to run and what to skip. Doing so might make more sense if the conditional becomes more complicated or we want to do more detailed reporting on _why_ steps were skipped or not. With the changes, the internal-to-this-workflow action, run-nextstrain-ci-build, is no longer that useful and so I've removed it in favor of inlining things. I think this is an improvement for readability. Related-to: <#95 (review)>
… to do anything Instead of checking for whether to do anything or not _inside_ each build step individually, move the check to the step's condition. This will make the logs a lot clearer and mean the GitHub workflow metadata stays more in sync with reality. I've used hashFiles() to check for file existence—it returns the empty string, a falsey value, when there are no matching files—but we could swap that out for using the output of a prior setup step that runs some shell to determine what to run and what to skip. Doing so might make more sense if the conditional becomes more complicated or we want to do more detailed reporting on _why_ steps were skipped or not. With the changes, the internal-to-this-workflow action, run-nextstrain-ci-build, is no longer that useful and so I've removed it in favor of inlining things. I think this is an improvement for readability. Related-to: <#95 (review)>
|
Take a gander at #96 and see what you think?
Nod. Reporting that out is still possible, but I didn't bother in the PR above. It seems like a minor thing to me: if a step is skipped unexpectedly, it's easy enough to grab the commit id checked out from the logs and look at what files exist in it? But we could still maintain that reporting if desired. |
There was a problem hiding this comment.
Thanks for the work here @genehack! I left a minor suggestion, but the changes here LGTM!
| echo "INGEST ATTEMPTED=${{ steps.ingest.outputs.run-attempted }}" | ||
| echo "PHYLOGENETIC ATTEMPTED=${{ steps.phylogenetic.outputs.run-attempted }}" | ||
| echo "NEXTCLADE ATTEMPTED=${{ steps.nextclade.outputs.run-attempted }}" |
There was a problem hiding this comment.
Minor suggestion to output these to the $GITHUB_STEP_SUMMARY so that it's easy to scan the workflow attempts from the GH Action summary instead of having to dig into the job logs.
There was a problem hiding this comment.
pushed an update — was that what you were thinking of?
There was a problem hiding this comment.
Yup, that's what I was thinking, except small typo $"GITHUB_STEP_SUMMARY" -> "$GITHUB_STEP_SUMMARY"
There was a problem hiding this comment.
>_< thanks -- pushed a fix, gonna merge this once CI finishes
genehack
force-pushed
the
bring-tha-noise-92
branch
2 times, most recently
from
1bad99e
to
df99e08
Compare
* Add ids to build steps in `pathogen-repo-ci` * Add `run-attempted` output to `run-nextstrain-ci-build` * Set output to true or false depending on whether build was attempted * Add step to `pathogen-repo-ci` to read outputs from `run-nextstrain-ci-build` and validate at least one build was tried
genehack
force-pushed
the
bring-tha-noise-92
branch
from
df99e08
to
fea28cb
Compare
… to do anything Instead of checking for whether to do anything or not _inside_ each build step individually, move the check to the step's condition. This will make the logs a lot clearer and mean the GitHub workflow metadata stays more in sync with reality. I've used hashFiles() to check for file existence—it returns the empty string, a falsey value, when there are no matching files—but we could swap that out for using the output of a prior setup step that runs some shell to determine what to run and what to skip. Doing so might make more sense if the conditional becomes more complicated or we want to do more detailed reporting on _why_ steps were skipped or not. With the changes, the internal-to-this-workflow action, run-nextstrain-ci-build, is no longer that useful and so I've removed it in favor of inlining things. I think this is an improvement for readability. Related-to: <#95 (review)>
… to do anything Instead of checking for whether to do anything or not _inside_ each build step individually, move the check to the step's condition. This will make the logs a lot clearer and mean the GitHub workflow metadata stays more in sync with reality. I've used hashFiles() to check for file existence—it returns the empty string, a falsey value, when there are no matching files—but we could swap that out for using the output of a prior setup step that runs some shell to determine what to run and what to skip. Doing so might make more sense if the conditional becomes more complicated or we want to do more detailed reporting on _why_ steps were skipped or not. With the changes, the internal-to-this-workflow action, run-nextstrain-ci-build, is no longer that useful and so I've removed it in favor of inlining things. I think this is an improvement for readability. Related-to: <#95 (review)>
Description of proposed changes
Make the CI fail if it doesn't try to run at least one build step, as that's an indication that somebody is trying to use "modern"
pathogen-repo-cion an un-modernized pathogen repo (or there's some other grievous misconfig happening).Related issue(s)
#92
Checklist
pathogen-nextstrain.yamlfails as expected proofpathogen-nexstrain.yamlbut lacking required files for all build steps fails as expected proofVerify a repo that runs all three steps still succeeds as expected— skipping because I don't think there's a repo handy with anextcladebuild that's also using pathogen-repo-ci(?)