All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR"
@ 2020-05-02 15:08 Johannes Schindelin via GitGitGadget
  2020-05-03  9:36 ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-05-02 15:08 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Jeff Hostetler, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It might have been invented by Travis CI (at least it is described here:
https://docs.travis-ci.com/user/customizing-the-build/#skipping-a-build)
to avoid unnecessary builds: if the tip commit message (or for PR
builds, the PR description) contains the needle `[skip ci]`, then the
build is skipped.

Unlike Azure Pipelines, GitHub workflows does not support that feature
out of the box (at least not at the time of writing), but we can
reinstate it manually.

Note: GitHub workflows might implement this at some stage. In the least,
the desire for this feature is expressed, together with some history, at
https://github.community/t5/GitHub-Actions/GitHub-Actions-does-not-respect-skip-ci/m-p/42834

While it is the number five in the kudo ranking of the GitHub Actions
posts, it might take a while given the current state of the world. In
the meantime let's do the manual thing.

Following Travis CI's example, we also add special handling to exclude
the PR build when the PR title or description (or any other part of the
pull request information provided by the GitHub event) contains the
needle `[skip pr]`.

Note: for technical reasons, a PR build will still run if a commit
message contains the needle `[skip ci]` but neither PR title nor
description contain the needle `[skip pr]`: for PR builds, the commit
messages are not part of the payload delivered via the event.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    ci: allow skipping CI/PR builds on GitHub
    
    It was mentioned to me that it might not be totally helpful to run all 
    the builds whenever an in-progress branch is pushed to GitHub. For
    example, if a contributor has dozens of topic branches in flight, they
    might not want to have all of them built whenever a new end-of-day push
    happens.
    
    This patch tries to address that, by following the convention that I
    believe Travis CI invented: if a commit message contains the needle 
    [skip ci], any CI build (i.e. a build triggered by a push) is skipped.
    Likewise, PR builds are skipped when the PR title and/or description
    contains [skip pr].
    
    Ideally, GitHub workflows will support this feature at some stage, but
    until then, we could have this (admittedly not very elegant) workaround.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-776%2Fdscho%2Fskip-ci-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-776/dscho/skip-ci-v1
Pull-Request: https://github.com/git/git/pull/776

 .github/workflows/main.yml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index fd4df939b50..0e4a280d309 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -7,6 +7,7 @@ env:
 
 jobs:
   windows-build:
+    if: "!contains(toJSON(github.event.commits.*.message), '[skip ci]') && !contains(toJSON(github.event.pull_request), '[skip pr]')"
     runs-on: windows-latest
     steps:
     - uses: actions/checkout@v1
@@ -70,6 +71,7 @@ jobs:
         name: failed-tests-windows
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   vs-build:
+    if: "!contains(toJSON(github.event.commits.*.message), '[skip ci]') && !contains(toJSON(github.event.pull_request), '[skip pr]')"
     env:
       MSYSTEM: MINGW64
       NO_PERL: 1
@@ -154,6 +156,7 @@ jobs:
                           ${{matrix.nr}} 10 t[0-9]*.sh)
         "@
   regular:
+    if: "!contains(toJSON(github.event.commits.*.message), '[skip ci]') && !contains(toJSON(github.event.pull_request), '[skip pr]')"
     strategy:
       matrix:
         vector:
@@ -189,6 +192,7 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   dockerized:
+    if: "!contains(toJSON(github.event.commits.*.message), '[skip ci]') && !contains(toJSON(github.event.pull_request), '[skip pr]')"
     strategy:
       matrix:
         vector:
@@ -213,6 +217,7 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   static-analysis:
+    if: "!contains(toJSON(github.event.commits.*.message), '[skip ci]') && !contains(toJSON(github.event.pull_request), '[skip pr]')"
     env:
       jobname: StaticAnalysis
     runs-on: ubuntu-latest
@@ -221,6 +226,7 @@ jobs:
     - run: ci/install-dependencies.sh
     - run: ci/run-static-analysis.sh
   documentation:
+    if: "!contains(toJSON(github.event.commits.*.message), '[skip ci]') && !contains(toJSON(github.event.pull_request), '[skip pr]')"
     env:
       jobname: Documentation
     runs-on: ubuntu-latest

base-commit: b34789c0b0d3b137f0bb516b417bd8d75e0cb306
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR"
  2020-05-02 15:08 [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR" Johannes Schindelin via GitGitGadget
@ 2020-05-03  9:36 ` Jeff King
  2020-05-03 12:05   ` Danh Doan
  2020-05-03 16:46   ` [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR" Junio C Hamano
  0 siblings, 2 replies; 62+ messages in thread
From: Jeff King @ 2020-05-03  9:36 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Jeff Hostetler, Johannes Schindelin

On Sat, May 02, 2020 at 03:08:07PM +0000, Johannes Schindelin via GitGitGadget wrote:

>     It was mentioned to me that it might not be totally helpful to run all 
>     the builds whenever an in-progress branch is pushed to GitHub. For
>     example, if a contributor has dozens of topic branches in flight, they
>     might not want to have all of them built whenever a new end-of-day push
>     happens.

As I was probably (one of) the mentioners here, thank you for looking
into this. This definitely _helps_, and if you don't want to go further
I could certainly make do with it[1].

But here are some thoughts on what a more ideal solution would look
like (to me, anyway).

What I'd _most_ like is a separately-maintained list of branches (or
branch patterns) on which to run CI.  I mostly care about just testing
my personal equivalent of "next" (which is really "next" plus my stable
topics), and I'd probably only list that branch. Plus potentially a
special CI branch that I'd use when chasing down a failure that I can't
reproduce locally.

But I don't think there's any good way to implement that via GitHub
Actions. I thought perhaps we could pull data from a different branch in
the same repo, but referring to external "Actions" seems to require a
full repo name (and obviously putting git/git in that name doesn't help
anybody who's trying to override something in a fork).

Another alternative: could we trigger CI based on branch-names? Locally,
I call my unstable branches "jk/something-wip", and I'd like to skip all
of the "-wip" branches. That's basically equivalent to "[skip ci]" in
that I could just amend the tip commit of the -wip branches to say "skip
ci". But what if we flipped the default to _not_ build? I.e.:

  - continue to build all pull requests; if you opened one, you're
    serious enough to have other people look and should get CI feedback

  - build branches with a few well-known names or patterns. Maybe
    names like "master"? Or maybe "refs/heads/build-ci/*"? Or maybe
    anything in "refs/heads/<initials>/*"? We'd have to decide on a
    convention as a community.

  - do not build anything else; we have no idea if somebody in a fork is
    just pushing up a work in progress or not, and they may be surprised
    to get a CI failure notification back.

I'm not sure whether we want to be building all of the individual topics
in gitster/git or not. In theory that provides more information, but I'm
not sure if anybody is looking at them (and all of the notifications
would go to Junio anyway).

My ideas aren't really developed, but I guess what I'm wondering
foremost is whether other people are thinking along the same lines.

> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index fd4df939b50..0e4a280d309 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -7,6 +7,7 @@ env:
>  
>  jobs:
>    windows-build:
> +    if: "!contains(toJSON(github.event.commits.*.message), '[skip ci]') && !contains(toJSON(github.event.pull_request), '[skip pr]')"

It's unfortunate to have to repeat this in every job, as opposed to in
the "on" event block, but I don't think that block is flexible enough to
do what we want here.

I don't know very much about Actions, but the code looks correct to me.

-Peff

[1] My current workaround is even more horrendous: I've turned off
    Actions entirely in peff/git, and then I separately push branches I
    want CI on into a separate repository. But that repo can't be a fork
    of git/git, because I already have one! So I now have an extra
    "git-ci" repo that isn't connected to anything. Yuck.

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR"
  2020-05-03  9:36 ` Jeff King
@ 2020-05-03 12:05   ` Danh Doan
  2020-05-04 15:01     ` Jeff King
  2020-05-03 16:46   ` [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR" Junio C Hamano
  1 sibling, 1 reply; 62+ messages in thread
From: Danh Doan @ 2020-05-03 12:05 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Schindelin via GitGitGadget, git, Jeff Hostetler,
	Johannes Schindelin

On 2020-05-03 05:36:46-0400, Jeff King <peff@peff.net> wrote:
> On Sat, May 02, 2020 at 03:08:07PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 
> >     It was mentioned to me that it might not be totally helpful to run all 
> >     the builds whenever an in-progress branch is pushed to GitHub. For
> >     example, if a contributor has dozens of topic branches in flight, they
> >     might not want to have all of them built whenever a new end-of-day push
> >     happens.
> 
> As I was probably (one of) the mentioners here, thank you for looking
> into this. This definitely _helps_, and if you don't want to go further
> I could certainly make do with it[1].
> 
> But here are some thoughts on what a more ideal solution would look
> like (to me, anyway).
> 
> What I'd _most_ like is a separately-maintained list of branches (or
> branch patterns) on which to run CI.  I mostly care about just testing
> my personal equivalent of "next" (which is really "next" plus my stable
> topics), and I'd probably only list that branch. Plus potentially a
> special CI branch that I'd use when chasing down a failure that I can't
> reproduce locally.
> 
> But I don't think there's any good way to implement that via GitHub
> Actions. I thought perhaps we could pull data from a different branch in
> the same repo, but referring to external "Actions" seems to require a
> full repo name (and obviously putting git/git in that name doesn't help
> anybody who's trying to override something in a fork).
> 
> Another alternative: could we trigger CI based on branch-names? Locally,
> I call my unstable branches "jk/something-wip", and I'd like to skip all
> of the "-wip" branches. That's basically equivalent to "[skip ci]" in
> that I could just amend the tip commit of the -wip branches to say "skip
> ci". But what if we flipped the default to _not_ build? I.e.:
> 
>   - continue to build all pull requests; if you opened one, you're
>     serious enough to have other people look and should get CI feedback
> 
>   - build branches with a few well-known names or patterns. Maybe
>     names like "master"? Or maybe "refs/heads/build-ci/*"? Or maybe
>     anything in "refs/heads/<initials>/*"? We'd have to decide on a
>     convention as a community.

I think this is better approach for Junio, and we can help people opt
in by asking them to push into "for-ci/<some-name>.

This is my proposal for it:

------------------8<-------------
From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?=
 <congdanhqx@gmail.com>
Date: Sun, 3 May 2020 18:56:50 +0700
Subject: [PATCH] CI: GitHub: limit GitHub Action to intergration branches
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 .github/workflows/main.yml | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index fd4df939b5..303393ba73 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -1,6 +1,19 @@
 name: CI/PR
 
-on: [push, pull_request]
+on:
+  pull_request:
+    branches:
+      - '*'
+  push:
+    branches:
+      - maint
+      - master
+      - next
+      - jch
+      - pu
+      - 'for-ci/**'
+    tags:
+      - '*'
 
 env:
   DEVELOPER: 1
-- 
2.26.2.672.g232c24e857


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR"
  2020-05-03  9:36 ` Jeff King
  2020-05-03 12:05   ` Danh Doan
@ 2020-05-03 16:46   ` Junio C Hamano
  1 sibling, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-03 16:46 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Schindelin via GitGitGadget, git, Jeff Hostetler,
	Johannes Schindelin

Jeff King <peff@peff.net> writes:

> I'm not sure whether we want to be building all of the individual topics
> in gitster/git or not. In theory that provides more information, but I'm
> not sure if anybody is looking at them (and all of the notifications
> would go to Junio anyway).

I do not think I am letting GitHub notifications for gitster/* come
to my mailbox, so it is quite possible that many have piled up there
without anybody (not even me) seeing them.

I won't be locally able to notice breakages introduced by an
individual topic to an environment I do not have access to, and
building these topics with GitHub Actions may reveal which ones
break, say, vsbuild, but I won't be debugging and fixing such issues
myself anyway, so at least to me it is useless to run build there.

I think I agree with your outline, i.e. build pull requests to help
contributors, and build public integration branches to help the
project as a whole.

Thanks.

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR"
  2020-05-03 12:05   ` Danh Doan
@ 2020-05-04 15:01     ` Jeff King
  2020-05-04 15:49       ` [PATCH v2 0/2] Limit GitHub Actions to designated branches Đoàn Trần Công Danh
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-04 15:01 UTC (permalink / raw)
  To: Danh Doan
  Cc: Johannes Schindelin via GitGitGadget, git, Jeff Hostetler,
	Johannes Schindelin

On Sun, May 03, 2020 at 07:05:46PM +0700, Danh Doan wrote:

> This is my proposal for it:
> [...]
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index fd4df939b5..303393ba73 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -1,6 +1,19 @@
>  name: CI/PR
>  
> -on: [push, pull_request]
> +on:
> +  pull_request:
> +    branches:
> +      - '*'
> +  push:
> +    branches:
> +      - maint
> +      - master
> +      - next
> +      - jch
> +      - pu
> +      - 'for-ci/**'
> +    tags:
> +      - '*'

Yeah, this seems quite reasonable to me. I'd just add a refspec to push
a duplicate of branches I'm interested in running CI on to for-ci.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v2 0/2] Limit GitHub Actions to designated branches
  2020-05-04 15:01     ` Jeff King
@ 2020-05-04 15:49       ` Đoàn Trần Công Danh
  2020-05-04 15:49         ` [PATCH v2 1/2] CI: limit " Đoàn Trần Công Danh
                           ` (2 more replies)
  0 siblings, 3 replies; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-04 15:49 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Jeff Hostetler,
	Johannes Schindelin, Jeff King

It seems like there's at least one contributor like this proposal.

> Yeah, this seems quite reasonable to me. I'd just add a refspec to push
> a duplicate of branches I'm interested in running CI on to for-ci.

I've tweaked the commit message for the proposal and added a new patch to
advertise GitHub Actions to our friends.

With this series applied, we will trigger GitHub Actions CI for those branches
only:

- 5 integration branches: maint, master, next, jch, and pu
- Contributors' opted-in branches: 'for-ci/**'


Đoàn Trần Công Danh (2):
  CI: limit GitHub Actions to designated branches
  SubmittingPatches: advertise GitHub Actions CI

 .github/workflows/main.yml      | 14 +++++++++++++-
 Documentation/SubmittingPatches |  5 +++++
 2 files changed, 18 insertions(+), 1 deletion(-)

Range-diff against v1:
1:  106620b2d8 ! 1:  73de97dfeb CI: GitHub: limit GitHub Action to intergration branches
    @@ Metadata
     Author: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
      ## Commit message ##
    -    CI: GitHub: limit GitHub Action to intergration branches
    +    CI: limit GitHub Actions to designated branches
    +
    +    Git's maintainer usually don't have enough time to debug the failure of
    +    invidual feature branch, they usually want to look at intergration
    +    branches.
    +
    +    Contributors now can have GitHub Actions as an opt-in option,
    +    should they want to check their code, they will push into designated
    +    branch.
     
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
    @@ .github/workflows/main.yml
     +on:
     +  pull_request:
     +    branches:
    -+      - '*'
    ++      - '**'
     +  push:
     +    branches:
     +      - maint
-:  ---------- > 2:  24a8fefe5a SubmittingPatches: advertise GitHub Actions CI
-- 
2.26.2.672.g232c24e857


^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 15:49       ` [PATCH v2 0/2] Limit GitHub Actions to designated branches Đoàn Trần Công Danh
@ 2020-05-04 15:49         ` Đoàn Trần Công Danh
  2020-05-04 16:23           ` Jeff King
  2020-05-04 15:49         ` [PATCH v2 2/2] SubmittingPatches: advertise GitHub Actions CI Đoàn Trần Công Danh
  2020-05-05 16:26         ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Đoàn Trần Công Danh
  2 siblings, 1 reply; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-04 15:49 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Jeff Hostetler,
	Johannes Schindelin, Jeff King

Git's maintainer usually don't have enough time to debug the failure of
invidual feature branch, they usually want to look at intergration
branches.

Contributors now can have GitHub Actions as an opt-in option,
should they want to check their code, they will push into designated
branch.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 .github/workflows/main.yml | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index fd4df939b5..ea43b03092 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -1,6 +1,18 @@
 name: CI/PR
 
-on: [push, pull_request]
+on:
+  pull_request:
+    branches:
+      - '**'
+  push:
+    branches:
+      - maint
+      - master
+      - next
+      - jch
+      - pu
+    tags:
+      - '*'
 
 env:
   DEVELOPER: 1
-- 
2.26.2.672.g232c24e857


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v2 2/2] SubmittingPatches: advertise GitHub Actions CI
  2020-05-04 15:49       ` [PATCH v2 0/2] Limit GitHub Actions to designated branches Đoàn Trần Công Danh
  2020-05-04 15:49         ` [PATCH v2 1/2] CI: limit " Đoàn Trần Công Danh
@ 2020-05-04 15:49         ` Đoàn Trần Công Danh
  2020-05-04 16:37           ` Junio C Hamano
  2020-05-05 16:26         ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Đoàn Trần Công Danh
  2 siblings, 1 reply; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-04 15:49 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Jeff Hostetler,
	Johannes Schindelin, Jeff King

From 889cacb689 (ci: configure GitHub Actions for CI/PR, 2020-04-11),
GitHub Actions was introduced as an alternative CI system for Git
project.

Let's advertise it to Git's contributors to help them test Git on
various platforms before submitting to Git.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 Documentation/SubmittingPatches | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 4515cab519..741867dfe3 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -78,6 +78,11 @@ on open source projects), you can use their Travis CI integration to
 test your changes on Linux, Mac (and hopefully soon Windows).  See
 GitHub-Travis CI hints section for details.
 
+Alternately, you can use GitHub Actions (which supports testing your changes
+on Linux, macOS, and Windows) by pushing into a branch whose name starts
+with "for-ci/" or opening a GitHub's Pull Request against
+https://github.com/git/git.git
+
 Do not forget to update the documentation to describe the updated
 behavior and make sure that the resulting documentation set formats
 well (try the Documentation/doc-diff script).
-- 
2.26.2.672.g232c24e857


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 15:49         ` [PATCH v2 1/2] CI: limit " Đoàn Trần Công Danh
@ 2020-05-04 16:23           ` Jeff King
  2020-05-04 21:58             ` Taylor Blau
  2020-05-05  0:34             ` [PATCH v2 1/2] CI: limit GitHub Actions to designated branches Đoàn Trần Công Danh
  0 siblings, 2 replies; 62+ messages in thread
From: Jeff King @ 2020-05-04 16:23 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Jeff Hostetler, Johannes Schindelin

On Mon, May 04, 2020 at 10:49:31PM +0700, Đoàn Trần Công Danh wrote:

> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index fd4df939b5..ea43b03092 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -1,6 +1,18 @@
>  name: CI/PR
>  
> -on: [push, pull_request]
> +on:
> +  pull_request:
> +    branches:
> +      - '**'

Doing "**" here makes sense to catch everything (it would be even better
if we could just say "everything with a pull request" by omitting the
branch filter entirely, but maybe that's not possible).

> +    tags:
> +      - '*'

Would we want that here, too? I guess nobody is likely to push
"foo/v1.2.3".

Or on the flip side, would we want to tighten this? If I push a tag
"wip", I probably don't want it built. Probably the right rule is
"annotated tags only", but I suspect that's not possible.

> +  push:
> +    branches:
> +      - maint
> +      - master
> +      - next
> +      - jch
> +      - pu

What happened to "for-ci" (presumably "for-ci/**")?

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 2/2] SubmittingPatches: advertise GitHub Actions CI
  2020-05-04 15:49         ` [PATCH v2 2/2] SubmittingPatches: advertise GitHub Actions CI Đoàn Trần Công Danh
@ 2020-05-04 16:37           ` Junio C Hamano
  2020-05-05  0:46             ` Đoàn Trần Công Danh
  0 siblings, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-04 16:37 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Jeff Hostetler, Johannes Schindelin, Jeff King

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> +Alternately, you can use GitHub Actions (which supports testing your changes
> +on Linux, macOS, and Windows) by pushing into a branch whose name starts
> +with "for-ci/" or opening a GitHub's Pull Request against
> +https://github.com/git/git.git

Can you tighten the description of "for-ci/" a bit?  It's not like
the convention is offered in _any_ repository, but it is active only
if you push to a fork of git.git, right?  If your fork is a fork of
a fork, what happens (e.g. github.com/gitster/git is marked as a
fork of git/git; when somebody forks from gitster/git, would they
also get the for-ci/ convention)?

Thanks.


^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 16:23           ` Jeff King
@ 2020-05-04 21:58             ` Taylor Blau
  2020-05-04 22:52               ` Junio C Hamano
  2020-05-04 23:36               ` Jeff King
  2020-05-05  0:34             ` [PATCH v2 1/2] CI: limit GitHub Actions to designated branches Đoàn Trần Công Danh
  1 sibling, 2 replies; 62+ messages in thread
From: Taylor Blau @ 2020-05-04 21:58 UTC (permalink / raw)
  To: Jeff King
  Cc: Đoàn Trần Công Danh, git, Jeff Hostetler,
	Johannes Schindelin

On Mon, May 04, 2020 at 12:23:11PM -0400, Jeff King wrote:
> On Mon, May 04, 2020 at 10:49:31PM +0700, Đoàn Trần Công Danh wrote:
>
> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> > index fd4df939b5..ea43b03092 100644
> > --- a/.github/workflows/main.yml
> > +++ b/.github/workflows/main.yml
> > @@ -1,6 +1,18 @@
> >  name: CI/PR
> >
> > -on: [push, pull_request]
> > +on:
> > +  pull_request:
> > +    branches:
> > +      - '**'
>
> Doing "**" here makes sense to catch everything (it would be even better
> if we could just say "everything with a pull request" by omitting the
> branch filter entirely, but maybe that's not possible).
>
> > +    tags:
> > +      - '*'
>
> Would we want that here, too? I guess nobody is likely to push
> "foo/v1.2.3".
>
> Or on the flip side, would we want to tighten this? If I push a tag
> "wip", I probably don't want it built. Probably the right rule is
> "annotated tags only", but I suspect that's not possible.
>
> > +  push:
> > +    branches:
> > +      - maint
> > +      - master
> > +      - next
> > +      - jch
> > +      - pu
>
> What happened to "for-ci" (presumably "for-ci/**")?

Huh; I'm not sure that I'm sold on the idea of a 'for-ci' namespace
here. In addition to running 'make test' on patches locally before I
send them, I find it tremendously convenient for GitHub to run them for
me when I push 'tb/' branches up to 'ttaylorr/git'.

So, while the above is more-or-less what I'd expect the monitored list
of branches to look like (at least, ignoring the missing 'for-ci/**'
bits), I wish that I could also build every branch that I push up to my
fork.

Of course, I don't want to maintain a one-patch difference between
ttaylorr/git@master and git/git@master, so I wonder if we could get a
little more creative with these rules and actually run Actions on
*every* branch, but introduce a new first step which stops the rest of
the actions run (so that in practice we're not running CI on
non-integration branches in Junio's tree).

I figure that we need something more flexible than the 'push.branches'
list, but I'd be very curious to hear if something like what I'm
describing is possible.

> -Peff

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 21:58             ` Taylor Blau
@ 2020-05-04 22:52               ` Junio C Hamano
  2020-05-04 23:15                 ` Taylor Blau
  2020-05-04 23:36               ` Jeff King
  1 sibling, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-04 22:52 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Jeff King, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

Taylor Blau <me@ttaylorr.com> writes:

> Huh; I'm not sure that I'm sold on the idea of a 'for-ci' namespace
> here. In addition to running 'make test' on patches locally before I
> send them, I find it tremendously convenient for GitHub to run them for
> me when I push 'tb/' branches up to 'ttaylorr/git'.
>
> So, while the above is more-or-less what I'd expect the monitored list
> of branches to look like (at least, ignoring the missing 'for-ci/**'
> bits), I wish that I could also build every branch that I push up to my
> fork.
>
> Of course, I don't want to maintain a one-patch difference between
> ttaylorr/git@master and git/git@master, so I wonder if we could get a
> little more creative with these rules and actually run Actions on
> *every* branch, but introduce a new first step which stops the rest of
> the actions run (so that in practice we're not running CI on
> non-integration branches in Junio's tree).

Hmph, what are we trying to avoid by using the for-ci/ convention?

If this is only a reaction to what I said earlier (i.e. "building
everything in github.com/gitster/git/ has no value to me"), then I
suspect it may be an over-engineered solution to a problem that does
not exist, and harms people like you.  I could just go there and
turn off GitHub Actions for that repository instead.

Or are there more issues being addressed with the "testing branches
are opt-in, unless a pull request against git/git explicitly says it
is ready to be tested" approach?


^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 22:52               ` Junio C Hamano
@ 2020-05-04 23:15                 ` Taylor Blau
  2020-05-04 23:35                   ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Taylor Blau @ 2020-05-04 23:15 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, Jeff King, Đoàn Trần Công Danh,
	git, Jeff Hostetler, Johannes Schindelin

On Mon, May 04, 2020 at 03:52:44PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Huh; I'm not sure that I'm sold on the idea of a 'for-ci' namespace
> > here. In addition to running 'make test' on patches locally before I
> > send them, I find it tremendously convenient for GitHub to run them for
> > me when I push 'tb/' branches up to 'ttaylorr/git'.
> >
> > So, while the above is more-or-less what I'd expect the monitored list
> > of branches to look like (at least, ignoring the missing 'for-ci/**'
> > bits), I wish that I could also build every branch that I push up to my
> > fork.
> >
> > Of course, I don't want to maintain a one-patch difference between
> > ttaylorr/git@master and git/git@master, so I wonder if we could get a
> > little more creative with these rules and actually run Actions on
> > *every* branch, but introduce a new first step which stops the rest of
> > the actions run (so that in practice we're not running CI on
> > non-integration branches in Junio's tree).
>
> Hmph, what are we trying to avoid by using the for-ci/ convention?
>
> If this is only a reaction to what I said earlier (i.e. "building
> everything in github.com/gitster/git/ has no value to me"), then I
> suspect it may be an over-engineered solution to a problem that does
> not exist, and harms people like you.  I could just go there and
> turn off GitHub Actions for that repository instead.

It is a reaction to that, yes. It would be nice to have a middle-ground
where you can run Actions on the official integration branches, but
contributors such as myself run Actions on *every* branch of their fork.

It does feel over-engineered, yes. I would not be surprised if Actions
supports something like this more directly, and I just don't know about
it.

> Or are there more issues being addressed with the "testing branches
> are opt-in, unless a pull request against git/git explicitly says it
> is ready to be tested" approach?

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 23:15                 ` Taylor Blau
@ 2020-05-04 23:35                   ` Jeff King
  2020-05-05  0:24                     ` Junio C Hamano
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-04 23:35 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Junio C Hamano, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

On Mon, May 04, 2020 at 05:15:44PM -0600, Taylor Blau wrote:

> > If this is only a reaction to what I said earlier (i.e. "building
> > everything in github.com/gitster/git/ has no value to me"), then I
> > suspect it may be an over-engineered solution to a problem that does
> > not exist, and harms people like you.  I could just go there and
> > turn off GitHub Actions for that repository instead.
> 
> It is a reaction to that, yes. It would be nice to have a middle-ground
> where you can run Actions on the official integration branches, but
> contributors such as myself run Actions on *every* branch of their fork.

The problem is that there is no way for people to _not_ run on every
branch of their fork, then. It is either every branch or no branches.

By choosing a prefix, it gives people the option to choose. But it does
place the burden on them of adding an extra refspec:

  refs/heads/*:refs/heads/for-ci/*

or similar. I don't like it, but I don't see any other way to let people
choose some branches but not others (that doesn't involve a one-line
yaml change at the beginning of every branch, which is an even higher
burden).

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 21:58             ` Taylor Blau
  2020-05-04 22:52               ` Junio C Hamano
@ 2020-05-04 23:36               ` Jeff King
  2020-05-05  0:20                 ` Taylor Blau
  1 sibling, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-04 23:36 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Đoàn Trần Công Danh, git, Jeff Hostetler,
	Johannes Schindelin

On Mon, May 04, 2020 at 03:58:24PM -0600, Taylor Blau wrote:

> Huh; I'm not sure that I'm sold on the idea of a 'for-ci' namespace
> here. In addition to running 'make test' on patches locally before I
> send them, I find it tremendously convenient for GitHub to run them for
> me when I push 'tb/' branches up to 'ttaylorr/git'.
> 
> So, while the above is more-or-less what I'd expect the monitored list
> of branches to look like (at least, ignoring the missing 'for-ci/**'
> bits), I wish that I could also build every branch that I push up to my
> fork.
> 
> Of course, I don't want to maintain a one-patch difference between
> ttaylorr/git@master and git/git@master, so I wonder if we could get a
> little more creative with these rules and actually run Actions on
> *every* branch, but introduce a new first step which stops the rest of
> the actions run (so that in practice we're not running CI on
> non-integration branches in Junio's tree).

I don't understand what that would accomplish. If we ran the actions on
every branch but stopped the run, then you wouldn't get the CI results
you want. What am I missing?

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 23:36               ` Jeff King
@ 2020-05-05  0:20                 ` Taylor Blau
  2020-05-05 16:43                   ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Taylor Blau @ 2020-05-05  0:20 UTC (permalink / raw)
  To: Jeff King
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

On Mon, May 04, 2020 at 07:36:34PM -0400, Jeff King wrote:
> On Mon, May 04, 2020 at 03:58:24PM -0600, Taylor Blau wrote:
>
> > Huh; I'm not sure that I'm sold on the idea of a 'for-ci' namespace
> > here. In addition to running 'make test' on patches locally before I
> > send them, I find it tremendously convenient for GitHub to run them for
> > me when I push 'tb/' branches up to 'ttaylorr/git'.
> >
> > So, while the above is more-or-less what I'd expect the monitored list
> > of branches to look like (at least, ignoring the missing 'for-ci/**'
> > bits), I wish that I could also build every branch that I push up to my
> > fork.
> >
> > Of course, I don't want to maintain a one-patch difference between
> > ttaylorr/git@master and git/git@master, so I wonder if we could get a
> > little more creative with these rules and actually run Actions on
> > *every* branch, but introduce a new first step which stops the rest of
> > the actions run (so that in practice we're not running CI on
> > non-integration branches in Junio's tree).
>
> I don't understand what that would accomplish. If we ran the actions on
> every branch but stopped the run, then you wouldn't get the CI results
> you want. What am I missing?

That on forks of git/git we *would't* stop the run for non-integration
branches, i.e., that we'd have something like:

  * Actions is running on all branches, of all forks, all the time.

  * If a branch is pushed to git/git, and it's not an integration
    branch, the run is aborted early.

  * If a branch is pushed to a fork of git/git, it's allowed to run no
    matter what.

Maybe it's an over-engineered solution to a problem that we don't
have, though. Junio seems happy to simply disable Actions on git/git.
OTOH, I think it would be OK to skip the second bullet point, and
have Junio disable Actions notifications.

/shrug

> -Peff

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 23:35                   ` Jeff King
@ 2020-05-05  0:24                     ` Junio C Hamano
  0 siblings, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-05  0:24 UTC (permalink / raw)
  To: Jeff King
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> On Mon, May 04, 2020 at 05:15:44PM -0600, Taylor Blau wrote:
>
>> > If this is only a reaction to what I said earlier (i.e. "building
>> > everything in github.com/gitster/git/ has no value to me"), then I
>> > suspect it may be an over-engineered solution to a problem that does
>> > not exist, and harms people like you.  I could just go there and
>> > turn off GitHub Actions for that repository instead.
>> 
>> It is a reaction to that, yes. It would be nice to have a middle-ground
>> where you can run Actions on the official integration branches, but
>> contributors such as myself run Actions on *every* branch of their fork.
>
> The problem is that there is no way for people to _not_ run on every
> branch of their fork, then. It is either every branch or no branches.

OK.

> .... I don't like it, but I don't see any other way to let people
> choose some branches but not others (that doesn't involve a one-line
> yaml change at the beginning of every branch, which is an even higher
> burden).

True, and having to mark a commit object with "do not run CI on this",
which is what I heard Travis does, is even worse.


^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-04 16:23           ` Jeff King
  2020-05-04 21:58             ` Taylor Blau
@ 2020-05-05  0:34             ` Đoàn Trần Công Danh
  1 sibling, 0 replies; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-05  0:34 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Jeff Hostetler, Johannes Schindelin

On 2020-05-04 12:23:11-0400, Jeff King <peff@peff.net> wrote:
> On Mon, May 04, 2020 at 10:49:31PM +0700, Đoàn Trần Công Danh wrote:
> Doing "**" here makes sense to catch everything (it would be even better
> if we could just say "everything with a pull request" by omitting the
> branch filter entirely, but maybe that's not possible).

When I was doing this, I couldn't create a fork of my fork to check
the syntax for GitHub PR. So, I pick a safe step.

Turn out, I can create PR against my own fork.
And, it's possible to omit the branch filter entirely.

> > +    tags:
> > +      - '*'
> 
> Would we want that here, too? I guess nobody is likely to push
> "foo/v1.2.3".
> 
> Or on the flip side, would we want to tighten this? If I push a tag
> "wip", I probably don't want it built. Probably the right rule is
> "annotated tags only", but I suspect that's not possible.

From my reading, GitHub Actions only accepts filter by refname.

From GitHub manual, we can limit tags selection by: 
-------------8<------
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index adf8824af1..9bba0ce068 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -11,7 +11,8 @@ on:
       - pu
       - 'for-ci**'
     tags:
-      - '*'
+      - '**'
+      - '!**wip**'
 
 env:
   DEVELOPER: 1
---------------->8-------------

But, I'm running into GitHub internal error with this snippet.
I'll look into it later.

> 
> > +  push:
> > +    branches:
> > +      - maint
> > +      - master
> > +      - next
> > +      - jch
> > +      - pu
> 
> What happened to "for-ci" (presumably "for-ci/**")?

Hm, it was lost when I created a lot of branch for testing and I send the incorrect one out.
It'll be correct next time.

-- 
Danh

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 2/2] SubmittingPatches: advertise GitHub Actions CI
  2020-05-04 16:37           ` Junio C Hamano
@ 2020-05-05  0:46             ` Đoàn Trần Công Danh
  0 siblings, 0 replies; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-05  0:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff Hostetler, Johannes Schindelin, Jeff King

On 2020-05-04 09:37:11-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
> 
> > +Alternately, you can use GitHub Actions (which supports testing your changes
> > +on Linux, macOS, and Windows) by pushing into a branch whose name starts
> > +with "for-ci/" or opening a GitHub's Pull Request against
> > +https://github.com/git/git.git
> 
> Can you tighten the description of "for-ci/" a bit?  It's not like
> the convention is offered in _any_ repository, but it is active only
> if you push to a fork of git.git, right?

The convention will work in any repository with
"$TOPDIR/.github/workflows/*.yml" exists.
Since GitHub Actions will look into the file ".github/workflows/*.yml"
in current repository.

> If your fork is a fork of
> a fork, what happens (e.g. github.com/gitster/git is marked as a
> fork of git/git; when somebody forks from gitster/git, would they
> also get the for-ci/ convention)?

Yes, they'll get that convention.

-- 
Danh

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v3 0/3] Provide option to opt in/out GitHub Actions
  2020-05-04 15:49       ` [PATCH v2 0/2] Limit GitHub Actions to designated branches Đoàn Trần Công Danh
  2020-05-04 15:49         ` [PATCH v2 1/2] CI: limit " Đoàn Trần Công Danh
  2020-05-04 15:49         ` [PATCH v2 2/2] SubmittingPatches: advertise GitHub Actions CI Đoàn Trần Công Danh
@ 2020-05-05 16:26         ` Đoàn Trần Công Danh
  2020-05-05 16:26           ` [PATCH v3 1/3] SubmittingPatches: advertise GitHub Actions CI Đoàn Trần Công Danh
                             ` (3 more replies)
  2 siblings, 4 replies; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-05 16:26 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Jeff Hostetler,
	Johannes Schindelin, Jeff King, Taylor Blau, Junio C Hamano

It seems like there're 2 schools for GitHub Actions,
- Opt-in by using some specific patterns
- Opt-out by using some specific patterns

I tried to help everyone happy, but with my current knowledge, I couldn't.

This series should be 2 patches only.
I provide the [3/3] as a fix-up for [2/3].

Should we conclude that it's over-engineered, please drop [3/3].
Should we think it's OK to go that route, please fix-up [3/3] into [2/3].

Hopefully, we can get into some agreement on this matter.

I'm not include range-diff/inter-diff against v2 since I reorder the patches,
and [3/3] is meant for discussion.


Đoàn Trần Công Danh (3):
  SubmittingPatches: advertise GitHub Actions CI
  CI: limit GitHub Actions to designated branches
  fixup! CI: limit GitHub Actions to designated branches

 .github/workflows/main.yml      | 82 ++++++++++++++++++++++++++++++++-
 Documentation/SubmittingPatches |  6 +++
 2 files changed, 87 insertions(+), 1 deletion(-)

-- 
2.26.2.672.g232c24e857


^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v3 1/3] SubmittingPatches: advertise GitHub Actions CI
  2020-05-05 16:26         ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Đoàn Trần Công Danh
@ 2020-05-05 16:26           ` Đoàn Trần Công Danh
  2020-05-05 16:47             ` Jeff King
  2020-05-05 16:26           ` [PATCH v3 2/3] CI: limit GitHub Actions to designated branches Đoàn Trần Công Danh
                             ` (2 subsequent siblings)
  3 siblings, 1 reply; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-05 16:26 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Jeff Hostetler,
	Johannes Schindelin, Jeff King, Taylor Blau, Junio C Hamano

From 889cacb689 (ci: configure GitHub Actions for CI/PR, 2020-04-11),
GitHub Actions was introduced as an alternative CI system for Git
project.

Let's advertise it to Git's contributors to help them test Git on
various platforms before submitting to Git.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 Documentation/SubmittingPatches | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 4515cab519..8686318550 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -78,6 +78,11 @@ on open source projects), you can use their Travis CI integration to
 test your changes on Linux, Mac (and hopefully soon Windows).  See
 GitHub-Travis CI hints section for details.
 
+Alternately, you can use GitHub Actions (which supports testing your changes
+on Linux, macOS, and Windows) by pushing into a branch in your fork
+or opening a GitHub's Pull Request against
+https://github.com/git/git.git or a fork of that repository.
+
 Do not forget to update the documentation to describe the updated
 behavior and make sure that the resulting documentation set formats
 well (try the Documentation/doc-diff script).
-- 
2.26.2.672.g232c24e857


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v3 2/3] CI: limit GitHub Actions to designated branches
  2020-05-05 16:26         ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Đoàn Trần Công Danh
  2020-05-05 16:26           ` [PATCH v3 1/3] SubmittingPatches: advertise GitHub Actions CI Đoàn Trần Công Danh
@ 2020-05-05 16:26           ` Đoàn Trần Công Danh
  2020-05-05 16:51             ` Jeff King
  2020-05-05 18:49             ` Junio C Hamano
  2020-05-05 16:26           ` [PATCH v3 3/3] fixup! " Đoàn Trần Công Danh
  2020-05-05 17:01           ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Jeff King
  3 siblings, 2 replies; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-05 16:26 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Jeff Hostetler,
	Johannes Schindelin, Jeff King, Taylor Blau, Junio C Hamano

Git's maintainer usually don't have enough time to debug the failure of
invidual feature branch, they usually want to look at intergration
branches only.

Contributors now can have GitHub Actions as an opt-in option,
should they want to check their code, they will push into designated
branch.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 .github/workflows/main.yml      | 14 +++++++++++++-
 Documentation/SubmittingPatches |  3 ++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index fd4df939b5..9bba0ce068 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -1,6 +1,18 @@
 name: CI/PR
 
-on: [push, pull_request]
+on:
+  pull_request:
+  push:
+    branches:
+      - maint
+      - master
+      - next
+      - jch
+      - pu
+      - 'for-ci**'
+    tags:
+      - '**'
+      - '!**wip**'
 
 env:
   DEVELOPER: 1
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 8686318550..e516b080df 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -79,7 +79,8 @@ test your changes on Linux, Mac (and hopefully soon Windows).  See
 GitHub-Travis CI hints section for details.
 
 Alternately, you can use GitHub Actions (which supports testing your changes
-on Linux, macOS, and Windows) by pushing into a branch in your fork
+on Linux, macOS, and Windows) by pushing into a branch whose name starts
+with "for-ci", or a tag whose name doesn't have `wip`,
 or opening a GitHub's Pull Request against
 https://github.com/git/git.git or a fork of that repository.
 
-- 
2.26.2.672.g232c24e857


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v3 3/3] fixup! CI: limit GitHub Actions to designated branches
  2020-05-05 16:26         ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Đoàn Trần Công Danh
  2020-05-05 16:26           ` [PATCH v3 1/3] SubmittingPatches: advertise GitHub Actions CI Đoàn Trần Công Danh
  2020-05-05 16:26           ` [PATCH v3 2/3] CI: limit GitHub Actions to designated branches Đoàn Trần Công Danh
@ 2020-05-05 16:26           ` Đoàn Trần Công Danh
  2020-05-05 18:59             ` Junio C Hamano
  2020-05-05 17:01           ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Jeff King
  3 siblings, 1 reply; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-05 16:26 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Jeff Hostetler,
	Johannes Schindelin, Jeff King, Taylor Blau, Junio C Hamano

Here is the patch to for discussion.

Should we want to enable GitHub Action for all repo-s, except git/git
and gitster/git. We'll want to have this patch fix-up to the previous
one.

With this patch merged, contributors will need to opt-out by push to
a branch/tag with "wip" anywhere in the refname.

Note that, integration branches (maint, master, next, jch, pu) will
always be built, regardless of repo.
Since the current condition is ugly enough, and I find it's very hard to
wrap my head around those condition.

Should this patch get fixing up into previous one, please remove the second
paragraph in the body of previous patch.

Thanks.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 .github/workflows/main.yml      | 80 ++++++++++++++++++++++++++++++---
 Documentation/SubmittingPatches |  4 +-
 2 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 9bba0ce068..81dfa3d228 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -4,12 +4,8 @@ on:
   pull_request:
   push:
     branches:
-      - maint
-      - master
-      - next
-      - jch
-      - pu
-      - 'for-ci**'
+      - '**'
+      - '!**wip**'
     tags:
       - '**'
       - '!**wip**'
@@ -19,6 +15,15 @@ env:
 
 jobs:
   windows-build:
+    if: >-
+      ${{
+      (github.repository_owner != 'git' && github.repository_owner != 'gitster') ||
+      github.ref == 'refs/heads/maint' ||
+      github.ref == 'refs/heads/master' ||
+      github.ref == 'refs/heads/next' ||
+      github.ref == 'refs/heads/jch' ||
+      github.ref == 'refs/heads/pu'
+      }}
     runs-on: windows-latest
     steps:
     - uses: actions/checkout@v1
@@ -43,6 +48,15 @@ jobs:
         name: windows-artifacts
         path: artifacts
   windows-test:
+    if: >-
+      ${{
+      (github.repository_owner != 'git' && github.repository_owner != 'gitster') ||
+      github.ref == 'refs/heads/maint' ||
+      github.ref == 'refs/heads/master' ||
+      github.ref == 'refs/heads/next' ||
+      github.ref == 'refs/heads/jch' ||
+      github.ref == 'refs/heads/pu'
+      }}
     runs-on: windows-latest
     needs: [windows-build]
     strategy:
@@ -82,6 +96,15 @@ jobs:
         name: failed-tests-windows
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   vs-build:
+    if: >-
+      ${{
+      (github.repository_owner != 'git' && github.repository_owner != 'gitster') ||
+      github.ref == 'refs/heads/maint' ||
+      github.ref == 'refs/heads/master' ||
+      github.ref == 'refs/heads/next' ||
+      github.ref == 'refs/heads/jch' ||
+      github.ref == 'refs/heads/pu'
+      }}
     env:
       MSYSTEM: MINGW64
       NO_PERL: 1
@@ -130,6 +153,15 @@ jobs:
         name: vs-artifacts
         path: artifacts
   vs-test:
+    if: >-
+      ${{
+      (github.repository_owner != 'git' && github.repository_owner != 'gitster') ||
+      github.ref == 'refs/heads/maint' ||
+      github.ref == 'refs/heads/master' ||
+      github.ref == 'refs/heads/next' ||
+      github.ref == 'refs/heads/jch' ||
+      github.ref == 'refs/heads/pu'
+      }}
     runs-on: windows-latest
     needs: [vs-build]
     strategy:
@@ -166,6 +198,15 @@ jobs:
                           ${{matrix.nr}} 10 t[0-9]*.sh)
         "@
   regular:
+    if: >-
+      ${{
+      (github.repository_owner != 'git' && github.repository_owner != 'gitster') ||
+      github.ref == 'refs/heads/maint' ||
+      github.ref == 'refs/heads/master' ||
+      github.ref == 'refs/heads/next' ||
+      github.ref == 'refs/heads/jch' ||
+      github.ref == 'refs/heads/pu'
+      }}
     strategy:
       matrix:
         vector:
@@ -201,6 +242,15 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   dockerized:
+    if: >-
+      ${{
+      (github.repository_owner != 'git' && github.repository_owner != 'gitster') ||
+      github.ref == 'refs/heads/maint' ||
+      github.ref == 'refs/heads/master' ||
+      github.ref == 'refs/heads/next' ||
+      github.ref == 'refs/heads/jch' ||
+      github.ref == 'refs/heads/pu'
+      }}
     strategy:
       matrix:
         vector:
@@ -225,6 +275,15 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   static-analysis:
+    if: >-
+      ${{
+      (github.repository_owner != 'git' && github.repository_owner != 'gitster') ||
+      github.ref == 'refs/heads/maint' ||
+      github.ref == 'refs/heads/master' ||
+      github.ref == 'refs/heads/next' ||
+      github.ref == 'refs/heads/jch' ||
+      github.ref == 'refs/heads/pu'
+      }}
     env:
       jobname: StaticAnalysis
     runs-on: ubuntu-latest
@@ -233,6 +292,15 @@ jobs:
     - run: ci/install-dependencies.sh
     - run: ci/run-static-analysis.sh
   documentation:
+    if: >-
+      ${{
+      (github.repository_owner != 'git' && github.repository_owner != 'gitster') ||
+      github.ref == 'refs/heads/maint' ||
+      github.ref == 'refs/heads/master' ||
+      github.ref == 'refs/heads/next' ||
+      github.ref == 'refs/heads/jch' ||
+      github.ref == 'refs/heads/pu'
+      }}
     env:
       jobname: Documentation
     runs-on: ubuntu-latest
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index e516b080df..a2e6a3d2ca 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -79,8 +79,8 @@ test your changes on Linux, Mac (and hopefully soon Windows).  See
 GitHub-Travis CI hints section for details.
 
 Alternately, you can use GitHub Actions (which supports testing your changes
-on Linux, macOS, and Windows) by pushing into a branch whose name starts
-with "for-ci", or a tag whose name doesn't have `wip`,
+on Linux, macOS, and Windows) by pushing into a branch or tag
+whose name doesn't have `wip`,
 or opening a GitHub's Pull Request against
 https://github.com/git/git.git or a fork of that repository.
 
-- 
2.26.2.672.g232c24e857


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-05  0:20                 ` Taylor Blau
@ 2020-05-05 16:43                   ` Jeff King
  2020-05-05 17:57                     ` Junio C Hamano
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-05 16:43 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Đoàn Trần Công Danh, git, Jeff Hostetler,
	Johannes Schindelin

On Mon, May 04, 2020 at 06:20:55PM -0600, Taylor Blau wrote:

> > > Of course, I don't want to maintain a one-patch difference between
> > > ttaylorr/git@master and git/git@master, so I wonder if we could get a
> > > little more creative with these rules and actually run Actions on
> > > *every* branch, but introduce a new first step which stops the rest of
> > > the actions run (so that in practice we're not running CI on
> > > non-integration branches in Junio's tree).
> >
> > I don't understand what that would accomplish. If we ran the actions on
> > every branch but stopped the run, then you wouldn't get the CI results
> > you want. What am I missing?
> 
> That on forks of git/git we *would't* stop the run for non-integration
> branches, i.e., that we'd have something like:

Ah, so the first step is "if we are gitster/git, then do not run", not
"do not run".

But that is missing the point of the exercise, no? The question of what
gitster/git should do was a side conversation. The purpose of Dscho's
original patch and Danh's followup was to allow anybody to choose which
branches in their own fork. I.e.:

>   * Actions is running on all branches, of all forks, all the time.

This is how it works now, and is the exact thing we are trying to fix.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v3 1/3] SubmittingPatches: advertise GitHub Actions CI
  2020-05-05 16:26           ` [PATCH v3 1/3] SubmittingPatches: advertise GitHub Actions CI Đoàn Trần Công Danh
@ 2020-05-05 16:47             ` Jeff King
  2020-05-05 16:59               ` Đoàn Trần Công Danh
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-05 16:47 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Jeff Hostetler, Johannes Schindelin, Taylor Blau, Junio C Hamano

On Tue, May 05, 2020 at 11:26:39PM +0700, Đoàn Trần Công Danh wrote:

> From 889cacb689 (ci: configure GitHub Actions for CI/PR, 2020-04-11),
> GitHub Actions was introduced as an alternative CI system for Git
> project.
> 
> Let's advertise it to Git's contributors to help them test Git on
> various platforms before submitting to Git.

I think this makes sense. Two things:

> +Alternately, you can use GitHub Actions (which supports testing your changes
> +on Linux, macOS, and Windows) by pushing into a branch in your fork
> +or opening a GitHub's Pull Request against
> +https://github.com/git/git.git or a fork of that repository.

Probably "GitHub Pull Request" would be more idiomatic English.

Do people need to enable Actions on their forks for the branch push to
work? I didn't need to for my fork of git/git, but I'm not sure if that's
because I was playing with Actions months ago and forgot, or if having
actions enabled on the parent repo makes it work.

When I made a new repository that was not connected, I had to explicitly
enable Actions on the site before it would run the workflow file.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v3 2/3] CI: limit GitHub Actions to designated branches
  2020-05-05 16:26           ` [PATCH v3 2/3] CI: limit GitHub Actions to designated branches Đoàn Trần Công Danh
@ 2020-05-05 16:51             ` Jeff King
  2020-05-05 17:05               ` Đoàn Trần Công Danh
  2020-05-05 18:49             ` Junio C Hamano
  1 sibling, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-05 16:51 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Jeff Hostetler, Johannes Schindelin, Taylor Blau, Junio C Hamano

On Tue, May 05, 2020 at 11:26:40PM +0700, Đoàn Trần Công Danh wrote:

> -on: [push, pull_request]
> +on:
> +  pull_request:
> +  push:
> +    branches:
> +      - maint
> +      - master
> +      - next
> +      - jch
> +      - pu
> +      - 'for-ci**'

Should this be "for-ci/**", or are we intending for "for-ci-foo" to
work? I'd suspect anybody who uses this would use a full directory
namespace in a refspec (like "refs/heads/*:refs/heads/for-ci/*"). It
might be simpler conceptually to only support that.

> +    tags:
> +      - '**'
> +      - '!**wip**'

IMHO this "wip" match is going too far. That was the name in the example
I used, but really it could have been anything. I think we should
either:

  - just build all tags; it usually takes special effort to push them up
    anyway, so a one-off "just mark this spot" tag likely wouldn't get
    pushed anyway

  - just build v[0-9]*, which would catch actual releases

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v3 1/3] SubmittingPatches: advertise GitHub Actions CI
  2020-05-05 16:47             ` Jeff King
@ 2020-05-05 16:59               ` Đoàn Trần Công Danh
  2020-05-05 17:07                 ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-05 16:59 UTC (permalink / raw)
  To: Jeff King
  Cc: git, Jeff Hostetler, Johannes Schindelin, Taylor Blau, Junio C Hamano

On 2020-05-05 12:47:40-0400, Jeff King <peff@peff.net> wrote:
> On Tue, May 05, 2020 at 11:26:39PM +0700, Đoàn Trần Công Danh wrote:
> 
> > From 889cacb689 (ci: configure GitHub Actions for CI/PR, 2020-04-11),
> > GitHub Actions was introduced as an alternative CI system for Git
> > project.
> > 
> > Let's advertise it to Git's contributors to help them test Git on
> > various platforms before submitting to Git.
> 
> I think this makes sense. Two things:
> 
> > +Alternately, you can use GitHub Actions (which supports testing your changes
> > +on Linux, macOS, and Windows) by pushing into a branch in your fork
> > +or opening a GitHub's Pull Request against
> > +https://github.com/git/git.git or a fork of that repository.
> 
> Probably "GitHub Pull Request" would be more idiomatic English.

I guess you're right.

Well, I'm not native English speaker.
I was thinking, this kind of Pull Request is specific to GitHub,
and it's different from git-request-pull(1), So, I use "'s" :)

> Do people need to enable Actions on their forks for the branch push to
> work? I didn't need to for my fork of git/git, but I'm not sure if that's
> because I was playing with Actions months ago and forgot, or if having
> actions enabled on the parent repo makes it work.

It's a long time from the first time I tinker with GitHub Actions,
I couldn't recall if I need to enable it explicitly.

I look into GitHub support pages[1] and GitHub tells me that
GitHub Actions is enabled by default for everyone, and every repo.
We need to disable GitHub Actions explicitly, if we don't want it.


> When I made a new repository that was not connected, I had to explicitly
> enable Actions on the site before it would run the workflow file.

It seems like GitHub Actions will be triggered automatically if GitHub
finds any files in "$TOPDIR/.github/workflows/*.yml"

[1]: https://help.github.com/en/actions/getting-started-with-github-actions/about-github-actions#disabling-or-limiting-github-actions-for-your-repository-or-organization

-- 
Danh

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v3 0/3] Provide option to opt in/out GitHub Actions
  2020-05-05 16:26         ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Đoàn Trần Công Danh
                             ` (2 preceding siblings ...)
  2020-05-05 16:26           ` [PATCH v3 3/3] fixup! " Đoàn Trần Công Danh
@ 2020-05-05 17:01           ` Jeff King
  3 siblings, 0 replies; 62+ messages in thread
From: Jeff King @ 2020-05-05 17:01 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Jeff Hostetler, Johannes Schindelin, Taylor Blau, Junio C Hamano

On Tue, May 05, 2020 at 11:26:38PM +0700, Đoàn Trần Công Danh wrote:

> It seems like there're 2 schools for GitHub Actions,
> - Opt-in by using some specific patterns
> - Opt-out by using some specific patterns
> 
> I tried to help everyone happy, but with my current knowledge, I couldn't.

Yeah, I agree it's not possible to make anyone happy.

I think the special handling of git/git and gitster/git in patch 3 is
not worth doing. That case is already easy to solve because it's two
repos: just turn off Actions in gitster/git if Junio isn't looking at
the results, and leave them on in git/git.

The real problem we're trying to solve is people with forks who might
want some branches with CI, but not others. Right now the only solution
is for them to make a separate repository, and push to both.

To do that we need some way of getting per-branch information into the
Actions workflow. The ideal solution would just pull extra data from
some other ref or config within the repository, but there doesn't seem
to be a way to do that.

So we're left with:

  - changing the tree of the branch; this is painful to manage, and the
    tree changes risk leaking out when we send out patches, etc. I think
    we all agree this is too horrible.

  - annotating the commit message with [skip ci] or similar. This is
    less painful, but still risks leaking out during patch mailing, etc.

  - sticking data in the branch name. This is the "for-ci" thing, which
    requires you to opt-in to CI. But it could be -wip or -no-ci or
    whatever, which would flip the default.

The third one seems like the only workable thing, so we have to pick a
direction. I prefer "for-ci" to "no-ci" because it's easier to add a
refspec to push to for-ci/* than it is to remember to put "no-ci" in
every branch name that you _don't_ want built.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v3 2/3] CI: limit GitHub Actions to designated branches
  2020-05-05 16:51             ` Jeff King
@ 2020-05-05 17:05               ` Đoàn Trần Công Danh
  2020-05-05 17:11                 ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-05 17:05 UTC (permalink / raw)
  To: Jeff King
  Cc: git, Jeff Hostetler, Johannes Schindelin, Taylor Blau, Junio C Hamano

On 2020-05-05 12:51:25-0400, Jeff King <peff@peff.net> wrote:
> On Tue, May 05, 2020 at 11:26:40PM +0700, Đoàn Trần Công Danh wrote:
> 
> > -on: [push, pull_request]
> > +on:
> > +  pull_request:
> > +  push:
> > +    branches:
> > +      - maint
> > +      - master
> > +      - next
> > +      - jch
> > +      - pu
> > +      - 'for-ci**'
> 
> Should this be "for-ci/**", or are we intending for "for-ci-foo" to
> work? I'd suspect anybody who uses this would use a full directory
> namespace in a refspec (like "refs/heads/*:refs/heads/for-ci/*"). It
> might be simpler conceptually to only support that.

I made this because I saw someone mentioned that they would like to
push to 'for-ci' and expect it works for them.

I guess it may be better to have:

	- for-ci
	- for-ci/**

> 
> > +    tags:
> > +      - '**'
> > +      - '!**wip**'
> 
> IMHO this "wip" match is going too far. That was the name in the example
> I used, but really it could have been anything. I think we should
> either:
> 
>   - just build all tags; it usually takes special effort to push them up
>     anyway, so a one-off "just mark this spot" tag likely wouldn't get
>     pushed anyway
> 
>   - just build v[0-9]*, which would catch actual releases

This sounds better, just build "v[0-9]*" and ignore everything else,
with this pattern, I think we don't need to advertise tag to our
users. And our maintainer shouldn't worry about it, since our
maintainer will (likely) only push v[0-9]* tagged code, anyway.

-- 
Danh

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v3 1/3] SubmittingPatches: advertise GitHub Actions CI
  2020-05-05 16:59               ` Đoàn Trần Công Danh
@ 2020-05-05 17:07                 ` Jeff King
  0 siblings, 0 replies; 62+ messages in thread
From: Jeff King @ 2020-05-05 17:07 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Jeff Hostetler, Johannes Schindelin, Taylor Blau, Junio C Hamano

On Tue, May 05, 2020 at 11:59:33PM +0700, Đoàn Trần Công Danh wrote:

> > > +Alternately, you can use GitHub Actions (which supports testing your changes
> > > +on Linux, macOS, and Windows) by pushing into a branch in your fork
> > > +or opening a GitHub's Pull Request against
> > > +https://github.com/git/git.git or a fork of that repository.
> > 
> > Probably "GitHub Pull Request" would be more idiomatic English.
> 
> I guess you're right.
> 
> Well, I'm not native English speaker.
> I was thinking, this kind of Pull Request is specific to GitHub,
> and it's different from git-request-pull(1), So, I use "'s" :)

Using the possessive "'s" would imply you're talking about GitHub's
feature itself. But you're talking about "a" pull request, so I think
you want to just GitHub as an adjective, which would not generally be
possessive. English is quirky. :)

> > When I made a new repository that was not connected, I had to explicitly
> > enable Actions on the site before it would run the workflow file.
> 
> It seems like GitHub Actions will be triggered automatically if GitHub
> finds any files in "$TOPDIR/.github/workflows/*.yml"

It definitely wasn't for me. Try creating a new repo and going to the
"Actions" tab. I get a "Get Started with GitHub Actions" page. If I push
up a copy of git.git's master, then I get "Workflows aren't being run in
this repository" with a big green button to enable.

I'm just not sure if that happens for people who fork git/git instead of
making a new repo. I'm reluctant to delete and remake my fork, but I
guess it wouldn't be too hard to re-create.

I think it's OK to leave it out for now. Even if this is how it works
for a fork, it's not hard to discover what to do if you click the
Actions tab.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v3 2/3] CI: limit GitHub Actions to designated branches
  2020-05-05 17:05               ` Đoàn Trần Công Danh
@ 2020-05-05 17:11                 ` Jeff King
  0 siblings, 0 replies; 62+ messages in thread
From: Jeff King @ 2020-05-05 17:11 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Jeff Hostetler, Johannes Schindelin, Taylor Blau, Junio C Hamano

On Wed, May 06, 2020 at 12:05:46AM +0700, Đoàn Trần Công Danh wrote:

> > Should this be "for-ci/**", or are we intending for "for-ci-foo" to
> > work? I'd suspect anybody who uses this would use a full directory
> > namespace in a refspec (like "refs/heads/*:refs/heads/for-ci/*"). It
> > might be simpler conceptually to only support that.
> 
> I made this because I saw someone mentioned that they would like to
> push to 'for-ci' and expect it works for them.
> 
> I guess it may be better to have:
> 
> 	- for-ci
> 	- for-ci/**

Ah, that makes sense.

Yeah, the double-rule looks fine to me, but understanding that use, the
original "for-ci**" is OK to me, too.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-05 16:43                   ` Jeff King
@ 2020-05-05 17:57                     ` Junio C Hamano
  2020-05-05 18:24                       ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-05 17:57 UTC (permalink / raw)
  To: Jeff King
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> But that is missing the point of the exercise, no? The question of what
> gitster/git should do was a side conversation. The purpose of Dscho's
> original patch and Danh's followup was to allow anybody to choose which
> branches in their own fork. I.e.:
>
>>   * Actions is running on all branches, of all forks, all the time.
>
> This is how it works now, and is the exact thing we are trying to fix.

Thanks for clarifying and refocusing the discussion.  I am onboard.

It seems to me that there are only two and half approaches, then:

 - Branch-build is opt-in; only branches that match selected, known,
   and fixed patterns will be built (e.g. 'maint', 'maint-*',
   'master', 'next', 'pu', and 'for-ci/*').

 - Branch-build is opt-out; branches that match selected, known, and
   fixed patterns will be excluded (e.g., '*-wip').

 - If you do not want your branches want to be skipped, you need to
   tweak the commit at the tip (e.g. mark with '[skip ci]' log
   message, munging .github/workflows/ in the tree).

The last one is only there for completeness.  I do not think mucking
with the objects recorded in the history, whether it is a tweaked
log message or tweaked tree contents, is a good way to do this.

More random ideas...

Would it be too much hassle to use notes for a thing like this?
Perhaps push out with refs/notes/skip-ci note attached to a commit
you do not want to be built?  I have a feeling that it gives way
overkill flexibility with little gain (probably too cumbersome to
manage).

Does push into GitHub repository offer an ability to pass arbitrary
push option, to which actions that trigger "on: push" event can
react?


^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-05 17:57                     ` Junio C Hamano
@ 2020-05-05 18:24                       ` Jeff King
  2020-05-05 21:04                         ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-05 18:24 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

On Tue, May 05, 2020 at 10:57:39AM -0700, Junio C Hamano wrote:

> Would it be too much hassle to use notes for a thing like this?
> Perhaps push out with refs/notes/skip-ci note attached to a commit
> you do not want to be built?  I have a feeling that it gives way
> overkill flexibility with little gain (probably too cumbersome to
> manage).

I think using notes would be a hassle. This config is really associated
with a branch, not a particular config (so you'd have to make sure they
propagate across rebases, etc).

But _if_ we can read from other refs in the repository, I would be very
happy if we parsed config out of refs/ci/branches or something. It feels
like that's something that ought to be possible, but I haven't quite
figured out a way to do it.

Really all we want is some kind of per-repo variable storage where the
values aren't baked into the tree. There is a "secrets" system that can
be used for this, though it kind of feels like an abuse of the concept.

> Does push into GitHub repository offer an ability to pass arbitrary
> push option, to which actions that trigger "on: push" event can
> react?

No, I don't think so.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v3 2/3] CI: limit GitHub Actions to designated branches
  2020-05-05 16:26           ` [PATCH v3 2/3] CI: limit GitHub Actions to designated branches Đoàn Trần Công Danh
  2020-05-05 16:51             ` Jeff King
@ 2020-05-05 18:49             ` Junio C Hamano
  1 sibling, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-05 18:49 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Jeff Hostetler, Johannes Schindelin, Jeff King, Taylor Blau

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> Git's maintainer usually don't have enough time to debug the failure of
> invidual feature branch, they usually want to look at intergration
> branches only.

Please please please do not make this thing about me.  I do not have
Actions enabled at https://github.com/gitster/git/ repository anyway.

Instead, please focus on making the end result useful for general
contributors.

> Contributors now can have GitHub Actions as an opt-in option,
> should they want to check their code, they will push into designated
> branch.

OK, so this is an opt-in approach as you outlined in the cover
letter.

> +  pull_request:
> +  push:
> +    branches:
> +      - maint
> +      - master
> +      - next
> +      - jch
> +      - pu
> +      - 'for-ci**'
> +    tags:
> +      - '**'
> +      - '!**wip**'

I agree with the agreement you and Peff reached about for-ci plus
for-ci/** patterns and building only v[0-9] tags in your discussion
downthread.

Thaks.

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v3 3/3] fixup! CI: limit GitHub Actions to designated branches
  2020-05-05 16:26           ` [PATCH v3 3/3] fixup! " Đoàn Trần Công Danh
@ 2020-05-05 18:59             ` Junio C Hamano
  0 siblings, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-05 18:59 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Jeff Hostetler, Johannes Schindelin, Jeff King, Taylor Blau

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> Should we want to enable GitHub Action for all repo-s, except git/git
> and gitster/git. We'll want to have this patch fix-up to the previous
> one.

Just to get everybody on the same page,

	git/git has only the primary integration branches and tags
	gitster/git has broken out individual topics

We do want to build everything in git/git repository.

I do not have Actions enabled for gitster/git repository.

I expect general contributors to fork from the former, push their
work on their own branches (not the copies of the public integration
branches like 'master', 'next', etc.) so that they can throw a pull
request at git/git.

>  jobs:
>    windows-build:
> +    if: >-
> +      ${{
> +      (github.repository_owner != 'git' && github.repository_owner != 'gitster') ||
> +      github.ref == 'refs/heads/maint' ||
> +      github.ref == 'refs/heads/master' ||
> +      github.ref == 'refs/heads/next' ||
> +      github.ref == 'refs/heads/jch' ||
> +      github.ref == 'refs/heads/pu'
> +      }}

Ialready asked not to make this about me.  You do not want to search
and replace 'gitster' with 'peff' or 'dscho' or whatever string,
only because/when I chose to retire.  Such an occasion shouldn't be
a significant event to the project's codebase.

Thanks.



^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-05 18:24                       ` Jeff King
@ 2020-05-05 21:04                         ` Jeff King
  2020-05-05 21:29                           ` Junio C Hamano
                                             ` (2 more replies)
  0 siblings, 3 replies; 62+ messages in thread
From: Jeff King @ 2020-05-05 21:04 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

On Tue, May 05, 2020 at 02:24:18PM -0400, Jeff King wrote:

> But _if_ we can read from other refs in the repository, I would be very
> happy if we parsed config out of refs/ci/branches or something. It feels
> like that's something that ought to be possible, but I haven't quite
> figured out a way to do it.

OK, I finally figured this out. The result is the patch below, which I
think should make everybody happy. Or at least has the ability to do so
if they're willing to push a config ref. ;)

-- >8 --
Subject: [PATCH] ci: allow per-branch config for GitHub Actions

Depending on the workflows of individual developers, it can either be
convenient or annoying that our GitHub Actions CI jobs are run on every
branch. As an example of annoying: if you carry many half-finished
work-in-progress branches and rebase them frequently against master,
you'd get tons of failure reports that aren't interesting (not to
mention the wasted CPU).

This commit adds a new job which checks a special ref within the
repository for CI config, and (optionally) causes all of the other jobs
to be skipped.

There have been a few alternatives discussed:

One option is to carry information in the commit itself about whether it
should be tested, either in the tree itself (changing the workflow YAML
file) or in the commit message (a "[skip ci]" flag or similar). But
these are frustrating and error-prone to use:

  - you have to manually apply them to each branch that you want to mark

  - it's easy for them to leak into other workflows, like emailing patches

We could likewise try to get some information from the branch name. But
that leads to debates about whether the default should be "off" or "on",
and overriding still ends up somewhat awkward. If we default to "on",
you have to remember to name your branches appropriately to skip CI. And
if "off", you end up having to contort your branch names or duplicate
your pushes with an extra refspec.

By comparison, this commit's solution lets you specify your config once
and forget about it, and all of the data is off in its own ref, where it
can be changed by individual forks without touching the main tree.

I used refs/ci/config as the config ref, which should be a commit whose
tree contains various config files (right now the only one is
"ref-whitelist"). It was intentional to avoid refs/heads/ here so we
don't conflict with any branch workflows. But it does make it a little
awkward to edit, since you can't check it out directly.

Right now the logic is to run CI for all branches by default, unless a
whitelist exists, in which case the branch must be mentioned there
(using its fully qualified ref name). We could easily add in a
blacklist, as well. Or since we're running a shell in a VM, we really
could just run "./allow-ref $refname" and let individual forks specify
whatever shell code they like.

Signed-off-by: Jeff King <peff@peff.net>
---
After writing that, I think we probably ought to just do the allow-ref
thing from the start, and skip this whitelist logic. Then we should
never need to change this workflow file again. People can implement
whatever weird custom logic they want to.

 .github/workflows/main.yml | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index fd4df939b5..51f4ff6e89 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -6,7 +6,29 @@ env:
   DEVELOPER: 1
 
 jobs:
+  check-ci:
+      runs-on: ubuntu-latest
+      outputs:
+        enabled: ${{ steps.check-ref.outputs.enabled }}
+      steps:
+        - uses: actions/checkout@v2
+          continue-on-error: true
+          with:
+            ref: refs/ci/config
+        - id: check-ref
+          name: check whether CI is enabled for ref
+          run: |
+            enabled=yes
+            if test -e ref-whitelist &&
+               ! grep '^${{ github.ref }}$' ref-whitelist
+            then
+              enabled=no
+            fi
+            echo "::set-output name=enabled::$enabled"
+
   windows-build:
+    needs: check-ci
+    if: needs.check-ci.outputs.enabled == 'yes'
     runs-on: windows-latest
     steps:
     - uses: actions/checkout@v1
@@ -70,6 +92,8 @@ jobs:
         name: failed-tests-windows
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   vs-build:
+    needs: check-ci
+    if: needs.check-ci.outputs.enabled == 'yes'
     env:
       MSYSTEM: MINGW64
       NO_PERL: 1
@@ -154,6 +178,8 @@ jobs:
                           ${{matrix.nr}} 10 t[0-9]*.sh)
         "@
   regular:
+    needs: check-ci
+    if: needs.check-ci.outputs.enabled == 'yes'
     strategy:
       matrix:
         vector:
@@ -189,6 +215,8 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   dockerized:
+    needs: check-ci
+    if: needs.check-ci.outputs.enabled == 'yes'
     strategy:
       matrix:
         vector:
@@ -213,6 +241,8 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   static-analysis:
+    needs: check-ci
+    if: needs.check-ci.outputs.enabled == 'yes'
     env:
       jobname: StaticAnalysis
     runs-on: ubuntu-latest
@@ -221,6 +251,8 @@ jobs:
     - run: ci/install-dependencies.sh
     - run: ci/run-static-analysis.sh
   documentation:
+    needs: check-ci
+    if: needs.check-ci.outputs.enabled == 'yes'
     env:
       jobname: Documentation
     runs-on: ubuntu-latest
-- 
2.26.2.962.g87ee755179


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-05 21:04                         ` Jeff King
@ 2020-05-05 21:29                           ` Junio C Hamano
  2020-05-05 21:58                             ` Jeff King
  2020-05-06 15:09                             ` Johannes Schindelin
  2020-05-06  0:46                           ` Đoàn Trần Công Danh
  2020-05-07 16:20                           ` [PATCH v2] ci: allow per-branch config for GitHub Actions Jeff King
  2 siblings, 2 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-05 21:29 UTC (permalink / raw)
  To: Jeff King
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> On Tue, May 05, 2020 at 02:24:18PM -0400, Jeff King wrote:
>
>> But _if_ we can read from other refs in the repository, I would be very
>> happy if we parsed config out of refs/ci/branches or something. It feels
>> like that's something that ought to be possible, but I haven't quite
>> figured out a way to do it.
>
> OK, I finally figured this out. The result is the patch below, which I
> think should make everybody happy. Or at least has the ability to do so
> if they're willing to push a config ref. ;)

That sounds good.

> Subject: [PATCH] ci: allow per-branch config for GitHub Actions
>
> Depending on the workflows of individual developers, it can either be
> convenient or annoying that our GitHub Actions CI jobs are run on every
> branch. As an example of annoying: if you carry many half-finished
> work-in-progress branches and rebase them frequently against master,
> you'd get tons of failure reports that aren't interesting (not to
> mention the wasted CPU).

OK.

> This commit adds a new job which checks a special ref within the
> repository for CI config, and (optionally) causes all of the other jobs
> to be skipped.

Nice---that way, all existing jobs do not even need to know about
the special controlling ref.

> Right now the logic is to run CI for all branches by default, unless a
> whitelist exists, in which case the branch must be mentioned there
> (using its fully qualified ref name).

So there is no wildcard?  Not really complaining, but am wondering.

> We could easily add in a
> blacklist, as well.

OK.

> Or since we're running a shell in a VM, we really
> could just run "./allow-ref $refname" and let individual forks specify
> whatever shell code they like.

I presume that you are saying "checking out the tree of refs/ci/config
and there is a program allow-ref that can tell which one to run ci on"?

> After writing that, I think we probably ought to just do the allow-ref
> thing from the start, and skip this whitelist logic. Then we should
> never need to change this workflow file again. People can implement
> whatever weird custom logic they want to.

Probably.

>  jobs:
> +  check-ci:
> +      runs-on: ubuntu-latest
> +      outputs:
> +        enabled: ${{ steps.check-ref.outputs.enabled }}
> +      steps:
> +        - uses: actions/checkout@v2
> +          continue-on-error: true
> +          with:
> +            ref: refs/ci/config
> +        - id: check-ref
> +          name: check whether CI is enabled for ref
> +          run: |
> +            enabled=yes
> +            if test -e ref-whitelist &&
> +               ! grep '^${{ github.ref }}$' ref-whitelist
> +            then
> +              enabled=no
> +            fi
> +            echo "::set-output name=enabled::$enabled"
> +
>    windows-build:
> +    needs: check-ci
> +    if: needs.check-ci.outputs.enabled == 'yes'
>      runs-on: windows-latest
>      steps:
>      - uses: actions/checkout@v1

Oh, quite nice.  Simple and clean.


^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-05 21:29                           ` Junio C Hamano
@ 2020-05-05 21:58                             ` Jeff King
  2020-05-05 22:28                               ` Junio C Hamano
  2020-05-06 15:09                             ` Johannes Schindelin
  1 sibling, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-05 21:58 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

On Tue, May 05, 2020 at 02:29:09PM -0700, Junio C Hamano wrote:

> > This commit adds a new job which checks a special ref within the
> > repository for CI config, and (optionally) causes all of the other jobs
> > to be skipped.
> 
> Nice---that way, all existing jobs do not even need to know about
> the special controlling ref.

Yep. Also, it only has to run once. It's not too expensive, but it does
seem to take a few seconds to run.

I think the VM setup is usually pretty quick due to the way Actions is
implemented. I made sure we avoid cloning the whole normal git.git tree,
but we still have to clone the individual config branch. It seems to
take ~3-5s to run that job. I'd have liked to make it even cheaper for
branches which don't want CI (less for latency, but more just because I
hate to waste GitHub's resources), but I think that's the best we can
do. And for cases where we actually run CI, 3s is a drop in the bucket. ;)

> > Right now the logic is to run CI for all branches by default, unless a
> > whitelist exists, in which case the branch must be mentioned there
> > (using its fully qualified ref name).
> 
> So there is no wildcard?  Not really complaining, but am wondering.

Correct, there's no wildcard. It would be easy to add one, now that this
gets us to a working shell environment with the refname available. And
probably worth doing if we don't just go the "execute a program from the
config tree" route.

> > Or since we're running a shell in a VM, we really
> > could just run "./allow-ref $refname" and let individual forks specify
> > whatever shell code they like.
> 
> I presume that you are saying "checking out the tree of refs/ci/config
> and there is a program allow-ref that can tell which one to run ci on"?

Yes, exactly.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-05 21:58                             ` Jeff King
@ 2020-05-05 22:28                               ` Junio C Hamano
  0 siblings, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-05 22:28 UTC (permalink / raw)
  To: Jeff King
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

Jeff King <peff@peff.net> writes:

>> > Or since we're running a shell in a VM, we really
>> > could just run "./allow-ref $refname" and let individual forks specify
>> > whatever shell code they like.
>> 
>> I presume that you are saying "checking out the tree of refs/ci/config
>> and there is a program allow-ref that can tell which one to run ci on"?
>
> Yes, exactly.

I guess a simple example implementation of allow-ref script, with a
bit of instruction to tell people how to initialize a separate and
otherwise empty repository with just a (possibly customized) copy of
the script in it, and push its 'master' branch to refs/ci/config of
their Git fork, would be a way to go, then?

Sounds simple enough.  Cycles to spin up a VM that adds 3 seconds
latency, only to know that a branch won't need to be built, does
sound like a bit unfortunate, though.

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-05 21:04                         ` Jeff King
  2020-05-05 21:29                           ` Junio C Hamano
@ 2020-05-06  0:46                           ` Đoàn Trần Công Danh
  2020-05-06  3:56                             ` Junio C Hamano
  2020-05-07 16:20                           ` [PATCH v2] ci: allow per-branch config for GitHub Actions Jeff King
  2 siblings, 1 reply; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-06  0:46 UTC (permalink / raw)
  To: Jeff King
  Cc: Junio C Hamano, Taylor Blau, git, Jeff Hostetler, Johannes Schindelin

On 2020-05-05 17:04:51-0400, Jeff King <peff@peff.net> wrote:
> We could likewise try to get some information from the branch name. But
> that leads to debates about whether the default should be "off" or "on",
> and overriding still ends up somewhat awkward. If we default to "on",
> you have to remember to name your branches appropriately to skip CI. And
> if "off", you end up having to contort your branch names or duplicate
> your pushes with an extra refspec.
> 
> By comparison, this commit's solution lets you specify your config once
> and forget about it, and all of the data is off in its own ref, where it
> can be changed by individual forks without touching the main tree.

How about supports the best of both worlds.

Let's say support wildcard 'wip/**' for opt-out.
And use "./allow-ref" to filter everything that passed the wildcard.

> I used refs/ci/config as the config ref, which should be a commit whose
> tree contains various config files (right now the only one is
> "ref-whitelist"). It was intentional to avoid refs/heads/ here so we
> don't conflict with any branch workflows. But it does make it a little
> awkward to edit, since you can't check it out directly.
> 
> Right now the logic is to run CI for all branches by default, unless a
> whitelist exists, in which case the branch must be mentioned there
> (using its fully qualified ref name). We could easily add in a
> blacklist, as well. Or since we're running a shell in a VM, we really
> could just run "./allow-ref $refname" and let individual forks specify
> whatever shell code they like.

Should we go with this route, here is a fix-up patch for your,
(after cherry-pick my [1/3])
--------------------8<-------------------
Subject: [PATCH] fixup! ci: allow per-branch config for GitHub Actions

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 .github/workflows/main.yml      |  3 +--
 Documentation/SubmittingPatches | 12 ++++++++++++
 contrib/ci-config-allow-ref     |  9 +++++++++
 3 files changed, 22 insertions(+), 2 deletions(-)
 create mode 100755 contrib/ci-config-allow-ref

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 51f4ff6e89..08217c5ed8 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -19,8 +19,7 @@ jobs:
           name: check whether CI is enabled for ref
           run: |
             enabled=yes
-            if test -e ref-whitelist &&
-               ! grep '^${{ github.ref }}$' ref-whitelist
+            if test -x allow-ref && ! ./allow-ref '${{ github.ref }}'
             then
               enabled=no
             fi
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 8686318550..8175424929 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -82,6 +82,18 @@ Alternately, you can use GitHub Actions (which supports testing your changes
 on Linux, macOS, and Windows) by pushing into a branch in your fork
 or opening a GitHub's Pull Request against
 https://github.com/git/git.git or a fork of that repository.
+In the event that you only want to trigger GitHub Actions for specific
+refname, you can create an executable file named `allow-ref` in
+`refs/ci/config`. Those below steps may help you:
+--------------
+$ git checkout --orphan ci-config
+$ cp contrib/ci-config-allow-ref allow-ref
+$ $EDITOR allow-ref
+$ git rm -rf .
+$ git commit allow-ref
+$ git push <your-fork> HEAD:refs/ci/config
+--------------
+
 
 Do not forget to update the documentation to describe the updated
 behavior and make sure that the resulting documentation set formats
diff --git a/contrib/ci-config-allow-ref b/contrib/ci-config-allow-ref
new file mode 100755
index 0000000000..b53e9ddbd0
--- /dev/null
+++ b/contrib/ci-config-allow-ref
@@ -0,0 +1,9 @@
+#!/bin/sh
+# Sample filter for GitHub Actions
+# GitHub Actions will run if and only if this script exit with zero status
+
+REFNAME="$1"
+
+case "$REFNAME" in
+refs/heads/no-ci*) exit 1 ;;
+esac
-- 
2.26.2.672.g232c24e857


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-06  0:46                           ` Đoàn Trần Công Danh
@ 2020-05-06  3:56                             ` Junio C Hamano
  2020-05-06 14:25                               ` Đoàn Trần Công Danh
  0 siblings, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-06  3:56 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: Jeff King, Taylor Blau, git, Jeff Hostetler, Johannes Schindelin

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 8686318550..8175424929 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -82,6 +82,18 @@ Alternately, you can u
>  on Linux, macOS, and Windows) by pushing into a branch in your fork
>  or opening a GitHub's Pull Request against
>  https://github.com/git/git.git or a fork of that repository.
> +In the event that you only want to trigger GitHub Actions for specific
> +refname, you can create an executable file named `allow-ref` in
> +`refs/ci/config`. Those below steps may help you:

"These steps below" or "The follwoing steps", perhaps.

> +--------------
> +$ git checkout --orphan ci-config
> +$ cp contrib/ci-config-allow-ref allow-ref
> +$ $EDITOR allow-ref
> +$ git rm -rf .

This sounds horrible.  You just nuked the entire files in the
working tree you use for your everyday Git hacking to edit a
single file.

> +$ git commit allow-ref

If allow-ref were added to the index before this "git commit", the
previous "git rm -rf ." would have removed it.  Since the surviving
allow-ref file must have been untracked, this "git commit" would not
commit anything.  Forgot to "git add"?

> +$ git push <your-fork> HEAD:refs/ci/config
> +--------------

>  Do not forget to update the documentation to describe the updated
>  behavior and make sure that the resulting documentation set formats
> diff --git a/contrib/ci-config-allow-ref b/contrib/ci-config-allow-ref
> new file mode 100755
> index 0000000000..b53e9ddbd0
> --- /dev/null
> +++ b/contrib/ci-config-allow-ref
> @@ -0,0 +1,9 @@
> +#!/bin/sh
> +# Sample filter for GitHub Actions
> +# GitHub Actions will run if and only if this script exit with zero status

s/exit/exits/;

As the instruction above says, we should set the example and
describe the behaviour we implemented initially.  Something as basic
like ...

    # Build any branch other than those whose name begins with "no-ci"

> +
> +REFNAME="$1"
> +
> +case "$REFNAME" in
> +refs/heads/no-ci*) exit 1 ;;
> +esac


^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-06  3:56                             ` Junio C Hamano
@ 2020-05-06 14:25                               ` Đoàn Trần Công Danh
  2020-05-06 16:31                                 ` Junio C Hamano
  0 siblings, 1 reply; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-06 14:25 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jeff King, Taylor Blau, git, Jeff Hostetler, Johannes Schindelin

On 2020-05-05 20:56:58-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
> > +--------------
> > +$ git checkout --orphan ci-config
> > +$ cp contrib/ci-config-allow-ref allow-ref
> > +$ $EDITOR allow-ref
> > +$ git rm -rf .
> 
> This sounds horrible.  You just nuked the entire files in the
> working tree you use for your everyday Git hacking to edit a
> single file.

It isn't that horrible as it sounds. It only removes the files that are
currently added in index, which is the same with tracked files in old
branch, and we can get it back by switching back to old branch.

I decided to make an orphanage branch because I would like to save
time and network bandwidth for the "check-ci" jobs. Since GitHub will
fetch only single branch in GitHub Actions:

	/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +refs/ci/config:refs/ci/config

I wonder whether the "git rm -rf ." makes that block sounds horrible?
If that is the case, we can use the experimental git-switch(1)
instead, it's doing more-or-less the same (or is it the same?) with
"git checkout --orphan" and "git rm -rf ."

-----------------
$ cp contrib/ci-config-allow-ref.sample allow-ref
$ git switch --orphan ci-config
$ edit allow-ref
$ git add allow-ref
$ git commit
-----------------

Note: I changed `$EDITOR` to `edit` to match other example of `edit`.

> As the instruction above says, we should set the example and
> describe the behaviour we implemented initially.  Something as basic
> like ...
> 
>     # Build any branch other than those whose name begins with "no-ci"

That's definitely better.
And I think we should add `.sample` suffix to this file, too

If all of that's sensible enough, here is the replacement fix-up patch.
--------------------8<------------------
Subject: [PATCH] fixup! ci: allow per-branch config for GitHub Actions

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 .github/workflows/main.yml         |  3 +--
 Documentation/SubmittingPatches    | 12 ++++++++++++
 contrib/ci-config-allow-ref.sample | 12 ++++++++++++
 3 files changed, 25 insertions(+), 2 deletions(-)
 create mode 100755 contrib/ci-config-allow-ref.sample

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 51f4ff6e89..08217c5ed8 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -19,8 +19,7 @@ jobs:
           name: check whether CI is enabled for ref
           run: |
             enabled=yes
-            if test -e ref-whitelist &&
-               ! grep '^${{ github.ref }}$' ref-whitelist
+            if test -x allow-ref && ! ./allow-ref '${{ github.ref }}'
             then
               enabled=no
             fi
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index b916f07f2c..bf06284f29 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -82,6 +82,18 @@ Alternately, you can use GitHub Actions (which supports testing your changes
 on Linux, macOS, and Windows) by pushing into a branch in your fork
 or opening a GitHub Pull Request against
 https://github.com/git/git.git or a fork of that repository.
+In the event that you only want to trigger GitHub Actions for specific
+refname, you can create an executable file named `allow-ref` in
+`refs/ci/config`. The following steps may help you:
+--------------
+$ cp contrib/ci-config-allow-ref.sample allow-ref
+$ git switch --orphan <a-branch-name-of-your-choice>
+$ edit allow-ref
+$ git add allow-ref
+$ git commit allow-ref
+$ git push <your-fork> HEAD:refs/ci/config
+--------------
+
 
 Do not forget to update the documentation to describe the updated
 behavior and make sure that the resulting documentation set formats
diff --git a/contrib/ci-config-allow-ref.sample b/contrib/ci-config-allow-ref.sample
new file mode 100755
index 0000000000..1973e88912
--- /dev/null
+++ b/contrib/ci-config-allow-ref.sample
@@ -0,0 +1,12 @@
+#!/bin/sh
+# Sample filter for GitHub Actions
+# GitHub Actions will run if and only if this script exits with zero status
+
+# This sample filter will ask GitHub Actions to build
+# any branches whose name doesn't start with "no-ci"
+
+REFNAME="$1"
+
+case "$REFNAME" in
+refs/heads/no-ci*) exit 1 ;;
+esac
-- 
2.26.2.672.g232c24e857


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-05 21:29                           ` Junio C Hamano
  2020-05-05 21:58                             ` Jeff King
@ 2020-05-06 15:09                             ` Johannes Schindelin
  2020-05-06 16:26                               ` Junio C Hamano
  2020-05-07 12:01                               ` Đoàn Trần Công Danh
  1 sibling, 2 replies; 62+ messages in thread
From: Johannes Schindelin @ 2020-05-06 15:09 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jeff King, Taylor Blau, Đoàn Trần Công Danh,
	git, Jeff Hostetler

Hi,

On Tue, 5 May 2020, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
> >  jobs:
> > +  check-ci:
> > +      runs-on: ubuntu-latest
> > +      outputs:
> > +        enabled: ${{ steps.check-ref.outputs.enabled }}
> > +      steps:
> > +        - uses: actions/checkout@v2
> > +          continue-on-error: true
> > +          with:
> > +            ref: refs/ci/config
> > +        - id: check-ref
> > +          name: check whether CI is enabled for ref
> > +          run: |
> > +            enabled=yes
> > +            if test -e ref-whitelist &&
> > +               ! grep '^${{ github.ref }}$' ref-whitelist
> > +            then
> > +              enabled=no
> > +            fi
> > +            echo "::set-output name=enabled::$enabled"
> > +
> >    windows-build:
> > +    needs: check-ci
> > +    if: needs.check-ci.outputs.enabled == 'yes'
> >      runs-on: windows-latest
> >      steps:
> >      - uses: actions/checkout@v1
>
> Oh, quite nice.  Simple and clean.

The idea is indeed very neat. I think we can do a bit better with resource
usage by not even bothering to check this branch out. Something along
those lines (sorry, I really would love to have the time to test this...):

      - id: check-ref
        name: check whether CI is enabled for ref
        uses: actions/github-script@0.9.0
        with:
          script: |
            const req = {
              owner: context.repo.owner,
              repo: context.repo.repo,
              ref: "ci/config"
            };

            try {
              req.tree_sha = (await github.git.getRef(req)).data.object.sha;
              (await github.git.getTree(req))
              .tree.filter(e => e.path == 'ref-whitelist').map(e => {
                req.file_sha = e.sha;
              });
              const list = Buffer.from((await github.git.getBlob(req)).data.content, 'base64').toString('UTF-8');
              core.setOutput('enabled', `\n${list}`.indexOf(`\n${{github.ref}}\n`) < 0 ? 'no' : 'yes');
            } catch (e) {
              core.setOutput('enabled', 'yes');
            }

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-06 15:09                             ` Johannes Schindelin
@ 2020-05-06 16:26                               ` Junio C Hamano
  2020-05-07 12:17                                 ` Jeff King
  2020-05-07 12:01                               ` Đoàn Trần Công Danh
  1 sibling, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-06 16:26 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Jeff King, Taylor Blau, Đoàn Trần Công Danh,
	git, Jeff Hostetler

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The idea is indeed very neat. I think we can do a bit better with resource
> usage by not even bothering to check this branch out. Something along
> those lines (sorry, I really would love to have the time to test this...):

Nice.  I view the latest round of Peff's idea "allow-ref" as
"because we are spinning a VM anyway, why not just run an end-user
supplied script and let it decide?" and not as "we must have a
Turing complete flexibility so let's run a script in a VM".  In
other words, it may be overkill and we may strike a better tradeoff
by living with reduced flexibility if there is a way to avoid the
associated cost.

But doesn't this (i.e. uses: actions/github-script) still pay the
cost of spinning up a VM?  How expensive is it to check out a small
tree with a single file, whether it is ref-whitelist or allow-ref?

If the cost to check out a tree is dwarfed, it probably is not worth
pursuing the approach to use a canned "don't customize, just use"
logic in the workflow file to read a configuration file that can
only list full names of allowed refs.

Thanks.

>       - id: check-ref
>         name: check whether CI is enabled for ref
>         uses: actions/github-script@0.9.0
>         with:
>           script: |
>             const req = {
>               owner: context.repo.owner,
>               repo: context.repo.repo,
>               ref: "ci/config"
>             };
>
>             try {
>               req.tree_sha = (await github.git.getRef(req)).data.object.sha;
>               (await github.git.getTree(req))
>               .tree.filter(e => e.path == 'ref-whitelist').map(e => {
>                 req.file_sha = e.sha;
>               });
>               const list = Buffer.from((await github.git.getBlob(req)).data.content, 'base64').toString('UTF-8');
>               core.setOutput('enabled', `\n${list}`.indexOf(`\n${{github.ref}}\n`) < 0 ? 'no' : 'yes');
>             } catch (e) {
>               core.setOutput('enabled', 'yes');
>             }
>
> Ciao,
> Dscho

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-06 14:25                               ` Đoàn Trần Công Danh
@ 2020-05-06 16:31                                 ` Junio C Hamano
  2020-05-07 12:25                                   ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-06 16:31 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: Jeff King, Taylor Blau, git, Jeff Hostetler, Johannes Schindelin

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> On 2020-05-05 20:56:58-0700, Junio C Hamano <gitster@pobox.com> wrote:
>> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
>> > +--------------
>> > +$ git checkout --orphan ci-config
>> > +$ cp contrib/ci-config-allow-ref allow-ref
>> > +$ $EDITOR allow-ref
>> > +$ git rm -rf .
>> 
>> This sounds horrible.  You just nuked the entire files in the
>> working tree you use for your everyday Git hacking to edit a
>> single file.
>
> It isn't that horrible as it sounds. It only removes the files that are
> currently added in index, which is the same with tracked files in old
> branch, and we can get it back by switching back to old branch.
>
> I decided to make an orphanage branch because I would like to save
> time and network bandwidth for the "check-ci" jobs.

I didn't say it is wrong to record a tree with a single file
(allow-ref) in a commit that is pointed by the ci-config ref.

I would have expected you to create such a commit in an otherwise
empty repository, and push into your fork of Git at GitHub.  That
way, you won't have to checkout and/or refresh the index all of the
3800+ files.

> I wonder whether the "git rm -rf ." makes that block sounds horrible?
> If that is the case, we can use the experimental git-switch(1)
> instead, it's doing more-or-less the same (or is it the same?) with
> "git checkout --orphan" and "git rm -rf ."

That does not change anything, as abuse of your primary repository
is what offends me.

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-06 15:09                             ` Johannes Schindelin
  2020-05-06 16:26                               ` Junio C Hamano
@ 2020-05-07 12:01                               ` Đoàn Trần Công Danh
  2020-05-07 12:47                                 ` Đoàn Trần Công Danh
  1 sibling, 1 reply; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-07 12:01 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Junio C Hamano, Jeff King, Taylor Blau, git, Jeff Hostetler

On 2020-05-06 17:09:39+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> The idea is indeed very neat. I think we can do a bit better with resource
> usage by not even bothering to check this branch out. Something along
> those lines (sorry, I really would love to have the time to test this...):

While this can avoid the cost of checking out a whole branch (which
can be mitigated by using an orphan branch with single file),

This still spins up an VM, and actions/github-script run (I think)
nodejs, which is more resource intensive than git and sh script.
Above statement maybe wrong, I'm not interacting much with nodejs.

>       - id: check-ref
>         name: check whether CI is enabled for ref
>         uses: actions/github-script@0.9.0
>         with:
>           script: |
>             const req = {
>               owner: context.repo.owner,
>               repo: context.repo.repo,
>               ref: "ci/config"
>             };
> 
>             try {
>               req.tree_sha = (await github.git.getRef(req)).data.object.sha;
>               (await github.git.getTree(req))
>               .tree.filter(e => e.path == 'ref-whitelist').map(e => {
>                 req.file_sha = e.sha;
>               });
>               const list = Buffer.from((await github.git.getBlob(req)).data.content, 'base64').toString('UTF-8');
>               core.setOutput('enabled', `\n${list}`.indexOf(`\n${{github.ref}}\n`) < 0 ? 'no' : 'yes');

And this `indexOf` will check if our ref (exact) matchs (full line)
with some white-list list, which is very limited.
So people couldn't match by some pattern (grep can work).

I haven't tested, but we may use part of above script to read a single
file from a ref, and add another steps for "grep"/"sh"
I'm not sure if that script will cost more resources than git-checkout
or not. And is that solutions over-engineered?

-- 
Danh

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-06 16:26                               ` Junio C Hamano
@ 2020-05-07 12:17                                 ` Jeff King
  2020-05-07 14:02                                   ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-07 12:17 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin, Taylor Blau,
	Đoàn Trần Công Danh, git, Jeff Hostetler

On Wed, May 06, 2020 at 09:26:53AM -0700, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The idea is indeed very neat. I think we can do a bit better with resource
> > usage by not even bothering to check this branch out. Something along
> > those lines (sorry, I really would love to have the time to test this...):
> 
> Nice.  I view the latest round of Peff's idea "allow-ref" as
> "because we are spinning a VM anyway, why not just run an end-user
> supplied script and let it decide?" and not as "we must have a
> Turing complete flexibility so let's run a script in a VM".  In
> other words, it may be overkill and we may strike a better tradeoff
> by living with reduced flexibility if there is a way to avoid the
> associated cost.

The script is just javascript, which has an eval(). We could probably
let refs/ci/config:allow-ref be a snippet of javascript that we run.

I do prefer the VM environment to snippets of javascript for a few
reasons:

  - shell scripts match the rest of our Git toolbox (though that's a
    personal preference, and for the amount of code we're talking about
    even _I_ can do a little javascript)

  - it's easy to test your code locally by running "./allow-ref foo".
    Whereas emulating the environment in which those scriptlets are run
    is much trickier (e.g., you need a working github-api object)

That said, if this is more lightweight, I think it's worth exploring.

> But doesn't this (i.e. uses: actions/github-script) still pay the
> cost of spinning up a VM?  How expensive is it to check out a small
> tree with a single file, whether it is ref-whitelist or allow-ref?

I suspect this script mechanism may be much cheaper. I don't know the
implementation details, but spinning up a nodejs container to run a
javascript snippet should be much cheaper than a full ubuntu VM running
"git clone" (the clone itself should be super cheap because it's a
shallow single-branch clone of a tree with one file in it, but getting
there is relatively heavy-weight).

But I could be wrong.  It's clear that they're not spinning up a full
ubuntu vm from scratch to serve the requests (since it happens in 3s).
I'll see if I can get something working and do some timings.

The latency isn't incredibly important. This is all happening async, and
either we'll skip CI (in which case you don't care how long it takes,
since you're not waiting on the result) or CI will take many minutes, in
which case 3s is nothing. But I would like to be respectful of the CPU
spent running Actions and be as lightweight as possible.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-06 16:31                                 ` Junio C Hamano
@ 2020-05-07 12:25                                   ` Jeff King
  2020-05-07 18:29                                     ` Junio C Hamano
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-07 12:25 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Đoàn Trần Công Danh, Taylor Blau, git,
	Jeff Hostetler, Johannes Schindelin

On Wed, May 06, 2020 at 09:31:25AM -0700, Junio C Hamano wrote:

> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
> 
> > On 2020-05-05 20:56:58-0700, Junio C Hamano <gitster@pobox.com> wrote:
> >> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
> >> > +--------------
> >> > +$ git checkout --orphan ci-config
> >> > +$ cp contrib/ci-config-allow-ref allow-ref
> >> > +$ $EDITOR allow-ref
> >> > +$ git rm -rf .
> >> 
> >> This sounds horrible.  You just nuked the entire files in the
> >> working tree you use for your everyday Git hacking to edit a
> >> single file.
> >
> > It isn't that horrible as it sounds. It only removes the files that are
> > currently added in index, which is the same with tracked files in old
> > branch, and we can get it back by switching back to old branch.
> >
> > I decided to make an orphanage branch because I would like to save
> > time and network bandwidth for the "check-ci" jobs.
> 
> I didn't say it is wrong to record a tree with a single file
> (allow-ref) in a commit that is pointed by the ci-config ref.
> 
> I would have expected you to create such a commit in an otherwise
> empty repository, and push into your fork of Git at GitHub.  That
> way, you won't have to checkout and/or refresh the index all of the
> 3800+ files.

Yeah, I agree that all of the mechanisms for dealing with the unrelated
history are somewhat awkward. Another issue is that you can't just:

  git clone --single-branch -b refs/ci/config my-config

to work on it, because "-b" wants only heads or tags (we could address
that by putting it in refs/heads/ci-config or similar).

If we do go the javascript route, perhaps it would make sense for
refs/ci/config to be a single blob containing a snippet of javascript
with several functions. And then we could just eval() that and call the
appropriate functions (if defined).

Then a sample can live in the main repo with something like:

  # This file contains functions which will be run by the GitHub
  # Actions CI script.
  #
  # You may customize it for your own fork by modifying it on any branch
  # you like, and installing with:
  #
  #  git push <remote> $(git rev-parse HEAD:ci/config):refs/ci/config
  #
  # [allow_ref() sample definition and documentation....]

That sidesteps most of those issues.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-07 12:01                               ` Đoàn Trần Công Danh
@ 2020-05-07 12:47                                 ` Đoàn Trần Công Danh
  0 siblings, 0 replies; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-07 12:47 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Junio C Hamano, Jeff King, Taylor Blau, git, Jeff Hostetler

On 2020-05-07 19:01:02+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> On 2020-05-06 17:09:39+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > The idea is indeed very neat. I think we can do a bit better with resource
> > usage by not even bothering to check this branch out. Something along
> > those lines (sorry, I really would love to have the time to test this...):
> 
> While this can avoid the cost of checking out a whole branch (which
> can be mitigated by using an orphan branch with single file),
> 
> This still spins up an VM, and actions/github-script run (I think)
> nodejs, which is more resource intensive than git and sh script.
> Above statement maybe wrong, I'm not interacting much with nodejs.

I was wrong, actions/checkout is also using nodejs,
So, this actions/github-script actual reduces the total time for
fetching the file ref-whitelist/ref-blacklist/allow-ref

> >       - id: check-ref
> >         name: check whether CI is enabled for ref
> >         uses: actions/github-script@0.9.0
> >         with:
> >           script: |
> >             const req = {
> >               owner: context.repo.owner,
> >               repo: context.repo.repo,
> >               ref: "ci/config"
> >             };
> > 
> >             try {
> >               req.tree_sha = (await github.git.getRef(req)).data.object.sha;
> >               (await github.git.getTree(req))
> >               .tree.filter(e => e.path == 'ref-whitelist').map(e => {
> >                 req.file_sha = e.sha;
> >               });
> >               const list = Buffer.from((await github.git.getBlob(req)).data.content, 'base64').toString('UTF-8');
> >               core.setOutput('enabled', `\n${list}`.indexOf(`\n${{github.ref}}\n`) < 0 ? 'no' : 'yes');
> 
> And this `indexOf` will check if our ref (exact) matchs (full line)
> with some white-list list, which is very limited.
> So people couldn't match by some pattern (grep can work).
> 
> I haven't tested, but we may use part of above script to read a single
> file from a ref, and add another steps for "grep"/"sh"
> I'm not sure if that script will cost more resources than git-checkout
> or not. And is that solutions over-engineered?

But this point still hold, now, I think using part of above script to
read the file, and allow more custom logic in a separated steps maybe
better solutions.

-- 
Danh

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-07 12:17                                 ` Jeff King
@ 2020-05-07 14:02                                   ` Jeff King
  2020-05-07 18:17                                     ` Junio C Hamano
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-07 14:02 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin, Taylor Blau,
	Đoàn Trần Công Danh, git, Jeff Hostetler

On Thu, May 07, 2020 at 08:17:27AM -0400, Jeff King wrote:

> > But doesn't this (i.e. uses: actions/github-script) still pay the
> > cost of spinning up a VM?  How expensive is it to check out a small
> > tree with a single file, whether it is ref-whitelist or allow-ref?
> 
> I suspect this script mechanism may be much cheaper. I don't know the
> implementation details, but spinning up a nodejs container to run a
> javascript snippet should be much cheaper than a full ubuntu VM running
> "git clone" (the clone itself should be super cheap because it's a
> shallow single-branch clone of a tree with one file in it, but getting
> there is relatively heavy-weight).

Sorry, this is all complete nonsense. There is no magical nodejs
container in Actions. You still have to say "runs-on: ubuntu-latest". So
it's still spinning up that VM and then running inside there.

I just did a timing with three jobs:

  noop:
    runs-on: ubuntu-latest
    steps:
    - run: exit 0

  script:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/github-script@0.9.0
      with:
        script: |
            const req = {
              owner: context.repo.owner,
              repo: context.repo.repo,
              ref: "refs/ci/config"
            };

            try {
              req.tree_sha = (await github.git.getRef(req)).data.object.sha;
              (await github.git.getTree(req))
              .tree.filter(e => e.path == 'ref-whitelist').map(e => {
                req.file_sha = e.sha;
              });
              const list = Buffer.from((await github.git.getBlob(req)).data.content, 'base64').toString('UTF-8');
              core.setOutput('enabled', `\n${list}`.indexOf(`\n${{github.ref}}\n`) < 0 ? 'no' : 'yes');
            } catch (e) {
              core.setOutput('enabled', 'yes');
            }

  checkout:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
      continue-on-error: true
      with:
        ref: refs/ci/config
    - run: ./allow-ref ${{ github.ref }}

and they took 1, 2, and 3 seconds respectively. They spend 2s getting
the environment set up and the actions loaded. So the API one spent less
than 1s on the network, but the single-file checkout spent slightly
more. Given the timing variations I've seen, I wouldn't be surprised if
it sometimes goes the other way. But even if those numbers are accurate,
I don't think the cost difference is enough to force our hand either
way.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v2] ci: allow per-branch config for GitHub Actions
  2020-05-05 21:04                         ` Jeff King
  2020-05-05 21:29                           ` Junio C Hamano
  2020-05-06  0:46                           ` Đoàn Trần Công Danh
@ 2020-05-07 16:20                           ` Jeff King
  2020-05-07 17:00                             ` Taylor Blau
  2020-05-07 19:53                             ` Junio C Hamano
  2 siblings, 2 replies; 62+ messages in thread
From: Jeff King @ 2020-05-07 16:20 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

On Tue, May 05, 2020 at 05:04:51PM -0400, Jeff King wrote:

> Subject: [PATCH] ci: allow per-branch config for GitHub Actions

Here's a "v2" of that patch based on the discussion.

I think it smooths some of the rough edges of the orphan-branch
approach, while still having a cost on par with other suggestions (or at
least ones that truly allow any config; we can check for "for-ci/**" or
something very cheaply, but that implies hard-coding it for everybody).
I think the cost here is acceptable, and it gives us room to add more
features in the future.

If Actions eventually adds per-repo variable storage that can be used in
"if:" conditionals, then we could eventually switch to that. :)

The documentation here should be enough to let people work with it. But
we'd probably want to take Danh's patch to mention Actions in
SubmittingPatches on top (and it possibly could be modified to point to
the ci/config directory).

-- >8 --
Subject: [PATCH] ci: allow per-branch config for GitHub Actions

Depending on the workflows of individual developers, it can either be
convenient or annoying that our GitHub Actions CI jobs are run on every
branch. As an example of annoying: if you carry many half-finished
work-in-progress branches and rebase them frequently against master,
you'd get tons of failure reports that aren't interesting (not to
mention the wasted CPU).

This commit adds a new job which checks a special branch within the
repository for CI config, and then runs a shell script it finds there to
decide whether to skip the rest of the tests. The default will continue
to run tests for all refs if that branch or script is missing.

There have been a few alternatives discussed:

One option is to carry information in the commit itself about whether it
should be tested, either in the tree itself (changing the workflow YAML
file) or in the commit message (a "[skip ci]" flag or similar). But
these are frustrating and error-prone to use:

  - you have to manually apply them to each branch that you want to mark

  - it's easy for them to leak into other workflows, like emailing patches

We could likewise try to get some information from the branch name. But
that leads to debates about whether the default should be "off" or "on",
and overriding still ends up somewhat awkward. If we default to "on",
you have to remember to name your branches appropriately to skip CI. And
if "off", you end up having to contort your branch names or duplicate
your pushes with an extra refspec.

By comparison, this commit's solution lets you specify your config once
and forget about it, and all of the data is off in its own ref, where it
can be changed by individual forks without touching the main tree.

There were a few design decisions that came out of on-list discussion.
I'll summarize here:

 - we could use GitHub's API to retrieve the config ref, rather than a
   real checkout (and then just operate on it via some javascript). We
   still have to spin up a VM and contact GitHub over the network from
   it either way, so it ends up not being much faster. I opted to go
   with shell to keep things similar to our other tools (and really
   could implement allow-refs in any language you want). This also makes
   it easy to test your script locally, and to modify it within the
   context of a normal git.git tree.

 - we could keep the well-known refname out of refs/heads/ to avoid
   cluttering the branch namespace. But that makes it awkward to
   manipulate. By contrast, you can just "git checkout ci-config" to
   make changes.

 - we could assume the ci-config ref has nothing in it except config
   (i.e., a branch unrelated to the rest of git.git). But dealing with
   orphan branches is awkward. Instead, we'll do our best to efficiently
   check out only the ci/config directory using a shallow partial clone,
   which allows your ci-config branch to be just a normal branch, with
   your config changes on top.

 - we could provide a simpler interface, like a static list of ref
   patterns. But we can't get out of spinning up a whole VM anyway, so
   we might as well use that feature to make the config as flexible as
   possible. If we add more config, we should be able to reuse our
   partial-clone to set more outputs.

Signed-off-by: Jeff King <peff@peff.net>
---
 .github/workflows/main.yml  | 42 +++++++++++++++++++++++++++++++++++++
 ci/config/allow-refs.sample | 26 +++++++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100755 ci/config/allow-refs.sample

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index fd4df939b5..802a4bf7cd 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -6,7 +6,39 @@ env:
   DEVELOPER: 1
 
 jobs:
+  ci-config:
+      runs-on: ubuntu-latest
+      outputs:
+        enabled: ${{ steps.check-ref.outputs.enabled }}
+      steps:
+        - name: try to clone ci-config branch
+          continue-on-error: true
+          run: |
+            git -c protocol.version=2 clone \
+              --no-tags \
+              --single-branch \
+              -b ci-config \
+              --depth 1 \
+              --no-checkout \
+              --filter=blob:none \
+              https://github.com/${{ github.repository }} \
+              config-repo &&
+              cd config-repo &&
+              git checkout HEAD -- ci/config
+        - id: check-ref
+          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 }}'
+            then
+              enabled=no
+            fi
+            echo "::set-output name=enabled::$enabled"
+
   windows-build:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     runs-on: windows-latest
     steps:
     - uses: actions/checkout@v1
@@ -70,6 +102,8 @@ jobs:
         name: failed-tests-windows
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   vs-build:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     env:
       MSYSTEM: MINGW64
       NO_PERL: 1
@@ -154,6 +188,8 @@ jobs:
                           ${{matrix.nr}} 10 t[0-9]*.sh)
         "@
   regular:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     strategy:
       matrix:
         vector:
@@ -189,6 +225,8 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   dockerized:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     strategy:
       matrix:
         vector:
@@ -213,6 +251,8 @@ jobs:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
   static-analysis:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     env:
       jobname: StaticAnalysis
     runs-on: ubuntu-latest
@@ -221,6 +261,8 @@ jobs:
     - run: ci/install-dependencies.sh
     - run: ci/run-static-analysis.sh
   documentation:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
     env:
       jobname: Documentation
     runs-on: ubuntu-latest
diff --git a/ci/config/allow-refs.sample b/ci/config/allow-refs.sample
new file mode 100755
index 0000000000..f157f1945a
--- /dev/null
+++ b/ci/config/allow-refs.sample
@@ -0,0 +1,26 @@
+#!/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-refs.sample allow-refs
+#   $EDITOR allow-refs
+#   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.26.2.1005.ge383644752


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ci: allow per-branch config for GitHub Actions
  2020-05-07 16:20                           ` [PATCH v2] ci: allow per-branch config for GitHub Actions Jeff King
@ 2020-05-07 17:00                             ` Taylor Blau
  2020-05-07 17:18                               ` Jeff King
  2020-05-07 19:53                             ` Junio C Hamano
  1 sibling, 1 reply; 62+ messages in thread
From: Taylor Blau @ 2020-05-07 17:00 UTC (permalink / raw)
  To: Jeff King
  Cc: Junio C Hamano, Taylor Blau,
	Đoàn Trần Công Danh, git, Jeff Hostetler,
	Johannes Schindelin

Hi Peff,

On Thu, May 07, 2020 at 12:20:11PM -0400, Jeff King wrote:
> On Tue, May 05, 2020 at 05:04:51PM -0400, Jeff King wrote:
>
> > Subject: [PATCH] ci: allow per-branch config for GitHub Actions
>
> Here's a "v2" of that patch based on the discussion.

I really like this direction. I think that it's a good mix of
flexibility and convenience. I'm happy to push a one-time 'ci-config'
branch to 'ttaylorr/git' and forget about it.

> I think it smooths some of the rough edges of the orphan-branch
> approach, while still having a cost on par with other suggestions (or at
> least ones that truly allow any config; we can check for "for-ci/**" or
> something very cheaply, but that implies hard-coding it for everybody).
> I think the cost here is acceptable, and it gives us room to add more
> features in the future.
>
> If Actions eventually adds per-repo variable storage that can be used in
> "if:" conditionals, then we could eventually switch to that. :)
>
> The documentation here should be enough to let people work with it. But
> we'd probably want to take Danh's patch to mention Actions in
> SubmittingPatches on top (and it possibly could be modified to point to
> the ci/config directory).
>
> -- >8 --
> Subject: [PATCH] ci: allow per-branch config for GitHub Actions
>
> Depending on the workflows of individual developers, it can either be
> convenient or annoying that our GitHub Actions CI jobs are run on every
> branch. As an example of annoying: if you carry many half-finished
> work-in-progress branches and rebase them frequently against master,
> you'd get tons of failure reports that aren't interesting (not to
> mention the wasted CPU).
>
> This commit adds a new job which checks a special branch within the
> repository for CI config, and then runs a shell script it finds there to
> decide whether to skip the rest of the tests. The default will continue
> to run tests for all refs if that branch or script is missing.
>
> There have been a few alternatives discussed:
>
> One option is to carry information in the commit itself about whether it
> should be tested, either in the tree itself (changing the workflow YAML
> file) or in the commit message (a "[skip ci]" flag or similar). But
> these are frustrating and error-prone to use:
>
>   - you have to manually apply them to each branch that you want to mark
>
>   - it's easy for them to leak into other workflows, like emailing patches
>
> We could likewise try to get some information from the branch name. But
> that leads to debates about whether the default should be "off" or "on",
> and overriding still ends up somewhat awkward. If we default to "on",
> you have to remember to name your branches appropriately to skip CI. And
> if "off", you end up having to contort your branch names or duplicate
> your pushes with an extra refspec.
>
> By comparison, this commit's solution lets you specify your config once
> and forget about it, and all of the data is off in its own ref, where it
> can be changed by individual forks without touching the main tree.
>
> There were a few design decisions that came out of on-list discussion.
> I'll summarize here:
>
>  - we could use GitHub's API to retrieve the config ref, rather than a
>    real checkout (and then just operate on it via some javascript). We
>    still have to spin up a VM and contact GitHub over the network from
>    it either way, so it ends up not being much faster. I opted to go
>    with shell to keep things similar to our other tools (and really
>    could implement allow-refs in any language you want). This also makes
>    it easy to test your script locally, and to modify it within the
>    context of a normal git.git tree.
>
>  - we could keep the well-known refname out of refs/heads/ to avoid
>    cluttering the branch namespace. But that makes it awkward to
>    manipulate. By contrast, you can just "git checkout ci-config" to
>    make changes.
>
>  - we could assume the ci-config ref has nothing in it except config
>    (i.e., a branch unrelated to the rest of git.git). But dealing with
>    orphan branches is awkward. Instead, we'll do our best to efficiently
>    check out only the ci/config directory using a shallow partial clone,
>    which allows your ci-config branch to be just a normal branch, with
>    your config changes on top.
>
>  - we could provide a simpler interface, like a static list of ref
>    patterns. But we can't get out of spinning up a whole VM anyway, so
>    we might as well use that feature to make the config as flexible as
>    possible. If we add more config, we should be able to reuse our
>    partial-clone to set more outputs.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  .github/workflows/main.yml  | 42 +++++++++++++++++++++++++++++++++++++
>  ci/config/allow-refs.sample | 26 +++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
>  create mode 100755 ci/config/allow-refs.sample
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index fd4df939b5..802a4bf7cd 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -6,7 +6,39 @@ env:
>    DEVELOPER: 1
>
>  jobs:
> +  ci-config:
> +      runs-on: ubuntu-latest
> +      outputs:
> +        enabled: ${{ steps.check-ref.outputs.enabled }}
> +      steps:
> +        - name: try to clone ci-config branch
> +          continue-on-error: true
> +          run: |
> +            git -c protocol.version=2 clone \
> +              --no-tags \
> +              --single-branch \
> +              -b ci-config \
> +              --depth 1 \
> +              --no-checkout \
> +              --filter=blob:none \
> +              https://github.com/${{ github.repository }} \
> +              config-repo &&
> +              cd config-repo &&
> +              git checkout HEAD -- ci/config
> +        - id: check-ref
> +          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 }}'
> +            then
> +              enabled=no
> +            fi
> +            echo "::set-output name=enabled::$enabled"
> +
>    windows-build:
> +    needs: ci-config
> +    if: needs.ci-config.outputs.enabled == 'yes'

One thing I wonder is whether the downstream 'windows-test' partitions.
I think that it should be fine, since we won't run the dependent
'windows-build', and then 'windows-test' won't have all of its
prerequisites filled.

>      runs-on: windows-latest
>      steps:
>      - uses: actions/checkout@v1
> @@ -70,6 +102,8 @@ jobs:
>          name: failed-tests-windows
>          path: ${{env.FAILED_TEST_ARTIFACTS}}
>    vs-build:
> +    needs: ci-config
> +    if: needs.ci-config.outputs.enabled == 'yes'
>      env:
>        MSYSTEM: MINGW64
>        NO_PERL: 1
> @@ -154,6 +188,8 @@ jobs:
>                            ${{matrix.nr}} 10 t[0-9]*.sh)
>          "@
>    regular:
> +    needs: ci-config
> +    if: needs.ci-config.outputs.enabled == 'yes'
>      strategy:
>        matrix:
>          vector:
> @@ -189,6 +225,8 @@ jobs:
>          name: failed-tests-${{matrix.vector.jobname}}
>          path: ${{env.FAILED_TEST_ARTIFACTS}}
>    dockerized:
> +    needs: ci-config
> +    if: needs.ci-config.outputs.enabled == 'yes'
>      strategy:
>        matrix:
>          vector:
> @@ -213,6 +251,8 @@ jobs:
>          name: failed-tests-${{matrix.vector.jobname}}
>          path: ${{env.FAILED_TEST_ARTIFACTS}}
>    static-analysis:
> +    needs: ci-config
> +    if: needs.ci-config.outputs.enabled == 'yes'
>      env:
>        jobname: StaticAnalysis
>      runs-on: ubuntu-latest
> @@ -221,6 +261,8 @@ jobs:
>      - run: ci/install-dependencies.sh
>      - run: ci/run-static-analysis.sh
>    documentation:
> +    needs: ci-config
> +    if: needs.ci-config.outputs.enabled == 'yes'
>      env:
>        jobname: Documentation
>      runs-on: ubuntu-latest
> diff --git a/ci/config/allow-refs.sample b/ci/config/allow-refs.sample
> new file mode 100755
> index 0000000000..f157f1945a
> --- /dev/null
> +++ b/ci/config/allow-refs.sample
> @@ -0,0 +1,26 @@
> +#!/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

Should we be recommending '--orphan' instead of '-b' here? It looks
like when you clone this branch down that you try to get as few bytes as
possible, so I figure it may be easier to have this be a orphaned
branch.

> +#   cp allow-refs.sample allow-refs
> +#   $EDITOR allow-refs
> +#   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.26.2.1005.ge383644752
>

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ci: allow per-branch config for GitHub Actions
  2020-05-07 17:00                             ` Taylor Blau
@ 2020-05-07 17:18                               ` Jeff King
  0 siblings, 0 replies; 62+ messages in thread
From: Jeff King @ 2020-05-07 17:18 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Junio C Hamano, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

On Thu, May 07, 2020 at 11:00:42AM -0600, Taylor Blau wrote:

> >    windows-build:
> > +    needs: ci-config
> > +    if: needs.ci-config.outputs.enabled == 'yes'
> 
> One thing I wonder is whether the downstream 'windows-test' partitions.
> I think that it should be fine, since we won't run the dependent
> 'windows-build', and then 'windows-test' won't have all of its
> prerequisites filled.

Yes, I intentionally left them out for that reason. It seemed simpler to
just let the skip percolate down the dependency tree.

> > +# 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
> 
> Should we be recommending '--orphan' instead of '-b' here? It looks
> like when you clone this branch down that you try to get as few bytes as
> possible, so I figure it may be easier to have this be a orphaned
> branch.

No, the whole point of doing the partial clone (rather than using
actions/checkout, which doesn't support that) was so people didn't have
to deal with the orphan-branch thing.

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-07 14:02                                   ` Jeff King
@ 2020-05-07 18:17                                     ` Junio C Hamano
  0 siblings, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-07 18:17 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Schindelin, Taylor Blau,
	Đoàn Trần Công Danh, git, Jeff Hostetler

Jeff King <peff@peff.net> writes:

> On Thu, May 07, 2020 at 08:17:27AM -0400, Jeff King wrote:
>
>> > But doesn't this (i.e. uses: actions/github-script) still pay the
>> > cost of spinning up a VM?  How expensive is it to check out a small
>> > tree with a single file, whether it is ref-whitelist or allow-ref?
>> 
>> I suspect this script mechanism may be much cheaper. I don't know the
>> implementation details, but spinning up a nodejs container to run a
>> javascript snippet should be much cheaper than a full ubuntu VM running
>> "git clone" (the clone itself should be super cheap because it's a
>> shallow single-branch clone of a tree with one file in it, but getting
>> there is relatively heavy-weight).
>
> Sorry, this is all complete nonsense. There is no magical nodejs
> container in Actions. You still have to say "runs-on: ubuntu-latest". So
> it's still spinning up that VM and then running inside there.

Ah, I did see "runs-on: ubuntu-latest" in the tutorial for the node
thing, and was very much dissapointed, before I sent that "don't you
still spin up a VM anyway?" response.  Glad to know that I wasn't
totally misreading the documentation, and unhappy that there wasn't
a magic bullet after all X-<.

> and they took 1, 2, and 3 seconds respectively. They spend 2s getting
> the environment set up and the actions loaded. So the API one spent less
> than 1s on the network, but the single-file checkout spent slightly
> more. Given the timing variations I've seen, I wouldn't be surprised if
> it sometimes goes the other way. But even if those numbers are accurate,
> I don't think the cost difference is enough to force our hand either
> way.

Yup, the above tempts me to say "because we are spinning a VM
anyway, why not just run an end-user supplied script and let it
decide?" would be the best approach.

Unless somebody finds a magic bullet, that is, but unfortunately the
nodejs one does not seem to be one.

Thanks.


^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-07 12:25                                   ` Jeff King
@ 2020-05-07 18:29                                     ` Junio C Hamano
  2020-05-07 18:54                                       ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-07 18:29 UTC (permalink / raw)
  To: Jeff King
  Cc: Đoàn Trần Công Danh, Taylor Blau, git,
	Jeff Hostetler, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> Yeah, I agree that all of the mechanisms for dealing with the unrelated
> history are somewhat awkward. Another issue is that you can't just:
>
>   git clone --single-branch -b refs/ci/config my-config
>
> to work on it, because "-b" wants only heads or tags (we could address
> that by putting it in refs/heads/ci-config or similar).

I somehow don't think that it is such a huge issue, because I expect
anybody who has legitimate interest in refs/ci/config to have a full
clone of git.git anyway.  So it is more like defining another
remote.origin.fetch like this:

	 [remote "origin"]
	 	url = https://git.kernel.org/pub/scm/git/git.git/
	 	fetch = +refs/heads/*:refs/remotes/origin/*
	+	fetch = +refs/ci/config:refs/remotes/origin/ci-config

         [remote "publish"]
		url = https://github.com/user/git/


and then do something like:
    
	$ git worktree add -b ci-config ../git-ci-config origin/ci-config
	$ cd ../git-ci-config
	... hack hack hack ...
	$ git push publish ci-config:refs/ci/config

> If we do go the javascript route, perhaps it would make sense for
> refs/ci/config to be a single blob containing a snippet of javascript
> with several functions. And then we could just eval() that and call the
> appropriate functions (if defined).

Yup.

> Then a sample can live in the main repo with something like:
>
>   # This file contains functions which will be run by the GitHub
>   # Actions CI script.
>   #
>   # You may customize it for your own fork by modifying it on any branch
>   # you like, and installing with:
>   #
>   #  git push <remote> $(git rev-parse HEAD:ci/config):refs/ci/config
>   #
>   # [allow_ref() sample definition and documentation....]
>
> That sidesteps most of those issues.

Perhaps.

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-07 18:29                                     ` Junio C Hamano
@ 2020-05-07 18:54                                       ` Jeff King
  2020-05-07 19:33                                         ` Junio C Hamano
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-07 18:54 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Đoàn Trần Công Danh, Taylor Blau, git,
	Jeff Hostetler, Johannes Schindelin

On Thu, May 07, 2020 at 11:29:09AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yeah, I agree that all of the mechanisms for dealing with the unrelated
> > history are somewhat awkward. Another issue is that you can't just:
> >
> >   git clone --single-branch -b refs/ci/config my-config
> >
> > to work on it, because "-b" wants only heads or tags (we could address
> > that by putting it in refs/heads/ci-config or similar).
> 
> I somehow don't think that it is such a huge issue, because I expect
> anybody who has legitimate interest in refs/ci/config to have a full
> clone of git.git anyway.  So it is more like defining another
> remote.origin.fetch like this:
> 
> 	 [remote "origin"]
> 	 	url = https://git.kernel.org/pub/scm/git/git.git/
> 	 	fetch = +refs/heads/*:refs/remotes/origin/*
> 	+	fetch = +refs/ci/config:refs/remotes/origin/ci-config
> 
>          [remote "publish"]
> 		url = https://github.com/user/git/
> 
> 
> and then do something like:
>     
> 	$ git worktree add -b ci-config ../git-ci-config origin/ci-config
> 	$ cd ../git-ci-config
> 	... hack hack hack ...
> 	$ git push publish ci-config:refs/ci/config

I dunno. The duality of refs/ci/config and refs/heads/ci-config is
weird. Your two refspecs overlap on their RHS, and once you push up the
refs/heads/ci-config you created with "git worktree add", they'll start
to conflict (which maybe is OK as long as they're the same).

In the patch I sent out a few hours ago I just caved and made
refs/heads/ci-config the magic ref, in order to avoid complexity.

But it also wouldn't be too bad to say "look, you can store this however
you like in refs/heads/ or not at all, but install it into place with
git push <remote> HEAD:refs/ci/config". The audience is git.git
developers (and even advanced ones whose workflows involve tweaking
their CI config), so I'd like to think they could figure it out.

I have a feeling that fewer than 5 people in the world will end up using
this feature either way. ;)

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2 1/2] CI: limit GitHub Actions to designated branches
  2020-05-07 18:54                                       ` Jeff King
@ 2020-05-07 19:33                                         ` Junio C Hamano
  0 siblings, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-07 19:33 UTC (permalink / raw)
  To: Jeff King
  Cc: Đoàn Trần Công Danh, Taylor Blau, git,
	Jeff Hostetler, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> In the patch I sent out a few hours ago I just caved and made
> refs/heads/ci-config the magic ref, in order to avoid complexity.
>
> But it also wouldn't be too bad to say "look, you can store this however
> you like in refs/heads/ or not at all, but install it into place with
> git push <remote> HEAD:refs/ci/config". The audience is git.git
> developers (and even advanced ones whose workflows involve tweaking
> their CI config), so I'd like to think they could figure it out.
>
> I have a feeling that fewer than 5 people in the world will end up using
> this feature either way. ;)

Yeah, let's not spend too much time and too many cycles to
overengineer it.

Thanks.

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ci: allow per-branch config for GitHub Actions
  2020-05-07 16:20                           ` [PATCH v2] ci: allow per-branch config for GitHub Actions Jeff King
  2020-05-07 17:00                             ` Taylor Blau
@ 2020-05-07 19:53                             ` Junio C Hamano
  2020-05-07 20:46                               ` Jeff King
  1 sibling, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-07 19:53 UTC (permalink / raw)
  To: Jeff King
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> +        - id: check-ref
> +          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 }}'

Is it deliberate that the output from the script is not redirected
to >/dev/null, which would mean they are allowed to do something
that looks like:

		echo "::set-output name=enabled::frotz"

or emit other random ::string-that-affects-github-actions to its
standard output stream?

> +            then
> +              enabled=no
> +            fi
> +            echo "::set-output name=enabled::$enabled"

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ci: allow per-branch config for GitHub Actions
  2020-05-07 19:53                             ` Junio C Hamano
@ 2020-05-07 20:46                               ` Jeff King
  2020-05-07 21:58                                 ` Junio C Hamano
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-07 20:46 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

On Thu, May 07, 2020 at 12:53:09PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +        - id: check-ref
> > +          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 }}'
> 
> Is it deliberate that the output from the script is not redirected
> to >/dev/null, which would mean they are allowed to do something
> that looks like:
> 
> 		echo "::set-output name=enabled::frotz"
> 
> or emit other random ::string-that-affects-github-actions to its
> standard output stream?

It was deliberate in the sense that I would allow them to write useful
messages to the Actions log. If they want to do nonsense like
"::set-output", then it's their foot and their gun.

I don't know if Actions distinguishes between stdout and stderr here
(i.e., if we redirected the script's stdout to stderr, would that
prevent this case or not?).

-Peff

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ci: allow per-branch config for GitHub Actions
  2020-05-07 20:46                               ` Jeff King
@ 2020-05-07 21:58                                 ` Junio C Hamano
  2020-05-08 18:00                                   ` Jeff King
  0 siblings, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-07 21:58 UTC (permalink / raw)
  To: Jeff King
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> It was deliberate in the sense that I would allow them to write useful
> messages to the Actions log. If they want to do nonsense like
> "::set-output", then it's their foot and their gun.

It's not like fooling the framework you laid out here is a
potentially useful attack vector.  We can assume that it is unlikely
for the custom allow-ref to be writing a string that happens to
begin with double-colon by mistake and making it harder to debug.

> I don't know if Actions distinguishes between stdout and stderr here
> (i.e., if we redirected the script's stdout to stderr, would that
> prevent this case or not?).

Perhaps we can experiment with "echo >&2 we are getting called" in
the allow-ref script itself ;-).

In any case, I'll queue it on 'pu'.  Thanks.


^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ci: allow per-branch config for GitHub Actions
  2020-05-07 21:58                                 ` Junio C Hamano
@ 2020-05-08 18:00                                   ` Jeff King
  2020-05-09  1:23                                     ` Đoàn Trần Công Danh
  0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-08 18:00 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, Đoàn Trần Công Danh, git,
	Jeff Hostetler, Johannes Schindelin

On Thu, May 07, 2020 at 02:58:16PM -0700, Junio C Hamano wrote:

> In any case, I'll queue it on 'pu'.  Thanks.

I just noticed this needs a small fix to the sample script, which I gave
the wrong name:

diff --git a/ci/config/allow-refs.sample b/ci/config/allow-ref.sample
similarity index 93%
rename from ci/config/allow-refs.sample
rename to ci/config/allow-ref.sample
index f157f1945a..c9c9aea9ff 100755
--- a/ci/config/allow-refs.sample
+++ b/ci/config/allow-ref.sample
@@ -7,8 +7,8 @@
 # your repository:
 #
 #   git checkout -b ci-config
-#   cp allow-refs.sample allow-refs
-#   $EDITOR allow-refs
+#   cp allow-refs.sample allow-ref
+#   $EDITOR allow-ref
 #   git commit -am "implement my ci preferences"
 #   git push
 #

> Perhaps we can experiment with "echo >&2 we are getting called" in
> the allow-ref script itself ;-).

That definitely ends up in the log. But doing:

  echo     "::error file=foo.sh,line=1,col=2::this is the stdout msg"
  echo >&2 "::error file=foo.sh,line=1,col=2::this is the stderr msg"

shows both versions formatted in red, which implies to me that stdout
and stderr are treated the same.

-Peff

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ci: allow per-branch config for GitHub Actions
  2020-05-08 18:00                                   ` Jeff King
@ 2020-05-09  1:23                                     ` Đoàn Trần Công Danh
  0 siblings, 0 replies; 62+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-09  1:23 UTC (permalink / raw)
  To: Jeff King
  Cc: Junio C Hamano, Taylor Blau, git, Jeff Hostetler, Johannes Schindelin

On 2020-05-08 14:00:47-0400, Jeff King <peff@peff.net> wrote:
> I just noticed this needs a small fix to the sample script, which I gave
> the wrong name:
> 
> diff --git a/ci/config/allow-refs.sample b/ci/config/allow-ref.sample
> similarity index 93%
> rename from ci/config/allow-refs.sample
> rename to ci/config/allow-ref.sample
> index f157f1945a..c9c9aea9ff 100755
> --- a/ci/config/allow-refs.sample
> +++ b/ci/config/allow-ref.sample
> @@ -7,8 +7,8 @@
>  # your repository:
>  #
>  #   git checkout -b ci-config
> -#   cp allow-refs.sample allow-refs
> -#   $EDITOR allow-refs
> +#   cp allow-refs.sample allow-ref

Hi Peff,

The source needs to be changed, too:
---------------8<-------------
diff --git a/ci/config/allow-ref.sample b/ci/config/allow-ref.sample
index c9c9aea9ff..249872425f 100755
--- a/ci/config/allow-ref.sample
+++ b/ci/config/allow-ref.sample
@@ -7,7 +7,7 @@
 # your repository:
 #
 #   git checkout -b ci-config
-#   cp allow-refs.sample allow-ref
+#   cp allow-ref.sample allow-ref
 #   $EDITOR allow-ref
 #   git commit -am "implement my ci preferences"
 #   git push
--------------------8<------------------

-- 
Danh

^ permalink raw reply related	[flat|nested] 62+ messages in thread

end of thread, other threads:[~2020-05-09  1:23 UTC | newest]

Thread overview: 62+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-02 15:08 [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR" Johannes Schindelin via GitGitGadget
2020-05-03  9:36 ` Jeff King
2020-05-03 12:05   ` Danh Doan
2020-05-04 15:01     ` Jeff King
2020-05-04 15:49       ` [PATCH v2 0/2] Limit GitHub Actions to designated branches Đoàn Trần Công Danh
2020-05-04 15:49         ` [PATCH v2 1/2] CI: limit " Đoàn Trần Công Danh
2020-05-04 16:23           ` Jeff King
2020-05-04 21:58             ` Taylor Blau
2020-05-04 22:52               ` Junio C Hamano
2020-05-04 23:15                 ` Taylor Blau
2020-05-04 23:35                   ` Jeff King
2020-05-05  0:24                     ` Junio C Hamano
2020-05-04 23:36               ` Jeff King
2020-05-05  0:20                 ` Taylor Blau
2020-05-05 16:43                   ` Jeff King
2020-05-05 17:57                     ` Junio C Hamano
2020-05-05 18:24                       ` Jeff King
2020-05-05 21:04                         ` Jeff King
2020-05-05 21:29                           ` Junio C Hamano
2020-05-05 21:58                             ` Jeff King
2020-05-05 22:28                               ` Junio C Hamano
2020-05-06 15:09                             ` Johannes Schindelin
2020-05-06 16:26                               ` Junio C Hamano
2020-05-07 12:17                                 ` Jeff King
2020-05-07 14:02                                   ` Jeff King
2020-05-07 18:17                                     ` Junio C Hamano
2020-05-07 12:01                               ` Đoàn Trần Công Danh
2020-05-07 12:47                                 ` Đoàn Trần Công Danh
2020-05-06  0:46                           ` Đoàn Trần Công Danh
2020-05-06  3:56                             ` Junio C Hamano
2020-05-06 14:25                               ` Đoàn Trần Công Danh
2020-05-06 16:31                                 ` Junio C Hamano
2020-05-07 12:25                                   ` Jeff King
2020-05-07 18:29                                     ` Junio C Hamano
2020-05-07 18:54                                       ` Jeff King
2020-05-07 19:33                                         ` Junio C Hamano
2020-05-07 16:20                           ` [PATCH v2] ci: allow per-branch config for GitHub Actions Jeff King
2020-05-07 17:00                             ` Taylor Blau
2020-05-07 17:18                               ` Jeff King
2020-05-07 19:53                             ` Junio C Hamano
2020-05-07 20:46                               ` Jeff King
2020-05-07 21:58                                 ` Junio C Hamano
2020-05-08 18:00                                   ` Jeff King
2020-05-09  1:23                                     ` Đoàn Trần Công Danh
2020-05-05  0:34             ` [PATCH v2 1/2] CI: limit GitHub Actions to designated branches Đoàn Trần Công Danh
2020-05-04 15:49         ` [PATCH v2 2/2] SubmittingPatches: advertise GitHub Actions CI Đoàn Trần Công Danh
2020-05-04 16:37           ` Junio C Hamano
2020-05-05  0:46             ` Đoàn Trần Công Danh
2020-05-05 16:26         ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Đoàn Trần Công Danh
2020-05-05 16:26           ` [PATCH v3 1/3] SubmittingPatches: advertise GitHub Actions CI Đoàn Trần Công Danh
2020-05-05 16:47             ` Jeff King
2020-05-05 16:59               ` Đoàn Trần Công Danh
2020-05-05 17:07                 ` Jeff King
2020-05-05 16:26           ` [PATCH v3 2/3] CI: limit GitHub Actions to designated branches Đoàn Trần Công Danh
2020-05-05 16:51             ` Jeff King
2020-05-05 17:05               ` Đoàn Trần Công Danh
2020-05-05 17:11                 ` Jeff King
2020-05-05 18:49             ` Junio C Hamano
2020-05-05 16:26           ` [PATCH v3 3/3] fixup! " Đoàn Trần Công Danh
2020-05-05 18:59             ` Junio C Hamano
2020-05-05 17:01           ` [PATCH v3 0/3] Provide option to opt in/out GitHub Actions Jeff King
2020-05-03 16:46   ` [PATCH] ci: respect the [skip ci] convention in our GitHub workflow "CI/PR" Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.