runner icon indicating copy to clipboard operation
runner copied to clipboard

Job-level "if" condition not evaluated correctly if job in "needs" property is skipped

Open whois-jon-peterson opened this issue 5 years ago • 32 comments

Describe the bug If a job needs a prior job which has been skipped, the if condition for the dependent job may behave unexpectedly under some conditions. (unsure if condition is evaluated incorrectly or if it's not evaluated at all)

To Reproduce Steps to reproduce the behavior: Given a workflow such as this (where job_b needs to complete or be skipped before subsequent jobs run, but subsequent jobs don't depend upon any outputs from job_b)

on: push

jobs:
  job_a:
    runs-on: ubuntu-latest
    outputs:
      truthy_string: ${{ steps.a.outputs.always }}
      null_value: ${{ steps.b.outputs.never }}
    steps:
      - id: a
        run: echo "::set-output name=always::something"
      - id: b
        run: echo "We opt not to set any output at this time"
  job_b:
    runs-on: ubuntu-latest
    needs: job_a
    if: needs.job_a.outputs.null_value
    steps:
      - run: echo "We've ensured this job will be skipped"
  job_c:
    runs-on: ubuntu-latest
    needs: [job_a, job_b]
    if: needs.job_a.outputs.truthy_string
    steps:
      - run: echo "This won't run, even though the IF condition evaluates true."
  job_d:
    runs-on: ubuntu-latest
    needs: [job_a, job_b]
    if: always() && needs.job_a.outputs.truthy_string
    steps:
      - run: echo "This will run, even though we've only changed the condition from `true` to `true && true`"

Examining the output of this workflow, job_a will always run, job_b will always be skipped, job_c will always be skipped, and job_d will run. The only difference between job_c and job_d is the addition of always() && to the if condition.

Expected behavior If a job-level conditional evaluates to true, the job should run after all needs'd jobs have completed or been skipped. Both job_c and job_d should run. The always() && should not be required for job_c, since it doesn't change the ultimate true/false result of the conditional.

OR documentation should be updated to indicate this is expected behavior. The current docks simply says of job needs (emphasis added):

Identifies any jobs that must complete successfully before this job will run. It can be a string or array of strings. If a job fails, all jobs that need it are skipped unless the jobs use a conditional statement that causes the job to continue. This is relatively ambiguous, but the plain interpretation is that a conditional statement that evaluates to true should cause the job to continue. As seen with job_c in the sample, that isn't always the case.

Runner Version and Platform

Version of your runner? Unsure - I'm only running via Github Actions, not using a self-hosted runner.

OS of the machine running the runner? OSX/Windows/Linux/... Linux: ubuntu-latest

What's not working?

Jobs are being skipped even when the job-level conditional evaluates to true.

Job Log Output

Because the job is skipped entirely, there is no job-level output, even if the appropriate DEBUG secret is set.

Runner and Worker's Diagnostic Logs

As above, there is no additional output, even if the appropriate DEBUG secret is set.

whois-jon-peterson avatar May 19 '20 22:05 whois-jon-peterson

I think this meant to be a feature actually, but it's too subtle in my opinion - it left me scratching my head for an hour over why my workflow step wasn't running. From https://docs.github.com/en/actions/learn-github-actions/expressions#job-status-check-functions:

A default status check of success() is applied unless you include one of these functions.

I would have expected the default value of if: success() to be replaced with if: <your_condition>, but it's actually if: success() && <your_condition> unless your condition includes one of the status functions

MetRonnie avatar Jul 17 '20 13:07 MetRonnie

This problem is still present on version 2.273.1

t0rr3sp3dr0 avatar Sep 11 '20 21:09 t0rr3sp3dr0

Stumbled upon this while looking for a different issue, but thought I'd throw in my perspective on the matter.

needs implies a dependent relationship, so if one of the jobs doesn't run (job_b in this case) then logically it seems to me the dependent job should not run either by default, lest there be something missing from job_b's non-existent run. The fact that you can override this behavior by adding the always() check seems like a good feature; you have to explicitly say "This job does not directly depend on the previous job(s) success". I would not call the reported behavior a problem.

The only thing I think would be appropriate and appreciated is a note about this functionality in the docs for the workflow job if field (ie here) noting the relationship between if and needs, and potentially also with the job status check functions. There's a lot of areas in the docs I find where connections like this are completely overlooked, which makes it hard to get up to speed.

obermillerk avatar Oct 26 '20 15:10 obermillerk

Bah, and of course 5 minutes after I file the bug I see my PEBCAK -- I forgot the .outputs in the condition. works fine now, sorry for the noise! Please close this (I'm not allowed to do that myself)

martinpitt avatar Oct 30 '20 07:10 martinpitt

This also had me scratching my head for a very long time. It's completely counter-intuitive you have to use any of the status functions. Maybe this is from before the outcome and conclusion status fields were available?

It would hope this requirement can be removed, because it just doesn't really make any sense. Why does one have to do if: ${{always() && ....}} to make things work?

holm avatar May 22 '21 07:05 holm

I think the issue is that needs is the only way to sequence jobs within a workflow and this gives rise to two groups of use cases: the ones where people are using it just to sequence their jobs, and the ones using it for it's intended purpose of creating a dependent relationship.

Those in the first category feel it is illogical because they are using it simply to order their jobs, while those using it for the original intended purpose I would guess generally find it makes sense. A job that needs another job shouldn't run if that other job doesn't, it would likely be broken because of the dependent relationship between the two.

Perhaps some other form of sequencing should be introduced to alleviate this confusion, or as I suggested in my last comment just making this much more clear in the docs could probably do the same work.

obermillerk avatar May 22 '21 08:05 obermillerk

In a similar case, I had a conditional job Z I wanted to run at the end of all other jobs A, B, C. However, when any of A, B, or C were skipped, my Z job also got skipped. Adding always() did make it run, but even when the previous jobs failed, so I had to also check the results of my previous jobs:

jobs:
  ...
  Z:
    runs-on: ubuntu-latest
    needs: [A, B, C]
    if: |
      always() &&
      (needs.A.result == 'success' || needs.A.result == 'skipped') &&
      (needs.B.result == 'success' || needs.B.result == 'skipped') &&
      (needs.C.result == 'success' || needs.C.result == 'skipped') &&
      my_original_conditional()
    steps:
    - name: blah
      ...

Very verbose and I'm probably not using workflows as intended, but it works! 😅

andreykaipov avatar May 29 '21 19:05 andreykaipov

i also find this behavior burdensome and confusing. for example:

---
name: Test

on:
  - pull_request
  - push

jobs:
  jobA:
    name: This will always runs (JobA)
    runs-on: ubuntu-latest
    steps:
      - run: echo jobA
    outputs:
      runs: ${{ true }}

  jobB:
    name: This will be always skipped (JobB)
    needs:
      - jobA
    if: ${{ false }} 
    runs-on: ubuntu-latest
    steps:
      - run: env

  jobC:
    name: This will always run (JobC)
    needs: [jobA, jobB]
    if: always()
    runs-on: ubuntu-latest
    steps:
      - run: echo world
    outputs:
      runs: ${{ true }}

  jobD: # THIS DOESN'T RUN BECUASE IT JUST DEPENDS ON JOB C
    name: This should run if jobC runs (JobD)
    needs: [jobC]
    runs-on: ubuntu-latest
    steps:
      - run: env

  jobE:
    name: This should run if jobD runs (jobE)
    runs-on: ubuntu-latest
    needs:
      - jobD
    steps:
      - run: env

my expectation from working with DAG based systems is that a parent node should determine a child node directly. why does a child node have to know about grandparents in order to execute a task that depends ONLY ON THE PARENT running? can't see the logic here..

gad2103 avatar Jul 19 '21 21:07 gad2103

Idea 💡

What if there was a new field called follows (or trails or runs-after), which would would behave like this?

  1. Job C lists job A and job B under follows.
  2. Job A and job B run.
  3. If both A and B were either executed successfully or skipped, run C as well.
  4. If either A or B failed, do not run C.

This would be effectively equivalent to @andreykaipov's solution, but less verbose. Example:

jobs:
  ...
  C:
    runs-on: ubuntu-latest
    follows: [A, B]
    if: my_original_conditional()
    steps:
    - name: blah
      ...

PaulRBerg avatar Aug 17 '21 10:08 PaulRBerg

@paulrberg do you understand the reasoning behind why needs shouldn't just behave the way we're expecting it to? I'm all for adding a new keyword if it makes sense to preserve the functionality of the original one, but here it seems to me like needs should just always reference the node closest to it in the DAG and the DAG builder should be able to resolve chained dependencies.

I'm trying to think of if needs was built this way to prevent cycles but not coming up with anything that this specific setup ameliorates.

I'm happy to be shown the light though...

gad2103 avatar Aug 17 '21 13:08 gad2103

I think following example will produce expected behavior of original question.

job_d:
  runs-on: ubuntu-latest
  needs: [job_a, job_b]
  if: |
    always() &&
    !contains(needs.*.result, 'failure') &&
    !contains(needs.*.result, 'cancelled') &&
    needs.job_a.outputs.truthy_string
  steps:
    - run: echo "${{ toJSON(needs) }}"


mkungla avatar Aug 27 '21 13:08 mkungla

@gad2103 I think you're arguing a different issue here. This issue is about needs causing jobs not to run if any of the needs jobs are skipped. While I agree with your point that you shouldn't need to include grandparents in needs (if that is actually the case, doesn't seem like it should be), I think your complaint belongs in a separate issue.

obermillerk avatar Aug 27 '21 14:08 obermillerk

People expect a conditional to be based solely on the value that its predicate evaluates to. But here it also depends on the side-effect of whether or not you invoke any job status functions. This is a pretty confusing way to design a conditional predicate.

ef4 avatar Sep 01 '21 15:09 ef4

I've been bitten too and twice... because it's transitive. I expected a successful job will be enough to trigger the next job, but no, it depends on the sequence above. As soon as you have an optionnal job:

Always use always().

Also, I try to trick with success() instead of always(), since they are both job status check functions, but it's not enough.

crakmartin avatar Sep 23 '21 19:09 crakmartin

The contains trick from @mkungla doesn't do for me. A part of it is because I have 2 optionnal jobs, and I want to run another one, if either of them as run successfuly.

With:

  e2e-test:
    name: e2e-test
    runs-on: ubuntu-latest
    needs:
     - deploy_backend_staging
     - deploy_frontend_staging
    if: |
      always() &&
      contains(needs.*.result, 'success') &&
      !contains(needs.*.result, 'failure')

I got this:

image

And I guess the successful waiting participate to this wildcard thing. So, no wildcard for me :(

And to be explicit about my expection on the snapshot, I would not expect e2e-test to run since there is no success in deployment.

crakmartin avatar Sep 23 '21 20:09 crakmartin

@crakmartin You can add the jobids as parameter to success and failure (It's undocumented, but I found a reference in this repo then trial and error :))

Always use always().

Bad idea, you loose the ability to cancel your job.

!cancelled() is much saver.

You want something like this in e2e-test: !failure() && !cancelled() && (success('deploy_backend_staging') || success('deploy_frontend_staging'))

  • This won't run on failure of any job
  • This won't run on cancel
  • This won't run if no of the two jobs has status success (e.g. not cancelled && both jobs skipped, one failed etc.)

Here are two sample runs:

  • https://github.com/ChristopherHX/newcomposite-sample/actions/runs/1271420694
  • https://github.com/ChristopherHX/newcomposite-sample/actions/runs/1271424793

Some observations from my side

  • success and failure are allowing to add multiple jobids as parameter.
  • !success() && !failure() && !cancelled() a job was skipped, but none were failed, I might be wrong in case your job is abandoned
  • !success('test') && !failure('test') && !cancelled(), job with job id test was skipped, I might be wrong in case your job is abandoned
  • success('deploy_backend_staging', 'deploy_frontend_staging')=success('deploy_backend_staging') && success('deploy_frontend_staging')
  • failure('deploy_backend_staging', 'deploy_frontend_staging')=failure('deploy_backend_staging') || failure('deploy_frontend_staging')
  • success() (default if you don't use any status function), all previous jobs (via needs) of the current one needs status success (excluding skipped) and your workflow isn't cancelled
  • failure(), any previous jobs (via needs) of the current one needs status failure (excluding skipped) and your workflow isn't cancelled
  • !failure() && !cancelled() run if all previous jobs are succeeded or skipped, I might be wrong in case your job is abandoned

BTW I reimplemented this stuff in https://github.com/ChristopherHX/runner.server.

ChristopherHX avatar Sep 24 '21 21:09 ChristopherHX

Addition to @ChristopherHX comment. If your job has 2 dependencies e.g.

e2e-test:
  needs:
     - deploy_backend_staging
     - deploy_frontend_staging

where one may skipped and atleast one of these must succeed and you use something like.

if: |
  !failure() && !cancelled() && 
  (success('issue-deploy_backend_staging') || success('deploy_frontend_staging')) 

then jobs following e2e-test need if condition as well.

after-e2e-test:
  needs: e2e-test
  # This if condition must be added. 
  # Omitting this if statement results that your job will not run 
  # if  `e2e-test` is successful but one of  `deploy_backend_staging` or `deploy_frontend_staging` was skipped
  if: ${{ !failure() && !cancelled() }}

End of the day I still think that if: in actions has very broken design and is desperate need of good overhaul or supplementary api to make chaining actions really predictable and useful. Specially when it comes down to dependent jobs as some of the use cases in the comments here.

mkungla avatar Sep 25 '21 05:09 mkungla

same issue

zhangguanzhang avatar Dec 21 '21 14:12 zhangguanzhang

End of the day I still think that if: in actions has very broken design and is desperate need of good overhaul or supplementary api to make chaining actions really predictable and useful.

Totally agree, at first glance the behavior seem buggy. This largely contribute to the GHA unfrendliness in terms of complexity and ease of use.

marianhlavac avatar Jan 18 '22 10:01 marianhlavac

@TingluoHuang I also suffer from this problem. This issue is labeled "Service Bug", so can it interpreted as a bug, not a specification, and will eventually be resolved?

takanakahiko avatar Feb 25 '22 05:02 takanakahiko

Hello, it looks like I have pretty much the same issue described here https://github.community/t/continue-workflow-on-skipped-job/238682 and in https://github.com/ClickHouse/ClickHouse/pull/35321

We'd like to conditionally run a job without canceling the whole workflow, but it seems to not working:

  FastTest:
    needs: DockerHubPush
    if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip-fast-test') }}
    continue-on-error: ${{ contains(github.event.pull_request.labels.*.name, 'skip-fast-test') }}

Even when I've tried to add a subsequent job to continue the workflow, every following dependent job were skipped because of skipped grandparent

Felixoid avatar Mar 17 '22 13:03 Felixoid

This is still completely broken. How are we supposed to use the if condition on a job ? Does anyone at GitHub have any idea what a directed graph is? It should be trivial to implement as there is plenty of literature on the subject.

Here is a very simple example that doesn't work:

name: Build (Linux)

on:
  workflow_dispatch:
    inputs:
      run-tests:
        description: Run the tests?
        required: true
        default: false
        type: boolean
  workflow_call:
    inputs:
      run-tests:
        required: true
        default: false
        type: boolean

jobs:
  Linux:
    name: Linux
    runs-on: ubuntu-latest
    outputs:
      run-tests: ${{ steps.setup.outputs.run-tests }}
    steps:
      - name: Setup
        id: setup
        run: |
          ## note: this ugly syntax is because workflow_dispatch and workflow_call can't even agree on a common API
          echo "::set-output name=run-tests::${{ github.event.inputs.run-tests || inputs.run-tests }}"

  Linux-test:
    needs: Linux
    if: |
      always() &&
      !contains(needs.*.result, 'failure') &&
      !contains(needs.*.result, 'cancelled') &&
      needs.Linux.outputs.run-tests
    name: Linux Test
    runs-on: ubuntu-latest
    steps:
      - run: echo "${{ toJSON(needs) }}"

Job linux-test runs even though the workflow parameter run-tests is false, which can be verified by the outcome of the echo command:

{
  Linux: {
    result: success,
    outputs: {
      build-type: Debug,
      run-tests: false
    }
  }
}

image

Kryptos-FR avatar May 25 '22 09:05 Kryptos-FR

@Kryptos-FR you should do needs.Linux.outputs.run-tests == 'true' as the outputs are always strings.

This ticket is not about if not working properly but a confusing case about skipped jobs and the need to use always().

BYK avatar May 25 '22 09:05 BYK

Hi, I am using @andreykaipov 's solution https://github.com/actions/runner/issues/491#issuecomment-850884422 but running into a very weird behaviour (maybe a bug?) so, building upon his example, in case of a closing Job "ZZ" needs job "Z", that job "ZZ" is being skipped without plausible reason. Is anybody facing the same issue or does somebody have an idea why is that behaviour?

basically:

jobs:
  ...
  Z:
    runs-on: ubuntu-latest
    needs: [A, B]
    if: |
      always() &&
      (needs.A.result == 'success' || needs.A.result == 'skipped') &&
      (needs.B.result == 'success' || needs.B.result == 'skipped')
    steps:
    - name: foo
      uses: actions/checkout@v3
  ZZ:
    runs-on: ubuntu-latest
    needs: [Z]
    steps:
    - name: bar
      uses: foo

in this example, ZZ is being skipped and I don't get why. Any hints?

Thanks a lot!!

p.s. if I do

  ZZ:
    runs-on: ubuntu-latest
    needs: [Z]
    if: always()
    steps:
    - name: bar
      uses: foo

then yes, it will run, but that is not what I am pursuing here.

roventus avatar Jul 06 '22 19:07 roventus

In a similar case, I had a conditional job Z I wanted to run at the end of all other jobs A, B, C. However, when any of A, B, or C were skipped, my Z job also got skipped. Adding always() did make it run, but even when the previous jobs failed, so I had to also check the results of my previous jobs:

jobs:
  ...
  Z:
    runs-on: ubuntu-latest
    needs: [A, B, C]
    if: |
      always() &&
      (needs.A.result == 'success' || needs.A.result == 'skipped') &&
      (needs.B.result == 'success' || needs.B.result == 'skipped') &&
      (needs.C.result == 'success' || needs.C.result == 'skipped') &&
      my_original_conditional()
    steps:
    - name: blah
      ...

Very verbose and I'm probably not using workflows as intended, but it works! 😅

The cancel action is not working after applying this solution.

amirkhonov avatar Aug 15 '22 09:08 amirkhonov

In a similar case, I had a conditional job Z I wanted to run at the end of all other jobs A, B, C. However, when any of A, B, or C were skipped, my Z job also got skipped. Adding always() did make it run, but even when the previous jobs failed, so I had to also check the results of my previous jobs:

jobs:
  ...
  Z:
    runs-on: ubuntu-latest
    needs: [A, B, C]
    if: |
      always() &&
      (needs.A.result == 'success' || needs.A.result == 'skipped') &&
      (needs.B.result == 'success' || needs.B.result == 'skipped') &&
      (needs.C.result == 'success' || needs.C.result == 'skipped') &&
      my_original_conditional()
    steps:
    - name: blah
      ...

Very verbose and I'm probably not using workflows as intended, but it works! 😅

The problem with this is there are so many always() in the code.

vinayakkulkarni avatar Aug 15 '22 09:08 vinayakkulkarni

Just run into the same issue. Would be nice to have a better solution here.

I was kind of surprised about the overall behaviour.

However, the issue was somehow solvable with the suggestions from the comments here.

ManuelRauber avatar Sep 26 '22 08:09 ManuelRauber

@BYK @martinpitt @holm I also come across this issue, jobs no "outputs" with "if" condition after I change "set-out" command to ">> $GITHIUB_OUTPUT" Screen Shot 2022-10-17 at 9 47 04 am Screen Shot 2022-10-17 at 9 47 11 am

christina-30 avatar Oct 14 '22 05:10 christina-30

I used the following solution to avoid skipping other dependent jobs:


on:
  workflow_dispatch:
    inputs:
      runFirstJob:
        description: 'Whether to launch a first job'
        required: true
        default: true
        type: choice
        options:
        - true
        - false
   
  first_job:
    name: First job
    runs-on: self-hosted
    steps:
      - uses: actions/checkout@v2
      - name: Check runFirstJob input
        id: runFirstJob
        run: |
          if [ '${{ inputs.runFirstJob }}' == 'true' ]; then
            echo "::set-output name=run::true"
          fi
        shell: bash
      - name: Run First Job
        if: ${{ steps.runFirstJob.outputs.run == 'true' }}
        uses: <COMPOSITE_ACTION_LINK>
  second_job:
    name: Second Job
    needs: first_job
    runs-on: self-hosted
    steps:
      - uses: actions/checkout@v2
      - name: Run Second Job
        run: |
          pwsh ./.github/scripts/script.ps1

amirkhonov avatar Oct 14 '22 08:10 amirkhonov

In a similar case, I had a conditional job Z I wanted to run at the end of all other jobs A, B, C. However, when any of A, B, or C were skipped, my Z job also got skipped. Adding always() did make it run, but even when the previous jobs failed, so I had to also check the results of my previous jobs:

jobs:
  ...
  Z:
    runs-on: ubuntu-latest
    needs: [A, B, C]
    if: |
      always() &&
      (needs.A.result == 'success' || needs.A.result == 'skipped') &&
      (needs.B.result == 'success' || needs.B.result == 'skipped') &&
      (needs.C.result == 'success' || needs.C.result == 'skipped') &&
      my_original_conditional()
    steps:
    - name: blah
      ...

Very verbose and I'm probably not using workflows as intended, but it works! 😅

The cancel action is not working after applying this solution.

Experiencing the same. I am unable to cancel my jobs with such if conditions.

igoralveslima avatar Nov 23 '22 14:11 igoralveslima