ci: respect the [skip ci] convention in our GitHub workflow "CI/PR"

It was mentioned to me that it might not be totally helpful to run all the builds whenever an in-progress branch is pushed to GitHub. For example, if a contributor has dozens of topic branches in flight, they might not want to have all of them built whenever a new end-of-day push happens.

Quoted 5 lines · Johannes Schindelin via GitGitGadget

It was mentioned to me that it might not be totally helpful to run all the builds whenever an in-progress branch is pushed to GitHub. For example, if a contributor has dozens of topic branches in flight, they might not want to have all of them built whenever a new end-of-day push happens.

As I was probably (one of) the mentioners here, thank you for looking into this. This definitely _helps_, and if you don't want to go further I could certainly make do with it[1].

But here are some thoughts on what a more ideal solution would look like (to me, anyway).

What I'd _most_ like is a separately-maintained list of branches (or branch patterns) on which to run CI. I mostly care about just testing my personal equivalent of "next" (which is really "next" plus my stable topics), and I'd probably only list that branch. Plus potentially a special CI branch that I'd use when chasing down a failure that I can't reproduce locally.

But I don't think there's any good way to implement that via GitHub Actions. I thought perhaps we could pull data from a different branch in the same repo, but referring to external "Actions" seems to require a full repo name (and obviously putting git/git in that name doesn't help anybody who's trying to override something in a fork).

Another alternative: could we trigger CI based on branch-names? Locally, I call my unstable branches "jk/something-wip", and I'd like to skip all of the "-wip" branches. That's basically equivalent to "[skip ci]" in that I could just amend the tip commit of the -wip branches to say "skip ci". But what if we flipped the default to _not_ build? I.e.:

- continue to build all pull requests; if you opened one, you're serious enough to have other people look and should get CI feedback

- build branches with a few well-known names or patterns. Maybe names like "master"? Or maybe "refs/heads/build-ci/*"? Or maybe anything in "refs/heads/<initials>/*"? We'd have to decide on a convention as a community.

This is my proposal for it:
[...]

.github/workflows/main.yml +4 -1

index fd4df939b5..303393ba73 100644
@@ -1,6 +1,19 @@
 name: CI/PR
 
-on: [push, pull_request]
+on:
+  pull_request:
+    branches:
+      - '*'

Yeah, this seems quite reasonable to me. I'd just add a refspec to push a duplicate of branches I'm interested in running CI on to for-ci.

.github/workflows/main.yml line 6

Quoted 12 lines · Đoàn Trần Công Danh

.github/workflows/main.yml +4 -1

index fd4df939b5..ea43b03092 100644
@@ -1,6 +1,18 @@
 name: CI/PR
 
-on: [push, pull_request]
+on:
+  pull_request:
+    branches:
+      - '**'

Doing "**" here makes sense to catch everything (it would be even better if we could just say "everything with a pull request" by omitting the branch filter entirely, but maybe that's not possible).

.github/workflows/main.yml line 15

Quoted 2 lines

.github/workflows/main.yml +2 -0

+    tags:
+      - '*'

Would we want that here, too? I guess nobody is likely to push "foo/v1.2.3".

Or on the flip side, would we want to tighten this? If I push a tag "wip", I probably don't want it built. Probably the right rule is "annotated tags only", but I suspect that's not possible.

.github/workflows/main.yml line 13

Quoted 7 lines

.github/workflows/main.yml +7 -0

+  push:
+    branches:
+      - maint
+      - master
+      - next
+      - jch
+      - pu

What happened to "for-ci" (presumably "for-ci/**")?

-Peff

Huh; I'm not sure that I'm sold on the idea of a 'for-ci' namespace here. In addition to running 'make test' on patches locally before I send them, I find it tremendously convenient for GitHub to run them for me when I push 'tb/' branches up to 'ttaylorr/git'.

So, while the above is more-or-less what I'd expect the monitored list of branches to look like (at least, ignoring the missing 'for-ci/**' bits), I wish that I could also build every branch that I push up to my fork.

Of course, I don't want to maintain a one-patch difference between ttaylorr/git@master and git/git@master, so I wonder if we could get a little more creative with these rules and actually run Actions on *every* branch, but introduce a new first step which stops the rest of the actions run (so that in practice we're not running CI on non-integration branches in Junio's tree).

Quoted 10 lines · Taylor Blau

Huh; I'm not sure that I'm sold on the idea of a 'for-ci' namespace here. In addition to running 'make test' on patches locally before I send them, I find it tremendously convenient for GitHub to run them for me when I push 'tb/' branches up to 'ttaylorr/git'.

So, while the above is more-or-less what I'd expect the monitored list of branches to look like (at least, ignoring the missing 'for-ci/**' bits), I wish that I could also build every branch that I push up to my fork.

Quoted 6 lines

Of course, I don't want to maintain a one-patch difference between ttaylorr/git@master and git/git@master, so I wonder if we could get a little more creative with these rules and actually run Actions on *every* branch, but introduce a new first step which stops the rest of the actions run (so that in practice we're not running CI on non-integration branches in Junio's tree).

Hmph, what are we trying to avoid by using the for-ci/ convention?

If this is only a reaction to what I said earlier (i.e. "building everything in github.com/gitster/git/ has no value to me"), then I suspect it may be an over-engineered solution to a problem that does not exist, and harms people like you. I could just go there and turn off GitHub Actions for that repository instead.

Or are there more issues being addressed with the "testing branches are opt-in, unless a pull request against git/git explicitly says it is ready to be tested" approach?

Quoted 5 lines

If this is only a reaction to what I said earlier (i.e. "building everything in github.com/gitster/git/ has no value to me"), then I suspect it may be an over-engineered solution to a problem that does not exist, and harms people like you. I could just go there and turn off GitHub Actions for that repository instead.

It is a reaction to that, yes. It would be nice to have a middle-ground where you can run Actions on the official integration branches, but contributors such as myself run Actions on *every* branch of their fork.

Quoted 6 lines · Taylor Blau

Quoted 5 lines

If this is only a reaction to what I said earlier (i.e. "building everything in github.com/gitster/git/ has no value to me"), then I suspect it may be an over-engineered solution to a problem that does not exist, and harms people like you. I could just go there and turn off GitHub Actions for that repository instead.

Quoted 3 lines

It is a reaction to that, yes. It would be nice to have a middle-ground where you can run Actions on the official integration branches, but contributors such as myself run Actions on *every* branch of their fork.

The problem is that there is no way for people to _not_ run on every branch of their fork, then. It is either every branch or no branches.

.... I don't like it, but I don't see any other way to let people choose some branches but not others (that doesn't involve a one-line yaml change at the beginning of every branch, which is an even higher burden).

Huh; I'm not sure that I'm sold on the idea of a 'for-ci' namespace here. In addition to running 'make test' on patches locally before I send them, I find it tremendously convenient for GitHub to run them for me when I push 'tb/' branches up to 'ttaylorr/git'.

So, while the above is more-or-less what I'd expect the monitored list of branches to look like (at least, ignoring the missing 'for-ci/**' bits), I wish that I could also build every branch that I push up to my fork.

Of course, I don't want to maintain a one-patch difference between ttaylorr/git@master and git/git@master, so I wonder if we could get a little more creative with these rules and actually run Actions on *every* branch, but introduce a new first step which stops the rest of the actions run (so that in practice we're not running CI on non-integration branches in Junio's tree).

Quoted 10 lines · Taylor Blau

Huh; I'm not sure that I'm sold on the idea of a 'for-ci' namespace here. In addition to running 'make test' on patches locally before I send them, I find it tremendously convenient for GitHub to run them for me when I push 'tb/' branches up to 'ttaylorr/git'.

So, while the above is more-or-less what I'd expect the monitored list of branches to look like (at least, ignoring the missing 'for-ci/**' bits), I wish that I could also build every branch that I push up to my fork.

Quoted 6 lines

Of course, I don't want to maintain a one-patch difference between ttaylorr/git@master and git/git@master, so I wonder if we could get a little more creative with these rules and actually run Actions on *every* branch, but introduce a new first step which stops the rest of the actions run (so that in practice we're not running CI on non-integration branches in Junio's tree).

I don't understand what that would accomplish. If we ran the actions on every branch but stopped the run, then you wouldn't get the CI results you want. What am I missing?

-Peff

Quoted 7 lines

Quoted 6 lines

Of course, I don't want to maintain a one-patch difference between ttaylorr/git@master and git/git@master, so I wonder if we could get a little more creative with these rules and actually run Actions on *every* branch, but introduce a new first step which stops the rest of the actions run (so that in practice we're not running CI on non-integration branches in Junio's tree).

Quoted 3 lines

I don't understand what that would accomplish. If we ran the actions on every branch but stopped the run, then you wouldn't get the CI results you want. What am I missing?

That on forks of git/git we *would't* stop the run for non-integration branches, i.e., that we'd have something like:

* Actions is running on all branches, of all forks, all the time.

But that is missing the point of the exercise, no? The question of what gitster/git should do was a side conversation. The purpose of Dscho's original patch and Danh's followup was to allow anybody to choose which branches in their own fork. I.e.:

Quoted 1 line

* Actions is running on all branches, of all forks, all the time.

This is how it works now, and is the exact thing we are trying to fix.

Would it be too much hassle to use notes for a thing like this? Perhaps push out with refs/notes/skip-ci note attached to a commit you do not want to be built? I have a feeling that it gives way overkill flexibility with little gain (probably too cumbersome to manage).

Does push into GitHub repository offer an ability to pass arbitrary push option, to which actions that trigger "on: push" event can react?

But _if_ we can read from other refs in the repository, I would be very happy if we parsed config out of refs/ci/branches or something. It feels like that's something that ought to be possible, but I haven't quite figured out a way to do it.

Quoted 4 lines · Jeff King

But _if_ we can read from other refs in the repository, I would be very happy if we parsed config out of refs/ci/branches or something. It feels like that's something that ought to be possible, but I haven't quite figured out a way to do it.

OK, I finally figured this out. The result is the patch below, which I think should make everybody happy. Or at least has the ability to do so if they're willing to push a config ref. ;)

Subject: [PATCH] ci: allow per-branch config for GitHub Actions

Depending on the workflows of individual developers, it can either be convenient or annoying that our GitHub Actions CI jobs are run on every branch. As an example of annoying: if you carry many half-finished work-in-progress branches and rebase them frequently against master, you'd get tons of failure reports that aren't interesting (not to mention the wasted CPU).

This commit adds a new job which checks a special ref within the repository for CI config, and (optionally) causes all of the other jobs to be skipped.

Right now the logic is to run CI for all branches by default, unless a whitelist exists, in which case the branch must be mentioned there (using its fully qualified ref name).

We could easily add in a
blacklist, as well.

Or since we're running a shell in a VM, we really
could just run "./allow-ref $refname" and let individual forks specify whatever shell code they like.

After writing that, I think we probably ought to just do the allow-ref thing from the start, and skip this whitelist logic. Then we should never need to change this workflow file again. People can implement whatever weird custom logic they want to.

jobs:

.github/workflows/main.yml +16 -0

+  check-ci:
+      runs-on: ubuntu-latest
+      outputs:
+        enabled: ${{ steps.check-ref.outputs.enabled }}
+      steps:
+        - uses: actions/checkout@v2
+          continue-on-error: true
+          with:
+            ref: refs/ci/config
+        - id: check-ref
+          name: check whether CI is enabled for ref
+          run: |
+            enabled=yes
+            if test -e ref-whitelist &&
+               ! grep '^${{ github.ref }}$' ref-whitelist
+            then

Quoted 3 lines

This commit adds a new job which checks a special ref within the repository for CI config, and (optionally) causes all of the other jobs to be skipped.

Nice---that way, all existing jobs do not even need to know about the special controlling ref.

Quoted 3 lines

Right now the logic is to run CI for all branches by default, unless a whitelist exists, in which case the branch must be mentioned there (using its fully qualified ref name).

So there is no wildcard? Not really complaining, but am wondering.

Quoted 3 lines

Or since we're running a shell in a VM, we really
could just run "./allow-ref $refname" and let individual forks specify whatever shell code they like.

I presume that you are saying "checking out the tree of refs/ci/config and there is a program allow-ref that can tell which one to run ci on"?

Quoted 6 lines

Quoted 3 lines

Or since we're running a shell in a VM, we really
could just run "./allow-ref $refname" and let individual forks specify whatever shell code they like.

I presume that you are saying "checking out the tree of refs/ci/config and there is a program allow-ref that can tell which one to run ci on"?

Yes, exactly.

Quoted 17 lines · Jeff King

jobs:

.github/workflows/main.yml +16 -0

+  check-ci:
+      runs-on: ubuntu-latest
+      outputs:
+        enabled: ${{ steps.check-ref.outputs.enabled }}
+      steps:
+        - uses: actions/checkout@v2
+          continue-on-error: true
+          with:
+            ref: refs/ci/config
+        - id: check-ref
+          name: check whether CI is enabled for ref
+          run: |
+            enabled=yes
+            if test -e ref-whitelist &&
+               ! grep '^${{ github.ref }}$' ref-whitelist
+            then

.github/workflows/main.yml env: · line 34

Quoted 10 lines

.github/workflows/main.yml +6 -0

+              enabled=no
+            fi
+            echo "::set-output name=enabled::$enabled"
+
   windows-build:
+    needs: check-ci
+    if: needs.check-ci.outputs.enabled == 'yes'
     runs-on: windows-latest
     steps:
     - uses: actions/checkout@v1

Oh, quite nice. Simple and clean.

The idea is indeed very neat. I think we can do a bit better with resource usage by not even bothering to check this branch out. Something along those lines (sorry, I really would love to have the time to test this...):

      - id: check-ref
        name: check whether CI is enabled for ref
        uses: actions/[email protected]
        with:
          script: |
            const req = {
              owner: context.repo.owner,
              repo: context.repo.repo,
              ref: "ci/config"
            };

            try {
              req.tree_sha = (await github.git.getRef(req)).data.object.sha;
              (await github.git.getTree(req))
              .tree.filter(e => e.path == 'ref-whitelist').map(e => {
                req.file_sha = e.sha;
              });
              const list = Buffer.from((await github.git.getBlob(req)).data.content, 'base64').toString('UTF-8');
              core.setOutput('enabled', `\n${list}`.indexOf(`\n${{github.ref}}\n`) < 0 ? 'no' : 'yes');
            } catch (e) {
              core.setOutput('enabled', 'yes');
            }

Ciao,
Dscho

Quoted 3 lines · Johannes Schindelin

The idea is indeed very neat. I think we can do a bit better with resource usage by not even bothering to check this branch out. Something along those lines (sorry, I really would love to have the time to test this...):

Nice. I view the latest round of Peff's idea "allow-ref" as "because we are spinning a VM anyway, why not just run an end-user supplied script and let it decide?" and not as "we must have a Turing complete flexibility so let's run a script in a VM". In other words, it may be overkill and we may strike a better tradeoff by living with reduced flexibility if there is a way to avoid the associated cost.

But doesn't this (i.e. uses: actions/github-script) still pay the cost of spinning up a VM? How expensive is it to check out a small tree with a single file, whether it is ref-whitelist or allow-ref?

Quoted 3 lines

But doesn't this (i.e. uses: actions/github-script) still pay the cost of spinning up a VM? How expensive is it to check out a small tree with a single file, whether it is ref-whitelist or allow-ref?

I suspect this script mechanism may be much cheaper. I don't know the implementation details, but spinning up a nodejs container to run a javascript snippet should be much cheaper than a full ubuntu VM running "git clone" (the clone itself should be super cheap because it's a shallow single-branch clone of a tree with one file in it, but getting there is relatively heavy-weight).

Quoted 10 lines · Jeff King

Quoted 3 lines

But doesn't this (i.e. uses: actions/github-script) still pay the cost of spinning up a VM? How expensive is it to check out a small tree with a single file, whether it is ref-whitelist or allow-ref?

I suspect this script mechanism may be much cheaper. I don't know the implementation details, but spinning up a nodejs container to run a javascript snippet should be much cheaper than a full ubuntu VM running "git clone" (the clone itself should be super cheap because it's a shallow single-branch clone of a tree with one file in it, but getting there is relatively heavy-weight).

Sorry, this is all complete nonsense. There is no magical nodejs container in Actions. You still have to say "runs-on: ubuntu-latest". So it's still spinning up that VM and then running inside there.

and they took 1, 2, and 3 seconds respectively. They spend 2s getting the environment set up and the actions loaded. So the API one spent less than 1s on the network, but the single-file checkout spent slightly more. Given the timing variations I've seen, I wouldn't be surprised if it sometimes goes the other way. But even if those numbers are accurate, I don't think the cost difference is enough to force our hand either way.

The idea is indeed very neat. I think we can do a bit better with resource usage by not even bothering to check this branch out. Something along those lines (sorry, I really would love to have the time to test this...):

      - id: check-ref
        name: check whether CI is enabled for ref
        uses: actions/[email protected]
        with:
          script: |
            const req = {
              owner: context.repo.owner,
              repo: context.repo.repo,
              ref: "ci/config"
            };

            try {
              req.tree_sha = (await github.git.getRef(req)).data.object.sha;
              (await github.git.getTree(req))
              .tree.filter(e => e.path == 'ref-whitelist').map(e => {
                req.file_sha = e.sha;
              });
              const list = Buffer.from((await github.git.getBlob(req)).data.content, 'base64').toString('UTF-8');
              core.setOutput('enabled', `\n${list}`.indexOf(`\n${{github.ref}}\n`) < 0 ? 'no' : 'yes');

Quoted 3 lines · Johannes Schindelin

The idea is indeed very neat. I think we can do a bit better with resource usage by not even bothering to check this branch out. Something along those lines (sorry, I really would love to have the time to test this...):

While this can avoid the cost of checking out a whole branch (which can be mitigated by using an orphan branch with single file),

This still spins up an VM, and actions/github-script run (I think) nodejs, which is more resource intensive than git and sh script. Above statement maybe wrong, I'm not interacting much with nodejs.

Quoted 11 lines

      - id: check-ref
        name: check whether CI is enabled for ref
        uses: actions/[email protected]
        with:
          script: |
            const req = {
              owner: context.repo.owner,
              repo: context.repo.repo,
              ref: "ci/config"
            };

Quoted 8 lines

            try {
              req.tree_sha = (await github.git.getRef(req)).data.object.sha;
              (await github.git.getTree(req))
              .tree.filter(e => e.path == 'ref-whitelist').map(e => {
                req.file_sha = e.sha;
              });
              const list = Buffer.from((await github.git.getBlob(req)).data.content, 'base64').toString('UTF-8');
              core.setOutput('enabled', `\n${list}`.indexOf(`\n${{github.ref}}\n`) < 0 ? 'no' : 'yes');

And this indexOf will check if our ref (exact) matchs (full line) with some white-list list, which is very limited.
So people couldn't match by some pattern (grep can work).

I haven't tested, but we may use part of above script to read a single file from a ref, and add another steps for "grep"/"sh"
I'm not sure if that script will cost more resources than git-checkout or not. And is that solutions over-engineered?

We could likewise try to get some information from the branch name. But that leads to debates about whether the default should be "off" or "on", and overriding still ends up somewhat awkward. If we default to "on", you have to remember to name your branches appropriately to skip CI. And if "off", you end up having to contort your branch names or duplicate your pushes with an extra refspec.

By comparison, this commit's solution lets you specify your config once and forget about it, and all of the data is off in its own ref, where it can be changed by individual forks without touching the main tree.

I used refs/ci/config as the config ref, which should be a commit whose tree contains various config files (right now the only one is "ref-whitelist"). It was intentional to avoid refs/heads/ here so we don't conflict with any branch workflows. But it does make it a little awkward to edit, since you can't check it out directly.

Right now the logic is to run CI for all branches by default, unless a whitelist exists, in which case the branch must be mentioned there (using its fully qualified ref name). We could easily add in a blacklist, as well. Or since we're running a shell in a VM, we really could just run "./allow-ref $refname" and let individual forks specify whatever shell code they like.

Documentation/SubmittingPatches +2 -0

+$ git push <your-fork> HEAD:refs/ci/config
+--------------
 
 Do not forget to update the documentation to describe the updated
 behavior and make sure that the resulting documentation set formats

contrib/ci-config-allow-ref new file +0 -0

new file mode 100755
index 0000000000..b53e9ddbd0

contrib/ci-config-allow-ref new file +6 -0

+
+REFNAME="$1"
+
+case "$REFNAME" in
+refs/heads/no-ci*) exit 1 ;;
+esac

Quoted 5 lines · Đoàn Trần Công Danh

+--------------
+$ git checkout --orphan ci-config
+$ cp contrib/ci-config-allow-ref allow-ref
+$ $EDITOR allow-ref
+$ git rm -rf .

This sounds horrible. You just nuked the entire files in the working tree you use for your everyday Git hacking to edit a single file.

As the instruction above says, we should set the example and describe the behaviour we implemented initially. Something as basic like ...

# Build any branch other than those whose name begins with "no-ci"

Quoted 10 lines · Junio C Hamano

Quoted 5 lines · Đoàn Trần Công Danh

+--------------
+$ git checkout --orphan ci-config
+$ cp contrib/ci-config-allow-ref allow-ref
+$ $EDITOR allow-ref
+$ git rm -rf .

This sounds horrible. You just nuked the entire files in the working tree you use for your everyday Git hacking to edit a single file.

It isn't that horrible as it sounds. It only removes the files that are currently added in index, which is the same with tracked files in old branch, and we can get it back by switching back to old branch.

I decided to make an orphanage branch because I would like to save time and network bandwidth for the "check-ci" jobs.

I wonder whether the "git rm -rf ." makes that block sounds horrible? If that is the case, we can use the experimental git-switch(1) instead, it's doing more-or-less the same (or is it the same?) with "git checkout --orphan" and "git rm -rf ."

Quoted 12 lines · Đoàn Trần Công Danh

Quoted 10 lines · Junio C Hamano

Quoted 5 lines · Đoàn Trần Công Danh

+--------------
+$ git checkout --orphan ci-config
+$ cp contrib/ci-config-allow-ref allow-ref
+$ $EDITOR allow-ref
+$ git rm -rf .

This sounds horrible. You just nuked the entire files in the working tree you use for your everyday Git hacking to edit a single file.

Quoted 6 lines

It isn't that horrible as it sounds. It only removes the files that are currently added in index, which is the same with tracked files in old branch, and we can get it back by switching back to old branch.

I decided to make an orphanage branch because I would like to save time and network bandwidth for the "check-ci" jobs.

I didn't say it is wrong to record a tree with a single file (allow-ref) in a commit that is pointed by the ci-config ref.

I would have expected you to create such a commit in an otherwise empty repository, and push into your fork of Git at GitHub. That way, you won't have to checkout and/or refresh the index all of the 3800+ files.

Yeah, I agree that all of the mechanisms for dealing with the unrelated history are somewhat awkward. Another issue is that you can't just:

  git clone --single-branch -b refs/ci/config my-config

to work on it, because "-b" wants only heads or tags (we could address that by putting it in refs/heads/ci-config or similar).

If we do go the javascript route, perhaps it would make sense for refs/ci/config to be a single blob containing a snippet of javascript with several functions. And then we could just eval() that and call the appropriate functions (if defined).

Then a sample can live in the main repo with something like:

# This file contains functions which will be run by the GitHub # Actions CI script.
#
# You may customize it for your own fork by modifying it on any branch

  # you like, and installing with:
  #
  #  git push <remote> $(git rev-parse HEAD:ci/config):refs/ci/config
  #
  # [allow_ref() sample definition and documentation....]

That sidesteps most of those issues.

Quoted 7 lines · Jeff King

Yeah, I agree that all of the mechanisms for dealing with the unrelated history are somewhat awkward. Another issue is that you can't just:

  git clone --single-branch -b refs/ci/config my-config

to work on it, because "-b" wants only heads or tags (we could address that by putting it in refs/heads/ci-config or similar).

I somehow don't think that it is such a huge issue, because I expect anybody who has legitimate interest in refs/ci/config to have a full clone of git.git anyway. So it is more like defining another remote.origin.fetch like this:

[remote "origin"]
url = https://git.kernel.org/pub/scm/git/git.git/
fetch = +refs/heads/*:refs/remotes/origin/*
+ fetch = +refs/ci/config:refs/remotes/origin/ci-config

[remote "publish"]
url = https://github.com/user/git/

and then do something like:

	$ git worktree add -b ci-config ../git-ci-config origin/ci-config
	$ cd ../git-ci-config
	... hack hack hack ...
	$ git push publish ci-config:refs/ci/config

In the patch I sent out a few hours ago I just caved and made refs/heads/ci-config the magic ref, in order to avoid complexity.

But it also wouldn't be too bad to say "look, you can store this however you like in refs/heads/ or not at all, but install it into place with git push <remote> HEAD:refs/ci/config". The audience is git.git developers (and even advanced ones whose workflows involve tweaking their CI config), so I'd like to think they could figure it out.

I have a feeling that fewer than 5 people in the world will end up using this feature either way. ;)

Subject: [PATCH] ci: allow per-branch config for GitHub Actions

Quoted 1 line · Jeff King

Subject: [PATCH] ci: allow per-branch config for GitHub Actions

Here's a "v2" of that patch based on the discussion.

I think it smooths some of the rough edges of the orphan-branch approach, while still having a cost on par with other suggestions (or at least ones that truly allow any config; we can check for "for-ci/**" or something very cheaply, but that implies hard-coding it for everybody). I think the cost here is acceptable, and it gives us room to add more features in the future.

If Actions eventually adds per-repo variable storage that can be used in "if:" conditionals, then we could eventually switch to that. :)

The documentation here should be enough to let people work with it. But we'd probably want to take Danh's patch to mention Actions in SubmittingPatches on top (and it possibly could be modified to point to the ci/config directory).

-- >8 --
Subject: [PATCH] ci: allow per-branch config for GitHub Actions

Depending on the workflows of individual developers, it can either be convenient or annoying that our GitHub Actions CI jobs are run on every branch. As an example of annoying: if you carry many half-finished work-in-progress branches and rebase them frequently against master, you'd get tons of failure reports that aren't interesting (not to mention the wasted CPU).

This commit adds a new job which checks a special branch within the repository for CI config, and then runs a shell script it finds there to decide whether to skip the rest of the tests. The default will continue to run tests for all refs if that branch or script is missing.

There have been a few alternatives discussed:

One option is to carry information in the commit itself about whether it should be tested, either in the tree itself (changing the workflow YAML file) or in the commit message (a "[skip ci]" flag or similar). But these are frustrating and error-prone to use:

- you have to manually apply them to each branch that you want to mark

- it's easy for them to leak into other workflows, like emailing patches

We could likewise try to get some information from the branch name. But that leads to debates about whether the default should be "off" or "on", and overriding still ends up somewhat awkward. If we default to "on", you have to remember to name your branches appropriately to skip CI. And if "off", you end up having to contort your branch names or duplicate your pushes with an extra refspec.

By comparison, this commit's solution lets you specify your config once and forget about it, and all of the data is off in its own ref, where it can be changed by individual forks without touching the main tree.

There were a few design decisions that came out of on-list discussion. I'll summarize here:

- we could use GitHub's API to retrieve the config ref, rather than a real checkout (and then just operate on it via some javascript). We still have to spin up a VM and contact GitHub over the network from it either way, so it ends up not being much faster. I opted to go with shell to keep things similar to our other tools (and really could implement allow-refs in any language you want). This also makes it easy to test your script locally, and to modify it within the context of a normal git.git tree.

- we could keep the well-known refname out of refs/heads/ to avoid cluttering the branch namespace. But that makes it awkward to manipulate. By contrast, you can just "git checkout ci-config" to make changes.

- we could assume the ci-config ref has nothing in it except config (i.e., a branch unrelated to the rest of git.git). But dealing with orphan branches is awkward. Instead, we'll do our best to efficiently check out only the ci/config directory using a shallow partial clone, which allows your ci-config branch to be just a normal branch, with your config changes on top.

- we could provide a simpler interface, like a static list of ref patterns. But we can't get out of spinning up a whole VM anyway, so we might as well use that feature to make the config as flexible as possible. If we add more config, we should be able to reuse our partial-clone to set more outputs.

Signed-off-by Jeff King <[email protected]>

---
 .github/workflows/main.yml  | 42 +++++++++++++++++++++++++++++++++++++
 ci/config/allow-refs.sample | 26 +++++++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100755 ci/config/allow-refs.sample

.github/workflows/main.yml +23 -0

index fd4df939b5..802a4bf7cd 100644
@@ -6,7 +6,39 @@ env:
   DEVELOPER: 1
 
 jobs:
+  ci-config:
+      runs-on: ubuntu-latest
+      outputs:
+        enabled: ${{ steps.check-ref.outputs.enabled }}
+      steps:
+        - name: try to clone ci-config branch
+          continue-on-error: true
+          run: |
+            git -c protocol.version=2 clone \
+              --no-tags \
+              --single-branch \
+              -b ci-config \
+              --depth 1 \
+              --no-checkout \
+              --filter=blob:none \
+              https://github.com/${{ github.repository }} \
+              config-repo &&
+              cd config-repo &&
+              git checkout HEAD -- ci/config
+        - id: check-ref
+          name: check whether CI is enabled for ref
+          run: |
+            enabled=yes

.github/workflows/main.yml +10 -0

     runs-on: windows-latest
     steps:
     - uses: actions/checkout@v1

@@ -70,6 +102,8 @@ jobs:
         name: failed-tests-windows
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   vs-build:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     env:
       MSYSTEM: MINGW64
       NO_PERL: 1

@@ -154,6 +188,8 @@ jobs:
                           ${{matrix.nr}} 10 t[0-9]*.sh)
         "@
   regular:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     strategy:
       matrix:
         vector:

@@ -189,6 +225,8 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   dockerized:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     strategy:
       matrix:
         vector:

@@ -213,6 +251,8 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   static-analysis:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     env:
       jobname: StaticAnalysis
     runs-on: ubuntu-latest

@@ -221,6 +261,8 @@ jobs:
     - run: ci/install-dependencies.sh
     - run: ci/run-static-analysis.sh
   documentation:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     env:
       jobname: Documentation
     runs-on: ubuntu-latest

ci/config/allow-refs.sample new file +0 -0

new file mode 100755
index 0000000000..f157f1945a

ci/config/allow-refs.sample new file +17 -1

+#   cp allow-refs.sample allow-refs
+#   $EDITOR allow-refs
+#   git commit -am "implement my ci preferences"
+#   git push
+#
+# This script will then be run when any refs are pushed to that repository. It
+# gets the fully qualified refname as the first argument, and should exit with
+# success only for refs for which you want to run CI.
+
+case "$1" in
+# allow one-off tests by pushing to "for-ci" or "for-ci/mybranch"
+refs/heads/for-ci*) true ;;
+# always build your integration branch
+refs/heads/my-integration-branch) true ;;
+# don't build any other branches or tags
+*) false ;;
+esac
--

2.26.2.1005.ge383644752

Review

Quoted 3 lines

windows-build:

(quoted diff) +2 -0

+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'

One thing I wonder is whether the downstream 'windows-test' partitions. I think that it should be fine, since we won't run the dependent 'windows-build', and then 'windows-test' won't have all of its prerequisites filled.

Quoted 4 lines

ci/config/allow-refs.sample new file +4 -0

+# Sample script for enabling/disabling GitHub Actions CI runs on
+# particular refs. By default, CI is run for all branches pushed to
+# GitHub. You can override this by dropping the ".sample" from the script,
+# editing it, committing, and pushing the result to the "ci-config" branch of

ci/config/allow-refs.sample line 9

Quoted 3 lines

ci/config/allow-refs.sample new file +3 -0

+# your repository:
+#
+#   git checkout -b ci-config

Should we be recommending '--orphan' instead of '-b' here? It looks like when you clone this branch down that you try to get as few bytes as possible, so I figure it may be easier to have this be a orphaned branch.

.github/workflows/main.yml +4 -0

+            then
+              enabled=no
+            fi
+            echo "::set-output name=enabled::$enabled"

.github/workflows/main.yml env: · line 33

Quoted 6 lines · Jeff King

.github/workflows/main.yml +6 -0

+        - id: check-ref
+          name: check whether CI is enabled for ref
+          run: |
+            enabled=yes
+            if test -x config-repo/ci/config/allow-ref &&
+               ! config-repo/ci/config/allow-ref '${{ github.ref }}'

Is it deliberate that the output from the script is not redirected to >/dev/null, which would mean they are allowed to do something that looks like:

echo "::set-output name=enabled::frotz"

or emit other random ::string-that-affects-github-actions to its standard output stream?

It was deliberate in the sense that I would allow them to write useful messages to the Actions log. If they want to do nonsense like "::set-output", then it's their foot and their gun.

I don't know if Actions distinguishes between stdout and stderr here (i.e., if we redirected the script's stdout to stderr, would that prevent this case or not?).

In any case, I'll queue it on 'pu'. Thanks.

Perhaps we can experiment with "echo >&2 we are getting called" in the allow-ref script itself ;-).

I just noticed this needs a small fix to the sample script, which I gave the wrong name:

ci/config/allow-ref.sample renamed from ci/config/allow-refs.sample +0 -0

similarity index 93%
rename from ci/config/allow-refs.sample
rename to ci/config/allow-ref.sample
index f157f1945a..c9c9aea9ff 100755

On Mon, May 04, 2020 at 10:49:31PM +0700, Đoàn Trần Công Danh wrote:
Doing "**" here makes sense to catch everything (it would be even better if we could just say "everything with a pull request" by omitting the branch filter entirely, but maybe that's not possible).

Review

Quoted 2 lines

(quoted diff) +2 -0

+    tags:
+      - '*'

Would we want that here, too? I guess nobody is likely to push "foo/v1.2.3".

Or on the flip side, would we want to tighten this? If I push a tag "wip", I probably don't want it built. Probably the right rule is "annotated tags only", but I suspect that's not possible.

Review

Quoted 8 lines

(quoted diff) +7 -0

+  push:
+    branches:
+      - maint
+      - master
+      - next
+      - jch
+      - pu

What happened to "for-ci" (presumably "for-ci/**")?

Documentation/SubmittingPatches on open source projects), you can use their Travis CI integration to · line 84

Quoted 4 lines · Đoàn Trần Công Danh

Documentation/SubmittingPatches +4 -0

+Alternately, you can use GitHub Actions (which supports testing your changes
+on Linux, macOS, and Windows) by pushing into a branch whose name starts
+with "for-ci/" or opening a GitHub's Pull Request against
+https://github.com/git/git.git

Can you tighten the description of "for-ci/" a bit? It's not like the convention is offered in _any_ repository, but it is active only if you push to a fork of git.git, right?

If your fork is a fork of
a fork, what happens (e.g. github.com/gitster/git is marked as a fork of git/git; when somebody forks from gitster/git, would they also get the for-ci/ convention)?

From 889cacb689 (ci: configure GitHub Actions for CI/PR, 2020-04-11), GitHub Actions was introduced as an alternative CI system for Git project.

Let's advertise it to Git's contributors to help them test Git on various platforms before submitting to Git.

Quoted 6 lines · Đoàn Trần Công Danh

From 889cacb689 (ci: configure GitHub Actions for CI/PR, 2020-04-11), GitHub Actions was introduced as an alternative CI system for Git project.

Let's advertise it to Git's contributors to help them test Git on various platforms before submitting to Git.

I think this makes sense. Two things:

Documentation/SubmittingPatches on open source projects), you can use their Travis CI integration to · line 84

Quoted 4 lines

Documentation/SubmittingPatches +4 -0

+Alternately, you can use GitHub Actions (which supports testing your changes
+on Linux, macOS, and Windows) by pushing into a branch in your fork
+or opening a GitHub's Pull Request against
+https://github.com/git/git.git or a fork of that repository.

Probably "GitHub Pull Request" would be more idiomatic English.

Do people need to enable Actions on their forks for the branch push to work? I didn't need to for my fork of git/git, but I'm not sure if that's because I was playing with Actions months ago and forgot, or if having actions enabled on the parent repo makes it work.

When I made a new repository that was not connected, I had to explicitly enable Actions on the site before it would run the workflow file.

Quoted 6 lines

Documentation/SubmittingPatches on open source projects), you can use their Travis CI integration to · line 84

Quoted 4 lines

Documentation/SubmittingPatches +4 -0

+Alternately, you can use GitHub Actions (which supports testing your changes
+on Linux, macOS, and Windows) by pushing into a branch in your fork
+or opening a GitHub's Pull Request against
+https://github.com/git/git.git or a fork of that repository.

Probably "GitHub Pull Request" would be more idiomatic English.

I guess you're right.

Well, I'm not native English speaker.
I was thinking, this kind of Pull Request is specific to GitHub, and it's different from git-request-pull(1), So, I use "'s" :)

Quoted 2 lines

When I made a new repository that was not connected, I had to explicitly enable Actions on the site before it would run the workflow file.

It seems like GitHub Actions will be triggered automatically if GitHub finds any files in "$TOPDIR/.github/workflows/*.yml"

.github/workflows/main.yml line 12

Quoted 11 lines · Đoàn Trần Công Danh

.github/workflows/main.yml +10 -1

-on: [push, pull_request]
+on:
+  pull_request:
+  push:
+    branches:
+      - maint
+      - master
+      - next
+      - jch
+      - pu
+      - 'for-ci**'

Should this be "for-ci/**", or are we intending for "for-ci-foo" to work? I'd suspect anybody who uses this would use a full directory namespace in a refspec (like "refs/heads/*:refs/heads/for-ci/*"). It might be simpler conceptually to only support that.

.github/workflows/main.yml line 15

Quoted 4 lines

.github/workflows/main.yml +3 -0

+    tags:
+      - '**'
+      - '!**wip**'

IMHO this "wip" match is going too far. That was the name in the example I used, but really it could have been anything. I think we should either:

- just build all tags; it usually takes special effort to push them up anyway, so a one-off "just mark this spot" tag likely wouldn't get pushed anyway

- just build v[0-9]*, which would catch actual releases

Quoted 4 lines

Should this be "for-ci/**", or are we intending for "for-ci-foo" to work? I'd suspect anybody who uses this would use a full directory namespace in a refspec (like "refs/heads/*:refs/heads/for-ci/*"). It might be simpler conceptually to only support that.

I made this because I saw someone mentioned that they would like to push to 'for-ci' and expect it works for them.

I guess it may be better to have:

- for-ci
- for-ci/**

Git's maintainer usually don't have enough time to debug the failure of invidual feature branch, they usually want to look at intergration branches only.

Contributors now can have GitHub Actions as an opt-in option, should they want to check their code, they will push into designated branch.

Should we want to enable GitHub Action for all repo-s, except git/git and gitster/git. We'll want to have this patch fix-up to the previous one.

It seems like there're 2 schools for GitHub Actions,
- Opt-in by using some specific patterns
- Opt-out by using some specific patterns

I tried to help everyone happy, but with my current knowledge, I couldn't.

I'm not sure whether we want to be building all of the individual topics in gitster/git or not. In theory that provides more information, but I'm not sure if anybody is looking at them (and all of the notifications would go to Junio anyway).