-
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
Make pathogen-repo-ci
fail when config is missing or no builds are attempted
#95
Conversation
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-data options:
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed an update — was that what you were thinking of?
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.
Yup, that's what I was thinking, except small typo $"GITHUB_STEP_SUMMARY"
-> "$GITHUB_STEP_SUMMARY"
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 -- pushed a fix, gonna merge this once CI finishes
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.
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
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-ci
on an un-modernized pathogen repo (or there's some other grievous misconfig happening).Related issue(s)
#92
Checklist
pathogen-nextstrain.yaml
fails as expected proofpathogen-nexstrain.yaml
but 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 anextclade
build that's also using pathogen-repo-ci(?)