ci: skip Github CI on branch pushes for forks

Sjors commented at 3:39 pm on July 19, 2024: member

When a contributor maintains a fork of the repo, any pull request they make to their own fork, or to the main repository, will trigger two CI runs one for the branch push and one for the pull request.

After this PR when SKIP_BRANCH_PUSH is set, pushes made to git branches inside fork repositories will no longer trigger CI runs, unless the git branches are associated with PRs in the fork repository, or the main repository.

The same behaviour was added for Cirrus in e9bfbb5414ab14ca14d8edcfdf77f28c9ed67c33.

Note to maintainers: SKIP_BRANCH_PUSH=true needs to be set for the GUI repo to maintain existing behaviour.

DrahtBot commented at 3:39 pm on July 19, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30487.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, m3dwards, achow101
Stale ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Jul 19, 2024

ryanofsky commented at 3:32 pm on July 24, 2024: contributor

Mentioning the history of this change in the PR description is helpful, but it would be clearer if it said what the change is and what effects it has.

From code comments in this PR and #29274 (comment), it seems like the main intended effect of this change is to prevent github tasks from running twice in PRs and causing the following output:

But in the last thread where this change was discussed (https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457587805) a potential concern was that it would cause CI not to run on branch pushes within forks like Inquisition and Knots.

This seems like it could be a real problem, though maybe it is not a big deal for the forks to modify this file if they want CI to run on branches they are modifying anyway.

DrahtBot added the label CI failed on Aug 5, 2024

Sjors commented at 10:59 am on August 8, 2024: member

@ryanofsky I updated the description.

In the earlier issue both @luke-jr (Knots) and @ajtowns (Inquisition) seemed to indicate that the change wouldn’t be a problem for them. See #29274 (comment). Let me know if that’s incorrect or if you changed your mind.

not a big deal for the forks to modify this file if they want CI to run on branches they are modifying anyway

So far it seems I’m the only one using this and it’s not bothering any other project. But if we end up with some forks that need to modify this file and others that don’t, then I don’t think this PR should be merged. The extra runs are annoying, but if needed I can wait for a glorious future in which Github CI gets their act together.

DrahtBot removed the label CI failed on Aug 9, 2024

ryanofsky approved

ryanofsky commented at 3:41 pm on August 16, 2024: contributor

Code review ACK 3067cd55f0fc79a7fa6342ca0600472cabcf2690.

Thanks for the explanation, that all sounds good to me.

The new description is definitely better than the original but it is not actually saying what behavior will be after this PR. Would be helpful to say something like “After this PR, pushes made to git branches inside fork repositories will no longer trigger CI runs, unless the git branches are associated with PRs in the fork repository, or the main repository.” (assuming that is correct)

Sjors commented at 10:19 am on August 17, 2024: member

Thanks, added that (correct) sentence to the description.

DrahtBot added the label CI failed on Sep 10, 2024

DrahtBot removed the label CI failed on Sep 14, 2024

achow101 requested review from vasild on Oct 15, 2024

achow101 requested review from achow101 on Oct 15, 2024

achow101 requested review from hebasto on Oct 15, 2024

willcl-ark requested review from m3dwards on Oct 15, 2024

willcl-ark requested review from willcl-ark on Oct 15, 2024

Sjors commented at 8:31 am on October 16, 2024: member

A use case that was mentioned to me offline is that some people, before opening a PR to the main repo, like to push their branch and have CI check it. They then wait for CI on the branch to pass before opening a new PR. Their main reason for this is to reduce noise on the main repo.

It seems this PR breaks that use case. I suggest we reconsider it if and when Github adds support for custom ENV variables on a fork, like Cirrus does.

Sjors closed this on Oct 16, 2024

willcl-ark commented at 8:44 am on October 16, 2024: member

From https://github.com/bitcoin/bitcoin/commit/e9bfbb5414ab14ca14d8edcfdf77f28c9ed67c33:

When a contributor maintains a fork of the repo, any pull request they make to their own fork, or to the main repository, will trigger two CI runs: one for the branch push and one for the pull request. This can be avoided by setting SKIP_BRANCH_PUSH=true as a custom env variable in Cirrus repository settings, accessible from

GitHub supports setting custom env vars in each repo like Cirrus: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#defining-configuration-variables-for-multiple-workflows

Sjors commented at 10:59 am on October 16, 2024: member

Oh wow that is remarkably well hidden! In that case I will change this PR to apply the same behaviour as Cirrus.

Sjors reopened this on Oct 16, 2024

Sjors marked this as a draft on Oct 16, 2024

vasild commented at 10:58 am on November 5, 2024: contributor

Concept ACK

A use case that was mentioned to me offline is that some people, before opening a PR to the main repo, like to push their branch and have CI check it.

FWIW I do that but I have a dedicated dummy branch which is associated with a PR to merge it into my copy of master. So I never merge that PR and keep push -fing into the dummy branch to test.

Sjors force-pushed on Nov 10, 2024

Sjors commented at 6:15 pm on November 10, 2024: member

Github documentation is pretty terrible. The change here should work, but the jobs are not skipped in https://github.com/Sjors/bitcoin/pull/70

See e.g. https://github.com/orgs/community/discussions/42133#discussioncomment-4501954

m3dwards commented at 9:48 pm on November 11, 2024: contributor

Pretty sure this is what you are looking for:

 0diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
 1index cc1d8dd905..131f350111 100644
 2--- a/.github/workflows/ci.yml
 3+++ b/.github/workflows/ci.yml
 4@@ -85,7 +85,7 @@ jobs:
 5     # one for the branch push and one for the pull request.
 6     # This can be avoided by setting SKIP_BRANCH_PUSH=true as a custom env variable
 7     # in Github repository settings.
 8-    if: ${{ vars.SKIP_BRANCH_PUSH != 'true' }} || github.event_name == 'pull_request'
 9+    if: ${{ vars.SKIP_BRANCH_PUSH != 'true' || github.event_name == 'pull_request' }}
10
11     timeout-minutes: 120
12
13@@ -139,7 +139,7 @@ jobs:
14     # See: https://github.com/actions/runner-images#available-images.
15     runs-on: windows-2022
16
17-    if: ${{ vars.SKIP_BRANCH_PUSH != 'true' }} || github.event_name == 'pull_request'
18+    if: ${{ vars.SKIP_BRANCH_PUSH != 'true' || github.event_name == 'pull_request' }}
19
20     env:
21       PYTHONUTF8: 1
22@@ -218,7 +218,7 @@ jobs:
23   asan-lsan-ubsan-integer-no-depends-usdt:
24     name: 'ASan + LSan + UBSan + integer, no depends, USDT'
25     runs-on: ubuntu-24.04 # has to match container in ci/test/00_setup_env_native_asan.sh for tracing tools
26-    if: ${{ vars.SKIP_BRANCH_PUSH != 'true' }} || github.event_name == 'pull_request'
27+    if: ${{ vars.SKIP_BRANCH_PUSH != 'true' || github.event_name == 'pull_request' }}
28     timeout-minutes: 120
29     env:
30       FILE_ENV: "./ci/test/00_setup_env_native_asan.sh"

Sjors commented at 11:08 am on November 12, 2024: member

@m3dwards thanks! I’ll give that a try. Any idea what was wrong with the syntax I used?

ci: skip Github CI on branch pushes for forks

Consistent with Cirrus behavior introduced in e9bfbb5414ab14ca14d8edcfdf77f28c9ed67c33.

8610bcef9d

Sjors force-pushed on Nov 12, 2024

Sjors marked this as ready for review on Nov 12, 2024

Sjors commented at 11:19 am on November 12, 2024: member

That worked! See https://github.com/Sjors/bitcoin/pull/70/checks where the “push” actions are skipped.

m3dwards commented at 2:15 pm on November 12, 2024: contributor

Any idea what was wrong with the syntax I used?

The || github.event_name == 'pull_request' is code that needs to be executed rather than static data so it needs to be inside the double curlies. I believe the hardcoded string of || github.event_name == 'pull_request' is truthy hence it would run the job. Github Actions is finicky!

vasild approved

vasild commented at 11:23 am on November 13, 2024: contributor

ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66

DrahtBot requested review from ryanofsky on Nov 13, 2024

m3dwards commented at 3:20 pm on November 13, 2024: contributor

ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66

m3dwards approved

Sjors commented at 3:41 pm on November 13, 2024: member

Just repeating my “note to maintainers” from the description: like with Cirrus, SKIP_BRANCH_PUSH=true needs to be set for the GUI repo to maintain existing behaviour.

Probably here.

achow101 commented at 9:30 pm on November 14, 2024: member

ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66

achow101 merged this on Nov 14, 2024

achow101 closed this on Nov 14, 2024

ryanofsky commented at 3:08 am on November 15, 2024: contributor

Just repeating my “note to maintainers” from the description: like with Cirrus, SKIP_BRANCH_PUSH=true needs to be set for the GUI repo to maintain existing behaviour.

Probably here.

Can confirm this was set at the time this PR was merged.

Sjors deleted the branch on Nov 15, 2024

ci: skip Github CI on branch pushes for forks #30487

Code Coverage & Benchmarks

Reviews

Conflicts