Skip to content
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

setup-nextstrain-cli: allow nextstrain update to continue-on-error #49

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

joverlee521
Copy link
Contributor

@joverlee521 joverlee521 commented Sep 5, 2023

Description of proposed changes

I missed in #48 that the command nextstrain update will exit with error for aws-batch and ambient runtimes. Instead of maintaining a list of runtime to run the update command, just allow it to continue-on-error.

I found this error in separate work to update forecasts-ncov to use the pathogen-repo-build workflow with the aws-batch runtime.

Checklist

  • Checks pass

I missed in #48 that the
command `nextstrain update` will exit with error for aws-batch and
ambient runtimes. Instead of maintaining a list of runtime to run the
update command, just allow it to continue-on-error.
@joverlee521 joverlee521 requested a review from a team September 5, 2023 20:56
@joverlee521
Copy link
Contributor Author

Tested this in forecasts-ncov workflow

Green checkmarks throughout the run even though the nextstrain update command failed. The Annotations section of the summary page does note the error, but the link to the log line is incorrect because of actions/runner#1901

@joverlee521 joverlee521 merged commit b59b79a into master Sep 5, 2023
34 of 38 checks passed
@joverlee521 joverlee521 deleted the fix-setup-nextstrain-cli branch September 5, 2023 22:27
@corneliusroemer
Copy link
Member

I find that error in the annotations summary page confusing. Would it be possible to do something about that? Like add a note to the annotation: "this is an expected failure" or the like?
See: https://bedfordlab.slack.com/archives/C0K3GS3J8/p1695495002980659

@victorlin
Copy link
Member

I can't think of an easy way to hide or rewrite the error. I think the proper solution is to remove this nextstrain update step (it's only in place because the conda runtime does not use the latest version during nextstrain setup, as mentioned in #48).

@corneliusroemer
Copy link
Member

@victorlin wasn't the update step added to address the conda issue? We could fix the root cause there, or alternatively: make that setup step conditional on conda runtime being used?

@joverlee521 wrote:

Instead of maintaining a list of runtime to run the update command, just allow it to continue-on-error.

In my view the confusing error message is worth maintaining a list of 1 that will need to almost never be updated 🙃

@tsibley
Copy link
Member

tsibley commented Sep 25, 2023

wasn't the update step added to address the conda issue? We could fix the root cause there

The root cause was already fixed by nextstrain/cli#312. I believe the nextstrain update is now extraneous.

@victorlin
Copy link
Member

@tsibley oh I missed that, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants