[PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Jeff King]

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

[PATCH 1/2] ci: allow branch selection through "vars"

[Jeff King]

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

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 1b41278a7f..c364abb8f8 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -21,6 +21,7 @@ concurrency:
 jobs:
   ci-config:
     name: config
+    if: vars.CI_BRANCHES == '' || contains(vars.CI_BRANCHES, github.ref_name)
     runs-on: ubuntu-latest
     outputs:
       enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
diff --git a/ci/config/README b/ci/config/README
new file mode 100644
index 0000000000..8de3a04e32
--- /dev/null
+++ b/ci/config/README
@@ -0,0 +1,14 @@
+You can configure some aspects of the GitHub Actions-based CI on a
+per-repository basis by setting "variables" and "secrets" from with the
+GitHub web interface. These can be found at:
+
+  https://github.com/<user>/git/settings/secrets/actions
+
+The following variables can be used:
+
+ - CI_BRANCHES
+
+   By default, CI is run when any branch is pushed. If this variable is
+   non-empty, then only the branches it lists will run CI. Branch names
+   should be separated by spaces, and should use their shortened form
+   (e.g., "main", not "refs/heads/main").
-- 
2.42.0.528.g5e092cb179

Re: [PATCH 1/2] ci: allow branch selection through "vars"

[Johannes Schindelin]

Hi Jeff,

On Wed, 30 Aug 2023, Jeff King wrote:

> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 1b41278a7f..c364abb8f8 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -21,6 +21,7 @@ concurrency:
>  jobs:
>    ci-config:
>      name: config
> +    if: vars.CI_BRANCHES == '' || contains(vars.CI_BRANCHES, github.ref_name)

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

Re: [PATCH 1/2] ci: allow branch selection through "vars"

[Jeff King]

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

> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> > index 1b41278a7f..c364abb8f8 100644
> > --- a/.github/workflows/main.yml
> > +++ b/.github/workflows/main.yml
> > @@ -21,6 +21,7 @@ concurrency:
> >  jobs:
> >    ci-config:
> >      name: config
> > +    if: vars.CI_BRANCHES == '' || contains(vars.CI_BRANCHES, github.ref_name)
> 
> 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.

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.

> 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))`.

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

Re: [PATCH 1/2] ci: allow branch selection through "vars"

[Johannes Schindelin]

Hi Jeff,

On Tue, 5 Sep 2023, Jeff King wrote:

> [...] 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?

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

Re: [PATCH 1/2] ci: allow branch selection through "vars"

[Jeff King]

On Tue, Sep 05, 2023 at 12:51:14PM +0200, Johannes Schindelin 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.

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

[PATCH 2/2] ci: deprecate ci/config/allow-ref script

[Jeff King]

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

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index c364abb8f8..dcf7d78f1d 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -44,10 +44,13 @@ jobs:
         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 }}'
+          if test -x config-repo/ci/config/allow-ref
           then
-            enabled=no
+            echo "::warning::ci/config/allow-ref is deprecated; use CI_BRANCHES instead"
+            if ! config-repo/ci/config/allow-ref '${{ github.ref }}'
+            then
+              enabled=no
+            fi
           fi
 
           skip_concurrent=yes
diff --git a/ci/config/allow-ref.sample b/ci/config/allow-ref.sample
deleted file mode 100755
index af0e076f8a..0000000000
--- a/ci/config/allow-ref.sample
+++ /dev/null
@@ -1,27 +0,0 @@
-#!/bin/sh
-#
-# 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
-# your repository:
-#
-#   git checkout -b ci-config
-#   cp allow-ref.sample allow-ref
-#   $EDITOR allow-ref
-#   git add allow-ref
-#   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.42.0.528.g5e092cb179

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Very nice.

> 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

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Phillip Wood]

Hi Peff

On 30/08/2023 20:49, Jeff King wrote:
> This is a more efficient way to do the same thing that
> ci/config/allow-ref does (which didn't exist back then).

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

> 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
> 

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Jeff King]

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

> On 30/08/2023 20:49, Jeff King wrote:
> > This is a more efficient way to do the same thing that
> > ci/config/allow-ref does (which didn't exist back then).
> 
> 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?

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

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Phillip Wood]

On 01/09/2023 18:32, Jeff King wrote:
> On Fri, Sep 01, 2023 at 02:24:59PM +0100, Phillip Wood wrote:
> 
>> On 30/08/2023 20:49, Jeff King wrote:
>>> This is a more efficient way to do the same thing that
>>> ci/config/allow-ref does (which didn't exist back then).
>>
>> 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?
> 
> Drat, I was hoping nobody was using it that way. :)

Sorry to be a pain.

> 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?

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

Best Wishes

Phillip

> -Peff

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Jeff King]

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

> > 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?
> 
> Oh I'd missed that, yes I do. All the globs are prefix matches but I'm not
> sure that helps.

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

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Phillip Wood]

On 05/09/2023 08:24, Jeff King wrote:
> On Mon, Sep 04, 2023 at 10:56:15AM +0100, Phillip Wood wrote:
> 
>>> 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?
>>
>> Oh I'd missed that, yes I do. All the globs are prefix matches but I'm not
>> sure that helps.
> 
> 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.

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

> -Peff

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Jeff King]

On Thu, Sep 07, 2023 at 11:04:33AM +0100, Phillip Wood 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.

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

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Phillip Wood]

Hi Peff

On 11/09/2023 10:36, Jeff King wrote:
> On Thu, Sep 07, 2023 at 11:04:33AM +0100, Phillip Wood wrote:
> 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,

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

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Jeff King]

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

> On 11/09/2023 10:36, Jeff King wrote:
> > On Thu, Sep 07, 2023 at 11:04:33AM +0100, Phillip Wood wrote:
> > 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,
> 
> 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.

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).

> 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.

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

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Jeff King]

Two minor corrections in what I wrote...

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

> (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).

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).

> 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.

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

-Peff

Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable

[Phillip Wood]

On 14/09/2023 01:30, Jeff King wrote:
> On Wed, Sep 13, 2023 at 04:16:48PM +0100, Phillip Wood wrote:
> 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.

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

>> 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.
> 
> 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 ...

Oh I didn't know about that

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

That's a shame

Best Wishes

Phillip