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

Test.yml: remove allow-failure #48

Closed
wants to merge 1 commit into from

Conversation

suvayu
Copy link
Collaborator

@suvayu suvayu commented Feb 20, 2024

allow-failure: false is the default, Adding it in the matrix, breaks subsequent include: modifications. E.g. see this run on TulipaIO. Instead of overiding allow-failure, it creates duplicate runs.

This is inline with the docs. See the rule for {fruit:banana}.

allow-failure: false is the default,  Adding it in the matrix, breaks subsequent include: modifications.  E.g. see this run on TulipaIO:
https://github.com/TulipaEnergy/TulipaIO.jl/actions/runs/7977712404

Instead of overiding allow-failure it creates duplicate runs.

This is inline with the docs:
https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#expanding-or-adding-matrix-configurations

See the rule for {fruit:banana}
@suvayu
Copy link
Collaborator Author

suvayu commented Feb 20, 2024

I would like to meet the person who created these rules of precedence 😖

@abelsiqueira
Copy link
Collaborator

Can't reproduce using https://github.com/nektos/act and the workflow

name: Test

on:
  push:

jobs:
  test:
    name: Test
    runs-on: ubuntu-latest
    strategy:
      fail-fast: false
      matrix:
        version:
          - "1.6"
          - "1"
        allow_failure: [false]
        include:
          - version: nightly
            allow_failure: true
          - version: 1
            allow_failure: true
    steps:
      - run: |
          echo "Version ${{ matrix.version }}, allow_failure=${{ matrix.allow_failure }}"

Running act -W this.yml will run the 4 combinations correctly. include will override the default allow_failure from false to true.

Let me know if I am wrong.

That being said, I noticed that I nightly should not have been included in the list. It is duplicated in the template.

@suvayu
Copy link
Collaborator Author

suvayu commented Feb 23, 2024

Yes, but that's not the correct behaviour. You never want to run the same pipeline twice once with allow_failure: false and again with allow_failure: true.

In the above test example, these are the final options:

  • version: "1.6", allow_failure: false
  • version: "1", allow_failure: false
  • version: "1", allow_failure: true <- this is redundant
  • version: "nightly", allow_failure: true

In the case of the template nightly is in the list, so you get:

  1. version: "nightly", allow_failure: false
  2. version: "nightly", allow_failure: true

when the intention was actually to override 1.

I noticed that I nightly should not have been included in the list

That's a separate issue. Although, removing it from the list does "solve" this one.

@abelsiqueira
Copy link
Collaborator

Ah, ok, that makes sense. That being said, removing allow_failure is still not the solution, because the example workflow still generates 4 jobs, apparently. 2 of them have empty allow_failure (which I suppose sets a default allow_failure).
The error is having nightly in the list, since it should only be in the include. I fixed that with #50.
Do you agree?

@suvayu
Copy link
Collaborator Author

suvayu commented Feb 24, 2024

the example workflow still generates 4 jobs, apparently. 2 of them have empty allow_failure (which I suppose sets a default allow_failure).

This is strange because the docs say otherwise, see the first two examples with shape: circle.

Agree with dropping "nightly" from the list.

@abelsiqueira
Copy link
Collaborator

Indeed you're right. I've created a playground repo and can reproduce the docs behaviour: https://github.com/abelsiqueira/github-workflow-playground/actions/runs/8037366367.
It looks like act has a bug: nektos/act#2220.

Can we close this?

@suvayu
Copy link
Collaborator Author

suvayu commented Feb 26, 2024

Sure, we can close

@suvayu suvayu deleted the allow-failure branch July 3, 2024 20:44
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.

2 participants