This job uses the shared "check-dco" image to validate that all
commits on a developer's branch have a suitable Signed-off-by
statement present.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---

This patch was against the Perl repo, but if this is approved then
I'll also apply it to *all* the other repos which currently lack a
DCO check, without reposting further patches for each repo.

For libvirt.git I'll send a patch to update its existing DCO job.

 .gitlab-ci.yml | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 .gitlab-ci.yml

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 0000000..56e0e02
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,16 @@
+
+stages:
+  - prebuild
+
+# Check that all commits are signed-off for the DCO. Skip
+# on master branch and -maint branches, since we only need
+# to test developer's personal branches.
+check-dco:
+  stage: prebuild
+  image: registry.gitlab.com/libvirt/libvirt-ci/check-dco:master
+  script:
+    - /check-dco
+  only:
+    - branches
+  except:
+    - master
-- 
2.25.2

On Wed, 2020-04-22 at 15:37 +0100, Daniel P. Berrangé wrote:
> +# Check that all commits are signed-off for the DCO. Skip
> +# on master branch and -maint branches, since we only need
> +# to test developer's personal branches.
> +check-dco:
> +  stage: prebuild
> +  image: registry.gitlab.com/libvirt/libvirt-ci/check-dco:master
> +  script:
> +    - /check-dco
> +  only:
> +    - branches
> +  except:
> +    - master

You're not actually skipping the -maint branches here, so either
you need to change this to

  except:
    - /^v.*-maint$/
    - master

which libvirt currently uses, or to drop the mention of -maint
branches from the comment.

Why is it that we want to skip those branches, anyway? I get why
they're not necessary in a MR-based workflow, but we're not quite
there yet...

Actually, now that we're using GitLab as the primary repository,
how are we ensuring commits without DCO don't slip in? We had a
hook that took care of that on libvirt.org - was something like
that introduced on GitLab?

-- 
Andrea Bolognani / Red Hat / Virtualization

On Wed, Apr 22, 2020 at 06:11:13PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-04-22 at 15:37 +0100, Daniel P. Berrangé wrote:
> > +# Check that all commits are signed-off for the DCO. Skip
> > +# on master branch and -maint branches, since we only need
> > +# to test developer's personal branches.
> > +check-dco:
> > +  stage: prebuild
> > +  image: registry.gitlab.com/libvirt/libvirt-ci/check-dco:master
> > +  script:
> > +    - /check-dco
> > +  only:
> > +    - branches
> > +  except:
> > +    - master
> 
> You're not actually skipping the -maint branches here, so either
> you need to change this to
> 
>   except:
>     - /^v.*-maint$/
>     - master
> 
> which libvirt currently uses, or to drop the mention of -maint
> branches from the comment.
> 
> Why is it that we want to skip those branches, anyway? I get why
> they're not necessary in a MR-based workflow, but we're not quite
> there yet...

This was an inexact way to stop the checks running against the
master repo, after the patches have been merged.

The flaw in this is that a user could indeed open a merge request
that uses a "master" or "v*maint" branch in their private fork,
rather than a named feature branch.

Really we want it to run on all commits in a user's fork, but
not run in the master repos post-merge.

> Actually, now that we're using GitLab as the primary repository,
> how are we ensuring commits without DCO don't slip in? We had a
> hook that took care of that on libvirt.org - was something like
> that introduced on GitLab?

It isn't as strict as before - there's a push rule that requires
the word "Signed-off-by" in the commit message:

  https://libvirt.org/newreposetup.html


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 :|

On Wed, 2020-04-22 at 17:20 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 22, 2020 at 06:11:13PM +0200, Andrea Bolognani wrote:
> > Why is it that we want to skip those branches, anyway? I get why
> > they're not necessary in a MR-based workflow, but we're not quite
> > there yet...
> 
> This was an inexact way to stop the checks running against the
> master repo, after the patches have been merged.
> 
> The flaw in this is that a user could indeed open a merge request
> that uses a "master" or "v*maint" branch in their private fork,
> rather than a named feature branch.
> 
> Really we want it to run on all commits in a user's fork, but
> not run in the master repos post-merge.

I still don't understand why we would want to single out those
branches and not run the DCO check on them. What harm would it
cause? It takes around a minute to run it, which is significantly
less than the other jobs running during the prebuild stage...

> > Actually, now that we're using GitLab as the primary repository,
> > how are we ensuring commits without DCO don't slip in? We had a
> > hook that took care of that on libvirt.org - was something like
> > that introduced on GitLab?
> 
> It isn't as strict as before - there's a push rule that requires
> the word "Signed-off-by" in the commit message:
> 
>   https://libvirt.org/newreposetup.html

Oh, cool! I had forgotten about that detail since reviewing the
document, and it's nice to know that we still have at least some
level of protection on that front :)

-- 
Andrea Bolognani / Red Hat / Virtualization

On Wed, Apr 22, 2020 at 07:01:50PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-04-22 at 17:20 +0100, Daniel P. Berrangé wrote:
> > On Wed, Apr 22, 2020 at 06:11:13PM +0200, Andrea Bolognani wrote:
> > > Why is it that we want to skip those branches, anyway? I get why
> > > they're not necessary in a MR-based workflow, but we're not quite
> > > there yet...
> > 
> > This was an inexact way to stop the checks running against the
> > master repo, after the patches have been merged.
> > 
> > The flaw in this is that a user could indeed open a merge request
> > that uses a "master" or "v*maint" branch in their private fork,
> > rather than a named feature branch.
> > 
> > Really we want it to run on all commits in a user's fork, but
> > not run in the master repos post-merge.
> 
> I still don't understand why we would want to single out those
> branches and not run the DCO check on them. What harm would it
> cause? It takes around a minute to run it, which is significantly
> less than the other jobs running during the prebuild stage...

The check-dco script doesn't actually work if run against the
main libvirt repo, as it ends up trying to use itself as a
reference and failing to figure out which commits need checking.
Of course that's a bug that's fixable, but in general I think it
is better to not runthe job at all and thus eliminate any risk
of false failures.

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 :|

On Wed, 2020-04-22 at 18:06 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 22, 2020 at 07:01:50PM +0200, Andrea Bolognani wrote:
> > I still don't understand why we would want to single out those
> > branches and not run the DCO check on them. What harm would it
> > cause? It takes around a minute to run it, which is significantly
> > less than the other jobs running during the prebuild stage...
> 
> The check-dco script doesn't actually work if run against the
> main libvirt repo, as it ends up trying to use itself as a
> reference and failing to figure out which commits need checking.
> Of course that's a bug that's fixable, but in general I think it
> is better to not runthe job at all and thus eliminate any risk
> of false failures.

It seems to work fine here:

  https://gitlab.com/abologna/libvirt/-/jobs/522467078

The corresponding commit is

  https://gitlab.com/abologna/libvirt/-/commit/435ef06536f590f4403d0ce59a1b9edc1dfa80ec

where I changed the local require-dco.py script to use the very same
branch I pushed to as the base branch for DCO checking.

I believe you fixed the issue you mention above with

  commit 769ff77c9c5afaec97350a4931e5ca123b6af6d2
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Fri Mar 27 14:38:49 2020 +0000

    scripts: avoid error in DCO check on empty branches

    If the DCO check is run on an empty branch (ie one which has no commits
    different from master), it throws an error due to trying to interpret
    the empty string as a git commit SHA.

    Reviewed-by: Andrea Bolognani <abologna@redhat.com>
    Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

  v6.1.0-306-g769ff77c9c

So I see no reason not to just run the check on all branches.


If you remove the except: part, and of course the corresponding
comment as well, then

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization