| Message ID | 20230830194919.GA1709446@coredump.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | replacing ci/config/allow-ref with a repo variable | expand |
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
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 >
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
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
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
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
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
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
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
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
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