| Message ID | 20230830195113.GA1709824@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 21c82dcd623a584b2f716f70eeec1db0326e46db |
| Headers | show |
| Series | replacing ci/config/allow-ref with a repo variable | expand |
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
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
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
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
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").
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