[v2,1/2] ci: skip GitHub workflow runs for already-tested commits/trees

Commit Message

Philippe Blain via GitGitGadget Oct. 8, 2020, 3:29 p.m. UTC

From: Johannes Schindelin <[e-mail]>

When pushing a commit that has already passed a CI or PR build
successfully, it makes sense to save some energy and time and skip the
new build.

Let's teach our GitHub workflow to do that.

For good measure, we also compare the tree ID, which is what we actually
test (the commit ID might have changed due to a reworded commit message,
which should not affect the outcome of the run).

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 .github/workflows/main.yml | 39 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

Comments

SZEDER Gábor Oct. 9, 2020, 7:29 a.m. UTC | #1

On Thu, Oct 08, 2020 at 03:29:34PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> When pushing a commit that has already passed a CI or PR build
> successfully, it makes sense to save some energy and time and skip the
> new build.
> 
> Let's teach our GitHub workflow to do that.
> 
> For good measure, we also compare the tree ID, which is what we actually
> test (the commit ID might have changed due to a reworded commit message,
> which should not affect the outcome of the run).

We have been doing this on Travis CI for a few years now.  Does that
approach not work on GitHub actions?  Please explain in the commit
message why a different approach is taken here.

> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>  .github/workflows/main.yml | 39 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 5bd321e5e1..7391b46d61 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -9,7 +9,7 @@ jobs:
>    ci-config:
>      runs-on: ubuntu-latest
>      outputs:
> -      enabled: ${{ steps.check-ref.outputs.enabled }}
> +      enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
>      steps:
>        - name: try to clone ci-config branch
>          continue-on-error: true
> @@ -35,6 +35,43 @@ jobs:
>              enabled=no
>            fi
>            echo "::set-output name=enabled::$enabled"
> +      - name: skip if the commit or tree was already tested
> +        id: skip-if-redundant
> +        uses: actions/github-script@v3
> +        if: steps.check-ref.outputs.enabled == 'yes'
> +        with:
> +          github-token: ${{secrets.GITHUB_TOKEN}}
> +          script: |
> +            // Figure out workflow ID, commit and tree
> +            const { data: run } = await github.actions.getWorkflowRun({
> +              owner: context.repo.owner,
> +              repo: context.repo.repo,
> +              run_id: context.runId,
> +            });
> +            const workflow_id = run.workflow_id;
> +            const head_sha = run.head_sha;
> +            const tree_id = run.head_commit.tree_id;
> +
> +            // See whether there is a successful run for that commit or tree
> +            const { data: runs } = await github.actions.listWorkflowRuns({
> +              owner: context.repo.owner,
> +              repo: context.repo.repo,
> +              per_page: 500,
> +              status: 'success',
> +              workflow_id,
> +            });
> +            for (const run of runs.workflow_runs) {
> +              if (head_sha === run.head_sha) {
> +                core.warning(`Successful run for the commit ${head_sha}: ${run.html_url}`);
> +                core.setOutput('enabled', ' but skip');
> +                break;
> +              }
> +              if (tree_id === run.head_commit.tree_id) {
> +                core.warning(`Successful run for the tree ${tree_id}: ${run.html_url}`);
> +                core.setOutput('enabled', ' but skip');
> +                break;
> +              }
> +            }
>  
>    windows-build:
>      needs: ci-config
> -- 
> gitgitgadget
>

Johannes Schindelin Oct. 9, 2020, 11:13 a.m. UTC | #2

Hi Gábor,

On Fri, 9 Oct 2020, SZEDER Gábor wrote:

> On Thu, Oct 08, 2020 at 03:29:34PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[e-mail]>
> >
> > When pushing a commit that has already passed a CI or PR build
> > successfully, it makes sense to save some energy and time and skip the
> > new build.
> >
> > Let's teach our GitHub workflow to do that.
> >
> > For good measure, we also compare the tree ID, which is what we actually
> > test (the commit ID might have changed due to a reworded commit message,
> > which should not affect the outcome of the run).
>
> We have been doing this on Travis CI for a few years now.  Does that
> approach not work on GitHub actions?  Please explain in the commit
> message why a different approach is taken here.

You're not being terribly clear about what exactly "We have been doing".

Are you referring to the `skip_good_tree()` function that stores
information in a file in the `good_trees_file`?

If so, no, we cannot do that anywhere else than on Travis because that
relies on a directory that is somehow shared between runs. And that is a
feature that only Travis offers as far as I know (and it does not come
without issues, e.g. when two concurrent runs try to write to the same
file at the same time).

Since this strategy relies on a Travis-only feature that does not work on
the three other CI services we use (Cirrus CI, Azure DevOps, GitHub
Actions), I see little point mentioning it in this commit message...

However, I might be very well wrong on that assessment.

Ciao,
Dscho

>
> > Signed-off-by: Johannes Schindelin <[e-mail]>
> > ---
> >  .github/workflows/main.yml | 39 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> > index 5bd321e5e1..7391b46d61 100644
> > --- a/.github/workflows/main.yml
> > +++ b/.github/workflows/main.yml
> > @@ -9,7 +9,7 @@ jobs:
> >    ci-config:
> >      runs-on: ubuntu-latest
> >      outputs:
> > -      enabled: ${{ steps.check-ref.outputs.enabled }}
> > +      enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
> >      steps:
> >        - name: try to clone ci-config branch
> >          continue-on-error: true
> > @@ -35,6 +35,43 @@ jobs:
> >              enabled=no
> >            fi
> >            echo "::set-output name=enabled::$enabled"
> > +      - name: skip if the commit or tree was already tested
> > +        id: skip-if-redundant
> > +        uses: actions/github-script@v3
> > +        if: steps.check-ref.outputs.enabled == 'yes'
> > +        with:
> > +          github-token: ${{secrets.GITHUB_TOKEN}}
> > +          script: |
> > +            // Figure out workflow ID, commit and tree
> > +            const { data: run } = await github.actions.getWorkflowRun({
> > +              owner: context.repo.owner,
> > +              repo: context.repo.repo,
> > +              run_id: context.runId,
> > +            });
> > +            const workflow_id = run.workflow_id;
> > +            const head_sha = run.head_sha;
> > +            const tree_id = run.head_commit.tree_id;
> > +
> > +            // See whether there is a successful run for that commit or tree
> > +            const { data: runs } = await github.actions.listWorkflowRuns({
> > +              owner: context.repo.owner,
> > +              repo: context.repo.repo,
> > +              per_page: 500,
> > +              status: 'success',
> > +              workflow_id,
> > +            });
> > +            for (const run of runs.workflow_runs) {
> > +              if (head_sha === run.head_sha) {
> > +                core.warning(`Successful run for the commit ${head_sha}: ${run.html_url}`);
> > +                core.setOutput('enabled', ' but skip');
> > +                break;
> > +              }
> > +              if (tree_id === run.head_commit.tree_id) {
> > +                core.warning(`Successful run for the tree ${tree_id}: ${run.html_url}`);
> > +                core.setOutput('enabled', ' but skip');
> > +                break;
> > +              }
> > +            }
> >
> >    windows-build:
> >      needs: ci-config
> > --
> > gitgitgadget
> >
>

SZEDER Gábor Oct. 10, 2020, 7:25 a.m. UTC | #3

On Fri, Oct 09, 2020 at 01:13:03PM +0200, Johannes Schindelin wrote:
> Hi Gábor,
> 
> On Fri, 9 Oct 2020, SZEDER Gábor wrote:
> 
> > On Thu, Oct 08, 2020 at 03:29:34PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > From: Johannes Schindelin <[e-mail]>
> > >
> > > When pushing a commit that has already passed a CI or PR build
> > > successfully, it makes sense to save some energy and time and skip the
> > > new build.
> > >
> > > Let's teach our GitHub workflow to do that.
> > >
> > > For good measure, we also compare the tree ID, which is what we actually
> > > test (the commit ID might have changed due to a reworded commit message,
> > > which should not affect the outcome of the run).
> >
> > We have been doing this on Travis CI for a few years now.  Does that
> > approach not work on GitHub actions?  Please explain in the commit
> > message why a different approach is taken here.
> 
> You're not being terribly clear about what exactly "We have been doing".
> 
> Are you referring to the `skip_good_tree()` function that stores
> information in a file in the `good_trees_file`?

Yes, in the commit message you pretty accurately described the exact
purpose of that function.

> If so, no, we cannot do that anywhere else than on Travis because that
> relies on a directory that is somehow shared between runs. And that is a
> feature that only Travis offers as far as I know (and it does not come
> without issues, e.g. when two concurrent runs try to write to the same
> file at the same time).

Every branchname-job combination has its own cache, and while the job
is running it writes to a local copy of its own cache, so concurrent
runs don't seem to be an issue.

> Since this strategy relies on a Travis-only feature that does not work on
> the three other CI services we use (Cirrus CI, Azure DevOps, GitHub
> Actions), I see little point mentioning it in this commit message...

This commit duplicates already existing functionality, so, yes, the
commit message should definitely have explained why that already
existing approach was not suitable for GitHub Actions.

Johannes Schindelin Oct. 11, 2020, 10:28 a.m. UTC | #4

Hi Gábor,

On Sat, 10 Oct 2020, SZEDER Gábor wrote:

> On Fri, Oct 09, 2020 at 01:13:03PM +0200, Johannes Schindelin wrote:
>
> > Since this strategy relies on a Travis-only feature that does not work
> > on the three other CI services we use (Cirrus CI, Azure DevOps, GitHub
> > Actions), I see little point mentioning it in this commit message...
>
> This commit duplicates already existing functionality, so, yes, the
> commit message should definitely have explained why that already
> existing approach was not suitable for GitHub Actions.

No, this is not duplicating functionality.

The `skip_good_tree()` function requires a persistent directory into which
it writes a record of the trees it considers good, based on past runs. It
later recalls which trees are considers good and skips the current run if
there is a record for this tree.

The fact that it requires a persistent directory binds it to Travis CI. As
far as I can tell, no other CI service offers that feature (and from where
I sit, for good reason, because it is asking for all kinds of fun in
concurrent scenarios).

What my patch does might duplicate the intention, but absolutely not the
functionality. For one, there is no extra record required. It uses the API
to query the existing logs.

Also, the patch specifically adjusts the GitHub workflow itself.
Therefore, unlike the `skip_good_tree()` function, it does not pretend to
be generic (which `skip_good_tree()` really is not, as pointed out above).

Ciao,
Dscho

Junio C Hamano Oct. 12, 2020, 4:12 p.m. UTC | #5

Johannes Schindelin <[e-mail]> writes:

> Also, the patch specifically adjusts the GitHub workflow itself.
> Therefore, unlike the `skip_good_tree()` function, it does not pretend to
> be generic (which `skip_good_tree()` really is not, as pointed out above).

I think skip_good_tree aspired to be a generic one, by having the
"if we are not in travis nor GitHub actions, return early" at its
very beginning.  The person who adds support to GitHub workflow
could have done one of two things.  

One is to recognise the aspiration, and restructure existing
skip_good_tree from

    skip_good_tree () {
	return if not travis and if not github actions

	bunch of code that happens to work only on travis
    }

to

    skip_good_tree () {
	if travis
	then
	    bunch of code that happens to work only on travis
	else if github actions
	    bunch of code that happens to work only with github
	fi
    }

without touching the caller.  That lets skip_good_tree to be
generic.

Another is to completely ignore that aspiration, maybe doing all
that inside the workflow script (which by definition works only with
github).

I think the latter (i.e. what the patch choose to do, which is not
to bother with the ci/*.sh scripts at all) would be cleaner in this
particular case, but then it would have made the intention more
clear if the conditional at the beginning of skip_good_tree() were
adjusted, perhaps?

Johannes Schindelin Oct. 12, 2020, 6:57 p.m. UTC | #6

Hi Junio,

On Mon, 12 Oct 2020, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > Also, the patch specifically adjusts the GitHub workflow itself.
> > Therefore, unlike the `skip_good_tree()` function, it does not pretend to
> > be generic (which `skip_good_tree()` really is not, as pointed out above).
>
> I think skip_good_tree aspired to be a generic one, by having the
> "if we are not in travis nor GitHub actions, return early" at its
> very beginning. [...] it would have made the intention more clear if the
> conditional at the beginning of skip_good_tree() were adjusted, perhaps?

Full disclosure: I am not even sure what to do with Travis support at this
stage (but then, I am not the one to decide over that).

In any case, it appears that no matter how generic the `skip_good_tree()`
function meant to be, by virtue of using that `cache: directories:`
feature (https://docs.travis-ci.com/user/caching) it is limited to Travis
CI nevertheless.

So if we were to adjust the conditional at the beginning of
`skip_good_tree()`, it would probably make sense to make it clear that
this not only does not work with GitHub Actions, but in fact it does not
work with anything but Travis.

Ciao,
Dscho

Junio C Hamano Oct. 15, 2020, 5:17 p.m. UTC | #7

Johannes Schindelin <[e-mail]> writes:

> Full disclosure: I am not even sure what to do with Travis support at this
> stage (but then, I am not the one to decide over that).

Are you talking about the migration to travis-ci.com and having to
give them potentially more access to github.com/git/git/ repository?

It does worry me, too.

Johannes Schindelin Oct. 15, 2020, 7:39 p.m. UTC | #8

Hi Junio,

On Thu, 15 Oct 2020, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > Full disclosure: I am not even sure what to do with Travis support at this
> > stage (but then, I am not the one to decide over that).
>
> Are you talking about the migration to travis-ci.com and having to
> give them potentially more access to github.com/git/git/ repository?
>
> It does worry me, too.

I meant that, together with worries about the future of Travis CI given
that they've been acquired by Idera.

Ciao,
Dscho

Patch

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 5bd321e5e1..7391b46d61 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -9,7 +9,7 @@  jobs:
   ci-config:
     runs-on: ubuntu-latest
     outputs:
-      enabled: ${{ steps.check-ref.outputs.enabled }}
+      enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
     steps:
       - name: try to clone ci-config branch
         continue-on-error: true
@@ -35,6 +35,43 @@  jobs:
             enabled=no
           fi
           echo "::set-output name=enabled::$enabled"
+      - name: skip if the commit or tree was already tested
+        id: skip-if-redundant
+        uses: actions/github-script@v3
+        if: steps.check-ref.outputs.enabled == 'yes'
+        with:
+          github-token: ${{secrets.GITHUB_TOKEN}}
+          script: |
+            // Figure out workflow ID, commit and tree
+            const { data: run } = await github.actions.getWorkflowRun({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              run_id: context.runId,
+            });
+            const workflow_id = run.workflow_id;
+            const head_sha = run.head_sha;
+            const tree_id = run.head_commit.tree_id;
+
+            // See whether there is a successful run for that commit or tree
+            const { data: runs } = await github.actions.listWorkflowRuns({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              per_page: 500,
+              status: 'success',
+              workflow_id,
+            });
+            for (const run of runs.workflow_runs) {
+              if (head_sha === run.head_sha) {
+                core.warning(`Successful run for the commit ${head_sha}: ${run.html_url}`);
+                core.setOutput('enabled', ' but skip');
+                break;
+              }
+              if (tree_id === run.head_commit.tree_id) {
+                core.warning(`Successful run for the tree ${tree_id}: ${run.html_url}`);
+                core.setOutput('enabled', ' but skip');
+                break;
+              }
+            }
 
   windows-build:
     needs: ci-config