Fixes the CI if condition

[mcakircali]

No description provided.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.91%. Comparing base (9fac682) to head (addcd8b).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #183   +/-   ##
========================================
  Coverage    64.91%   64.91%           
========================================
  Files         1107     1107           
  Lines        56489    56489           
  Branches      4247     4247           
========================================
  Hits         36669    36669           
  Misses       19820    19820           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Private downstream CI failed.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14083413580.

[github-actions]

Private downstream CI failed.
Workflow name: private-downstream-ci-hpc
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14085058936.

[pgeier]

that's what we have in other downstreams as well.

Just started to wonder about the reason for having success() || failure() ... seems to always happen, or are there other states like skipped() ?

[mcakircali]

that's what we have in other downstreams as well.

Just started to wonder about the reason for having success() || failure() ... seems to always happen, or are there other states like skipped() ?

I think it's cancelled. all our repos have it outside the "${{" but not sure which form is correct. or maybe we need !cancelled() .. I don't know

[github-actions]

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14263248192.

[github-actions]

Private downstream CI succeeded.
Workflow name: private-downstream-ci
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/14263248192.

[Ozaq]

The change is not correct as far as I can see:
inside the conditional you can either use literals or expressions not mixed.

[Ozaq]

@mcakircali Do you want to pick this up again or should I close it?

[mcakircali]

The change is not correct as far as I can see: inside the conditional you can either use literals or expressions not mixed.

can you attach a reference for that statement?

also, have you looked at our other repos and compare?

I believe there was an issue in the past that lead to fix (see our other repos), it's best to ask Jakub or Dusan to review it.

[Ozaq]

@mcakircali If you look at: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idif

It is stated that ${{ }} can be omitted in this case because the whole condition will be evaluated as expression.

See the respective discussion here: https://github.com/orgs/community/discussions/27152

No ${{ ... }} at all should work, too. But using it for parts of the if value will always create a non-empty string, which is always true.

[recmanj]

While this works I can understand it might be confusing. It is technically possible to omit ${{ }} syntax but mixing it would not be my recommendation - it is not explicitly mentioned in GitHub docs but it would make it consistent.

The mixed syntax is currently being used in multiple ECMWF repos so I suggest to update those too along with the documentation.

[recmanj]

Suggested change

	if: (success() || failure()) && ${{ !github.event.pull_request.head.repo.fork && github.event.action != 'labeled' || github.event.label.name == 'approved-for-ci' }}
	if: ${{ (success() || failure()) && (!github.event.pull_request.head.repo.fork && github.event.action != 'labeled' || github.event.label.name == 'approved-for-ci') }}

[Ozaq]

This is just undoing the change from the PR, isn't it?

[recmanj]

Not exactly. Previously, if PR was coming from a fork, the expression would be evaluated as false every time. The change allows PRs from fork to run the CI if it's labeled as approved-for-ci.

[mcakircali]

While this works I can understand it might be confusing. It is technically possible to omit ${{ }} syntax but mixing it would not be my recommendation - it is not explicitly mentioned in GitHub docs but it would make it consistent.

The mixed syntax is currently being used in multiple ECMWF repos so I suggest to update those too along with the documentation.

as I mentioned in previous comment, previously we did not have the mixed syntax in our repos. The mixed syntax came as a fix from Dusan (I believe), hence I created this PR in eckit because it was not same as other repos.

I committed your suggestion to this PR (which also fixes the scope) and if it works, we can merge this and align other repos too, so that they are also not mixed syntax.

[Ozaq]

This does not yet address the issue fully. The private and non-private downstream jobs are not running for PRs from forks.

Looking at the other conditions:
private-downstream depends on downstream but downstream is never run for forks, and is thus skipped. The depending job for private-downstream will never run because success() || failure() id fall if downstream is skipped.

[recmanj]

This does not yet address the issue fully. The private and non-private downstream jobs are not running for PRs from forks.

Looking at the other conditions: private-downstream depends on downstream but downstream is never run for forks, and is thus skipped. The depending job for private-downstream will never run because success() || failure() id fall if downstream is skipped.

Yes you are right, the fix should be applied to dowstream-ci too (as correctly presented in docs).

eckit/.github/workflows/ci.yml

Line 26 in 59a51fc

if: ${{ !github.event.pull_request.head.repo.fork && (github.event.action != 'labeled' || github.event.label.name == 'approved-for-ci') }}

eckit/.github/workflows/ci.yml

Line 57 in 59a51fc

if: ${{ !github.event.pull_request.head.repo.fork && (github.event.action != 'labeled' || github.event.label.name == 'approved-for-ci') }}

->

${{ !github.event.pull_request.head.repo.fork && github.event.action != 'labeled' || github.event.label.name == 'approved-for-ci' }}

[mcakircali]

FYI, (don't ask me why) but I see that the tag is removed by bot:

[github-actions]

Private downstream CI failed.
Workflow name: private-downstream-ci-hpc
View the logs at https://github.com/ecmwf/private-downstream-ci/actions/runs/15876941752.