actions / runner

The Runner for GitHub Actions :rocket:
https://github.com/features/actions
MIT License
4.87k stars 957 forks source link

if conditional evaluated wrong for contains(needs.*.result, 'failure') #1540

Open samanthawalter opened 2 years ago

samanthawalter commented 2 years ago

Describe the bug A job-level if conditional isn't evaluated properly when using the contains function and array wildcard syntax.

For example, if contains(needs.*.result, 'failure') evaluates to false, a job with if: always() && contains(needs.*.result, 'failure') will run, but a job with if: always() && (needs.job_a.result == 'failure' || needs.job_b.result == 'failure') won't run.

To Reproduce

  1. Run a workflow with the following yaml configuration. Examining the output reveals that: a. Job a fails b. Job b is skipped c. Job c is skipped d. Job d runs (unexpected) even though contains(needs.*.result, 'failure') evaluates to false
jobs:
  a:
    runs-on: ubuntu-latest
    steps:
      - run: |
          echo "This job fails."
          exit 1
  b:
    runs-on: ubuntu-latest
    if: ${{ false }}
    needs: a
    steps:
      - run: echo "This job never happens."
  c:
    runs-on: ubuntu-latest
    if: ${{ false }}
    needs: [a, b]
    steps:
      - run: echo "This job never happens."
  d:
    runs-on: ubuntu-latest
    needs: [b, c]
    if: always() && contains(needs.*.result, 'failure')
    steps:
      - run: |
          echo "I don't want this to run unless b or c fails!"
          echo ${{ contains(needs.*.result, 'failure') }}   # Evaluates to false!

image

  1. However, running a workflow with the following yaml has the expected outcome: a. Job a fails b. Job b is skipped c. Job c is skipped d. Job d is skipped (expected)
jobs:
  a:
    runs-on: ubuntu-latest
    steps:
      - run: |
          echo "This job fails."
          exit 1
  b:
    runs-on: ubuntu-latest
    if: ${{ false }}
    needs: a
    steps:
      - run: echo "This job never happens."
  c:
    runs-on: ubuntu-latest
    if: ${{ false }}
    needs: [a, b]
    steps:
      - run: echo "This job never happens."
  d:
    runs-on: ubuntu-latest
    needs: [b, c]
    if: always() && (needs.b.result == 'failure' || needs.c.result == 'failure')  # ONLY CHANGE
    steps:
      - run: |
          echo "I don't want this to run unless b or c fails!"
          echo ${{ contains(needs.*.result, 'failure') }}   # Evaluates to false!

image

Expected behavior if: always() && contains(needs.*.result, 'failure') should have the same behavior as if: always() && (needs.b.result == 'failure' || needs.c.result == 'failure'). In other words, if: always() && contains(needs.*.result, 'failure') should evaluate to false and job d should be skipped.

Runner Version and Platform

Version of your runner? Not sure - running with GitHub Actions (not self-hosted runner)

OS of the machine running the runner? ubuntu-latest

TingluoHuang commented 2 years ago

I think I know why you are seeing this, when evaluating job level if condition, we are pulling in both direct and in-direct dependency. So, in your example of if: always() && contains(needs.*.result, 'failure'), the * actually means a, b and c, since both job a is in-direct required by d That's why contains(needs.*.result, 'failure')=>true and (needs.b.result == 'failure' || needs.c.result == 'failure')=>false I guess if you try (needs.b.result == 'failure' || needs.c.result == 'failure' || needs.a.result == 'failure'), it will return true.

@ericsciple do you recall why we need to pull in the indirect dependency to evaluate job conditions? Looks like we don't send those indirect dependencies as context data to the runner.

    "needs": {
      "t": 2,
      "d": [
        {
          "k": "b",
          "v": {
            "t": 2,
            "d": [
              {
                "k": "result",
                "v": "skipped"
              },
              {
                "k": "outputs",
                "v": {
                  "t": 2
                }
              }
            ]
          }
        },
        {
          "k": "c",
          "v": {
            "t": 2,
            "d": [
              {
                "k": "result",
                "v": "skipped"
              },
              {
                "k": "outputs",
                "v": {
                  "t": 2
                }
              }
            ]
          }
        }
      ]
    },
dbarbier commented 2 years ago

Hi there, not related to the original issue, but could this inconsistency with respect to indirect dependencies please be fixed? The same condition could pass at the job level but not at the step level, this is error prone: I spent some time debugging a workflow because of that. Thanks!

ericsciple commented 2 years ago

do you recall why we need to pull in the indirect dependency to evaluate job conditions?

Nope. That surprises me. I do see the success() that is used for the job-if condition checks all needs so we should think through potential behavior changes when we fix this.

klutchell commented 1 year ago

This issue really caught me off guard, because expanding the needs array in the job context only shows the immediate dependencies, but evaluating needs.* in the job conditions behaves as if it includes all jobs in the dependency chain.

Any progress on this to avoid having checking the results of individual jobs rather than the array?

smkjason commented 1 year ago

Indirect dependencies also come as a surprise to me.

In the context of the original example, if job d only has b and c as its dependency, it should really only depend on job b and c and the status of job a should not be a dependency.

Consider another scenario where this indirect dependency creates confusion:

  1. job a skips
  2. job b runs
  3. job c skips (Unexpected, it only depends on job b being successful)
 jobs:
  a:
    runs-on: ubuntu-latest
    if: github.event_name == 'schedule'
    steps:
      - run: |
          echo "This job got skipped"
          exit 1
  b:
    runs-on: ubuntu-latest
    if: |
      always() && 
      (needs.a.result == 'success' || needs.a.result == 'skipped')
    needs: a
    steps:
      - run: echo "This job still happens"
  c:
    runs-on: ubuntu-latest
    needs: [b]
    steps:
      - run: echo "I expect this job to happen but gets skipped"
kvfairchild commented 1 year ago

Just hopping on to hopefully encourage some movement on this issue; I spent way too long debugging my workflow before finding the solution in this thread.

Am3ra commented 1 year ago

Ran into this issue as well. Had many long workflows, and use the if: always() && contains(needs.*.result, "failure") for retrying certain fallible jobs. This tripped me up, as once one retry job ran, all subsequent jobs also ran. (can't use normal retry methods because of company policies :/ )

Anyways, would love updates on this issue as it is not at all straightforward.

pepitoenpeligro commented 9 months ago

Any news on this?