Re: [PATCH 1/2] ci: allow branch selection through "vars"
- From
Jeff King <peff@peff.net>
- Date
- Sep 5, 2023, 07:30 UTC
- Message-ID
- <20230905073013.GI199565@coredump.intra.peff.net>
- In-Reply-To
- <d000f469-2b7d-e037-c92e-013034490461@gmx.de>
On Sun, Sep 03, 2023 at 10:59:37AM +0200, Johannes Schindelin wrote:
Show 14 quoted lines
> > 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