patch, 2 partsreplacing ci/config/allow-ref with a repo variable

This is a more efficient way to do the same thing that ci/config/allow-ref does (which didn't exist back then).

We should be able to do the same with ci/config/skip-concurrent (and just use vars.CI_SKIP_CONCURRENT) throughout the workflow. But I didn't test that at all.

After that, the only useful thing left in the "config" job would be the "skip-if-redundant" step. I'm not sure if it will be possible to get the same behavior there without spinning up a VM.

  [1/2]: ci: allow branch selection through "vars"
  [2/2]: ci: deprecate ci/config/allow-ref script

 .github/workflows/main.yml | 10 +++++++---
 ci/config/README           | 14 ++++++++++++++
 ci/config/allow-ref.sample | 27 ---------------------------
 3 files changed, 21 insertions(+), 30 deletions(-)
 create mode 100644 ci/config/README
 delete mode 100755 ci/config/allow-ref.sample

When we added config to skip CI for certain branches in e76eec3554 (ci: allow per-branch config for GitHub Actions, 2020-05-07), there wasn't any way to avoid spinning up a VM just to check the config. From the developer's perspective this isn't too bad, as the "skipped" branches complete successfully after running the config job (the workflow result is "success" instead of "skipped", but that is a minor lie).

But we are still wasting time and GitHub's CPU to spin up a VM just to check the result of a short shell script. At the time there wasn't any way to avoid this. But they've since introduced repo-level variables that should let us do the same thing:

  https://github.blog/2023-01-10-introducing-required-workflows-and-configuration-variables-to-github-actions/#configuration-variables

This is more efficient, and as a bonus is probably less confusing to configure (the existing system requires sticking your config on a magic ref).

See the included docs for how to configure it.

The code itself is pretty simple: it checks the variable and skips the config job if appropriate (and everything else depends on the config job already). There are two slight inaccuracies here:

  - we don't insist on branches, so this likewise applies to tag names
    or other refs. I think in practice this is OK, and keeping the code
    (and docs) short is more important than trying to be more exact. We
    are targeting developers of git.git and their limited workflows.

  - the match is done as a substring (so if you want to run CI for
    "foobar", then branch "foo" will accidentally match). Again, this
    should be OK in practice, as anybody who uses this is likely to only
    specify a handful of well-known names. If we want to be more exact,
    we can have the code check for adjoining spaces. Or even move to a
    more general CI_CONFIG variable formatted as JSON. I went with this
    scheme for the sake of simplicity.

Signed-off-by: Jeff King <peff@peff.net>
---
 .github/workflows/main.yml |  1 +
 ci/config/README           | 14 ++++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 ci/config/README

Hi Jeff,

On Wed, 30 Aug 2023, Jeff King wrote:

This might be too loose a check, as branch names that are a substring of any name listed in `CI_BRANCHES` would be false positive match. For example, if `CI_BRANCHES` was set to `maint next seen`, a branch called `see` would be a false match.

Due to the absence of a `concat()` function (for more details, see https://docs.github.com/en/actions/learn-github-actions/expressions#functions), I fear that we'll have to resort to something like `contains(format(' {0} ', vars.CI_BRANCHES), format(' {0} ', github.ref_name))`.

Ciao, Johannes

On Sun, Sep 03, 2023 at 10:59:37AM +0200, Johannes Schindelin wrote:

Yes, I wrote it about it in the commit message. :)

My assumption is that this may be good enough, just because we are realistically talking about the needs of a handful of Git developers. Folks doing one-off patches would just push to their forks and get CI (which they'd want in order to use GGG anyway). This is for people with more exotic workflows, and my guess is that half a dozen people would use this at all.

But we can make it more robust if we think somebody will actually run into it in practice.

Yeah, I had imagined checking startsWith() and endsWith(), but auto-inserting the leading/trailing space as you suggest is even shorter.

I think that contains() is more robust if used against an actual list data structure. But there doesn't seem to be any split()-type function. So I don't see how to get one short of using fromJSON(). But coupled with Phillip's use cases in the other part of the thread, maybe we should have a JSON-formatted CI_CONFIG variable instead.

That requires the developer to hand-write a bit of JSON, but it's not too bad (and again, I really think it's only a couple folks using this).

What do you think?

-Peff

Hi Jeff,

On Tue, 5 Sep 2023, Jeff King wrote:

Thank you for asking my opinion. The `[no ci]` support described in https://github.blog/changelog/2021-02-08-github-actions-skip-pull-request-and-push-workflows-with-skip-ci/ solves the problem adequately and with a lot less complexity than the current or the `vars.`-based solution. In my opinion.

Ciao, Johannes

On Tue, Sep 05, 2023 at 12:51:14PM +0200, Johannes Schindelin wrote:

Unfortunately that doesn't work well for the uses cases allow-ref was meant to support, for the reasons given in e76eec3554 (ci: allow per-branch config for GitHub Actions, 2020-05-07).

-Peff

Now that we have the CI_BRANCHES mechanism, there is no need for anybody to use the ci/config/allow-ref mechanism. In the long run, we can hopefully remove it and the whole "config" job, as it consumes CPU and adds to the end-to-end latency of the whole workflow. But we don't want to do that immediately, as people need time to migrate until the CI_BRANCHES change has made it into the workflow file of every branch.

So let's issue a warning, which will appear in the "annotations" section below the workflow result in GitHub's web interface. And let's remove the sample allow-refs script, as we don't want to encourage anybody to use it.

Signed-off-by: Jeff King <peff@peff.net>
---
 .github/workflows/main.yml |  9 ++++++---
 ci/config/allow-ref.sample | 27 ---------------------------
 2 files changed, 6 insertions(+), 30 deletions(-)
 delete mode 100755 ci/config/allow-ref.sample

Jeff King <peff@peff.net> writes:

Very nice.

Hi Peff

On 30/08/2023 20:49, Jeff King wrote:

I like the idea of a more efficient way to skip the ci for certain refs. I've got my allow-ref script set up to reject a bunch of refs and run the ci on everything else. It's not clear to me how to replicate that with the setup proposed here. Would it be possible to add a second variable that prevents the ci from being run if it contains ref being pushed?

Best Wishes

Phillip

On Fri, Sep 01, 2023 at 02:24:59PM +0100, Phillip Wood wrote:

Drat, I was hoping nobody was using it that way. :)

Yes, I think it would be possible to do something like:

  if: |
    (vars.CI_BRANCHES == '' || contains(vars.CI_BRANCHES, github.ref_name)) &&
    !contains(vars.CI_BRANCHES_REJECT, github.ref_name)

It doesn't allow globbing, though. Do you need that?

-Peff

On 01/09/2023 18:32, Jeff King wrote:

Sorry to be a pain.

Oh I'd missed that, yes I do. All the globs are prefix matches but I'm not sure that helps.

Best Wishes

Phillip

On Mon, Sep 04, 2023 at 10:56:15AM +0100, Phillip Wood wrote:

It does make it easier. There's no globbing function available to us, but if we know something is a prefix, there's a startsWith() we can use. It does seem we're getting a combinatorial expansion of things to check, though:

  - full names to accept
  - full names to reject
  - prefixes to accept
  - prefixes to reject

I wrote "prefixes" but I'm actually not sure how feasible that is. That implies iterating over the list of prefixes, which I'm not sure we can do.

-Peff

On 05/09/2023 08:24, Jeff King wrote:

I scanned the github documentation the other day and wondered if it would be possible to use with fromJson with a json array to do a prefx match on each element. It all sounds like it is getting a bit complicated though.

Best Wishes

Phillip

On Thu, Sep 07, 2023 at 11:04:33AM +0100, Phillip Wood wrote:

I poked around and I don't think this is possible. You can use contains() to do a full match of items in a json array, but I don't see any other way to iterate. What we'd really want is a "map" function, to do something like:

  map { startsWith(github.ref_name, $1) } fromJSON(vars.CI_CONFIG.allow_prefix)

But no such "map" exists (and I guess we'd need to collapse the result down to "is any item true", similar to perl's "grep" function).

Do you need multiple prefixes, or would one each of allow/reject be sufficient? I tried this and it works:

jobs:
  ci-config:
    name: config
    if: |
      (fromJSON(vars.CI_CONFIG).allow == '' ||
       contains(fromJSON(vars.CI_CONFIG).allow, github.ref_name)) &&
      (fromJSON(vars.CI_CONFIG).reject == '' ||
       !contains(fromJSON(vars.CI_CONFIG).reject, github.ref_name)) &&
      (fromJSON(vars.CI_CONFIG).allow-prefix == '' ||
       startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).allow-prefix)) &&
      (fromJSON(vars.CI_CONFIG).reject-prefix == '' ||
       !startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).reject-prefix))

And then various values of CI_CONFIG seem to work:

  - allow explicit branch names ("one" and "two" are run, everything
    else is skipped)

    { "allow": ["one", "two"] }

  - reject explicit names (everything except "three" is skipped)

    { "reject": ["three"] }

  - allow by prefix ("ok/* is run, everything else is skipped)

    { "allow-prefix": "ok/" }

  - reject by prefix (everything except "nope/*" is run)

    { "reject-prefix": "nope/" }

  - every key must approve to run, but missing keys default to running.
    So a reject overrides allow ("ok/one" and "ok/two" run, but
    "ok/three" does not)

    {
      "allow-prefix": "ok/",
      "reject", "ok/three"
    }

I suppose one hacky way to support multiple prefixes is to just unroll the loop ourselves (since missing keys are ignored). I didn't test it, but something like:

      (fromJSON(vars.CI_CONFIG).allow-prefix[0] == '' ||
       startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).allow-prefix[0])) &&
      (fromJSON(vars.CI_CONFIG).allow-prefix[1] == '' ||
       startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).allow-prefix[1])) &&
      (fromJSON(vars.CI_CONFIG).allow-prefix[2] == '' ||
       startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).allow-prefix[2])) &&
       ...etc...

Would allow:

  {
    "allow-prefix": [
      "ok/",
      "also-ok/"
    ]
  }

Looking at the ci-config branch of phillipwood/git.git, I see this in your allow-refs:

      refs/heads/main|refs/heads/master|refs/heads/maint|refs/heads/next|refs/heads/seen|refs/tags/gitgui-*|refs/tags/pr-[0-9]*|refs/tags/v[1-9].*)

So you do use multiple prefixes, though all in refs/tags/. Do you actually push tags for which you do want to run CI, or would it be sufficient to just reject "refs/tags/" completely (though that implies using the fully qualified ref and not the short name; it would also be easy to add a boolean "allow tags" config option).

-Peff

Hi Peff

On 11/09/2023 10:36, Jeff King wrote:

Yes, but not very often - I could probably just reject all tags and start the CI manually when I want it (assuming that's an option). Thanks for digging into the various options, it sounds like it is possible so long as we don't want multiple prefixes.

Aside: what I'd really like is to be able to set an environment variable 
when I push to skip or force the CI

	GITHUB_SKIP_CI=1 git push github ...

but that would require support from the git client, the protocol and the server.

Best Wishes

Phillip

On Wed, Sep 13, 2023 at 04:16:48PM +0100, Phillip Wood wrote:

I'll additionally have to switch to using the full refname in the matches, but that is probably a reasonable thing to do anyway (it makes config a bit more verbose, but it is obviously much more flexible).

We can do the loop-unroll thing if we really want to support multiple prefixes, but if you're OK with it, let's try the single-prefix way and see if anybody runs into problems (I'm still convinced there's only a few of us using this stuff anyway). I'm hesitant to do the unroll just because it requires picking a maximum value, with a bizarre failure if you happen to have 4 prefixes or whatever.

I'll see if I can polish up what I showed earlier into a patch.

(BTW, one other thing I tried was using fromJSON(vars.CI_CONFIG) in the "on" expression, which in theory would allow globbing. But it doesn't look like expressions work at all in that context. And even if they did, I'm not sure we could make it work such that an empty CI_CONFIG used some defaults, since there's no conditional-expression ternary operator).

We have the necessary bits at the protocol: push-options. But it's up to the server-side hooks to decide which ones are meaningful and to do something useful with them. It looks like GitLab supports:

  git push -o ci.skip=1 ...

but I don't think GitHub respects any equivalent option.

-Peff

Two minor corrections in what I wrote...

On Wed, Sep 13, 2023 at 08:30:10PM -0400, Jeff King wrote:

It's not a real ?: operator, but GitHub does use the word "ternary" to describe the short-circuit behavior of:

  condition && "if-true" || "if-not-true"

So in theory we could do:

  on: ${{ fromJSON(var.CI_ON != '' && vars.CI_ON || "[pull_request, push]" }}

but I couldn't make it work (it complains about the value of "on" in such a way that I assume it is simply not expanding expressions at all).

The syntax here is actually just "git push -o ci.skip", without the equals.

-Peff

On 14/09/2023 01:30, Jeff King wrote:

Lets start with a single-prefix and see if anyone complains

Oh I didn't know about that

That's a shame

Best Wishes

Phillip