Re: [PATCH 2/2] gitlab: don't run CI jobs by default on push to user forks

[Daniel P . Berrangé]

On Wed, Aug 25, 2021 at 12:42:32PM +0200, Thomas Huth wrote:
> 
> (meta note: patch does not apply anymore and needs to be refreshed)
> 
> On 12/08/2021 20.04, Daniel P. Berrangé wrote:
> [...]
> > diff --git a/.gitlab-ci.d/rules.yml b/.gitlab-ci.d/rules.yml
> > new file mode 100644
> > index 0000000000..3399722ca9
> > --- /dev/null
> > +++ b/.gitlab-ci.d/rules.yml
> > @@ -0,0 +1,116 @@
> > +
> > +# This defines rules used to control individual job execution
> > +# See docs/devel/ci-jobs.rst for an explanation of the various
> > +# variables and branch naming conventions applied here.
> > +#
> > +.job_rules:
> > +  rules:
> > +    # 
> > ======================================================================
> > +    # Rules that apply regardless of whether the primary qemu repo or a 
> > fork
> > +    # 
> > ======================================================================
> > +
> > +    # Skip any cirrus job if either repo or api token are missing
> > +    # as we won't be able to talk to cirrus
> > +    - if: '$CIRRUS_VM_INSTANCE_TYPE && ($CIRRUS_GITHUB_REPO == null || 
> > $CIRRUS_API_TOKEN == null)'
> > +      when: never
> > +
> > +    # Any jobs marked as manual, never get automatically run in any 
> > scenario
> > +    # and don't contribute to pipeline status
> > +    - if: '$QEMU_JOB_MANUAL'
> > +      when: manual
> > +      allow_failure: true
> > +
> > +    # For the main repo, tags represent official releases.
> > +    # The branch associated with the release will have passed
> > +    # a CI pipeline already
> > +    #
> > +    # For user forks, tags are tyically added by tools like
> 
> s/tyically/typically/
> 
> > +    # git-publish at the same time as pushing the branch prior
> > +    # to sending out for review
> > +    #
> > +    # In neither case do we wish to run CI pipelines for tags
> > +    - if: '$CI_PIPELINE_SOURCE == "push" && $CI_COMMIT_TAG'
> > +      when: never
> 
> Not sure whether I like this rule ... First, tags are very seldom compared
> to normal pushes to branches, so this should not affect us that much.
> Second, I think it might be good for "documentation" purposes to be able to
> say that the CI ran properly for a certain tag. Ok, you could still look it
> up in the push to a branch that might have happened before, but that's
> cumbersome. Just my 0.02 €.

"git-publish" creates tags for every version and pushes them to your
repo when you use --pull arg. I don't want jobs triggered when
git-publish pushes tags, because I will have already tested the code
before I ask git-publish to send the pull.  So IMHO skipping CI pipelines
on forks is important to avoid git-publish burning everyone's CI minutes.

In terms of the upstream repo, a tag is only pushed when the changes for
the release have already been pushed. Those changes would have undergone
CI already. There's no point running CI again for the tag, especially if
no one is going to do anything with failures it might report. 

> > +    # ====================================
> > +    # Rules for running jobs in user forks
> > +    # ====================================
> > +
> > +    # Part 1: gating jobs
> > +    # -------------------
> > +
> > +    # If on a branch with name prefix 'ci-acceptance-', then run
> > +    # everything, just as a gating job on 'staging' branch would
> > +    - if: '$CI_COMMIT_BRANCH =~ /^ci-gating-/'
> > +      when: on_success
> > +
> > +    # If user set QEMU_CI_GATING=1, then run everything just as
> > +    # a gating job on 'staging' branch would
> > +    - if: '$QEMU_CI_GATING'
> > +      when: on_success
> > +
> > +    # Otherwise never run jobs marked as gating, but allow manual trigger
> > +    # without affecting pipeline status
> > +    - if: '$QEMU_JOB_GATING'
> > +      when: manual
> > +      allow_failure: true
> 
> IMHO if it's "gating", then we should not use "allow_failure: true", no
> matter whether the job is manual or not. Otherwise this is very confusing.

I think you're misinterpreting this.

Gating jobs run on the upstream pushes to 'staging' (not shown in this
quoted context - its higher in rules.yml), or if you have pushed to a
branch 'ci-gating-' (the first rule here), or if you set QEMU_CI_GATING
env var when pushing (the second rule here).

This 3rd rule is about ensuring gating jobs do NOT get launched in any
other scenario. We could use 'when: never' but that's a big hammer that
prevents users to opt-ing in to running a single job from the web UI,
so we use 'when: manual'.  If you use 'when: manual' on its own, the
pipeline will never complete, as it'll be waiting for someone to
trigger the manual job which is not what we need. 'allow_failure: true'
means that the pipeline will complete without waiting for the manual
job.

Ideally there would be a way to say "Let this job be manually run,
and make its failure affect the pipeline status, but ignore the job
if not ru nmanually".  GitLab doesn't support that AFAIK though, so
this is the next best option, that isn't 'when: never'.

> > +
> > +    # =============================================
> > +    # Part 2: opt-in for all CI, except gating jobs
> > +    # =============================================
> > +
> > +    # If pushing to a branch with name prefix 'ci-all', then run all jobs
> > +    - if: '$CI_COMMIT_BRANCH =~ /^ci-all/'
> > +      when: on_success
> > +
> > +    # If user QEMU_CI_ALL=1, then run all jobs
> > +    - if: '$QEMU_CI_ALL'
> > +      when: on_success
> 
> Uh, so "all" is not running all jobs? ... that's confusing, too. Could you
> come up with some better naming? QEMU_CI_CORE maybe?

"all" is running everything that currently runs today when you do a
git push in a fork.  I admit it is odd that 'gating' runs more than
'all'.

We could change it to "default" instead.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|