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

Step conditions using !cancelled() need explict success check #117

Open
genehack opened this issue Nov 22, 2024 · 2 comments
Open

Step conditions using !cancelled() need explict success check #117

genehack opened this issue Nov 22, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@genehack
Copy link
Contributor

genehack commented Nov 22, 2024

Current Behavior

Slack context

- name: Run phylogenetic
if: hashFiles('phylogenetic/Snakefile') && hashFiles('phylogenetic/build-configs/ci/config.yaml') && !cancelled()
id: phylogenetic
run: nextstrain build phylogenetic --configfile build-configs/ci/config.yaml
- name: Run nextclade
if: hashFiles('nextclade/Snakefile') && hashFiles('nextclade/build-configs/ci/config.yaml') && !cancelled()
id: nextclade
run: nextstrain build nextclade --configfile build-configs/ci/config.yaml

Expected behavior

The Conda build step failing should abort the rest of the workflow

Possible solution

  • Add an explicit success() check
  • ??
@genehack genehack added the bug Something isn't working label Nov 22, 2024
@tsibley
Copy link
Member

tsibley commented Nov 22, 2024

Background: The commit which switched to using !cancelled() instead of continue-on-error: true (which didn't have this bug).

Adding a success() check alone won't DTRT, as we intentionally wanted the phylogenetic step to run even if the ingest step failed and the nextclade step to run even if the ingest or phylogenetic steps failed. Conceptually, we want to check that every step before the ingest step was successful.

Possible solutions:

  1. Calculate status of steps before ingest explicitly (in a new step) using something like contains(steps.*.conclusion, 'failure'), setting it as the new step output, and then condition on that in ingest, phylogenetic, and nextclade (in addition to continuing to condition on !cancelled()).

  2. Move each of ingest, phylogenetic, and nextclade into their own jobs, where they're naturally independent of each other, instead of doing a lot of work to make them independent steps in the same job.

@victorlin
Copy link
Member

  1. Move each of ingest, phylogenetic, and nextclade into their own jobs, where they're naturally independent of each other, instead of doing a lot of work to make them independent steps in the same job.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants