Re: [PATCH v2 1/2] GitLab Gating CI: introduce pipeline-status contrib script

[Cleber Rosa <crosa@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6302 bytes --]

On Thu, Jul 09, 2020 at 10:55:19AM +0200, Erik Skultety wrote:
> On Wed, Jul 08, 2020 at 10:46:56PM -0400, Cleber Rosa wrote:
> > This script is intended to be used right after a push to a branch.
> >
> > By default, it will look for the pipeline associated with the commit
> > that is the HEAD of the *local* staging branch.  It can be used as a
> > one time check, or with the `--wait` option to wait until the pipeline
> > completes.
> >
> > If the pipeline is successful, then a merge of the staging branch into
> > the master branch should be the next step.
> >
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >  scripts/ci/gitlab-pipeline-status | 156 ++++++++++++++++++++++++++++++
> >  1 file changed, 156 insertions(+)
> >  create mode 100755 scripts/ci/gitlab-pipeline-status
> >
> > diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
> > new file mode 100755
> > index 0000000000..4a9de39872
> > --- /dev/null
> > +++ b/scripts/ci/gitlab-pipeline-status
> > @@ -0,0 +1,156 @@
> > +#!/usr/bin/env python3
> > +#
> > +# Copyright (c) 2019-2020 Red Hat, Inc.
> > +#
> > +# Author:
> > +#  Cleber Rosa <crosa@redhat.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > +# later.  See the COPYING file in the top-level directory.
> > +
> > +"""
> > +Checks the GitLab pipeline status for a given commit commit
> 
> s/commit$/(hash|sha|ID|)
> 

Just for the record, this was picked up by Thomas (thanks Thomas!).

> > +"""
> > +
> > +# pylint: disable=C0103
> > +
> > +import argparse
> > +import http.client
> > +import json
> > +import os
> > +import subprocess
> > +import time
> > +import sys
> > +
> > +
> > +def get_local_staging_branch_commit():
> > +    """
> > +    Returns the commit sha1 for the *local* branch named "staging"
> > +    """
> > +    result = subprocess.run(['git', 'rev-parse', 'staging'],
> 
> If one day Peter decides that "staging" is not a cool name anymore and use a
> different name for the branch :) we should account for that and make it a
> variable, possibly even parametrize this function with it.
>

This function is currently only called to set a default for the
-c/--commit command line option, so users can always set it to the
commit ID of any branch.  But, your point still holds with regards to
future extensibility.  I'll send a patch with that change.

> > +                            stdin=subprocess.DEVNULL,
> > +                            stdout=subprocess.PIPE,
> > +                            stderr=subprocess.DEVNULL,
> > +                            cwd=os.path.dirname(__file__),
> > +                            universal_newlines=True).stdout.strip()
> > +    if result == 'staging':
> > +        raise ValueError("There's no local staging branch")
> 
> "There's no local branch named 'staging'" would IMO be more descriptive, so as
> not to confuse it with staging in git.
>

Ack, also picked up by Thomas.

> > +    if len(result) != 40:
> > +        raise ValueError("Branch staging HEAD doesn't look like a sha1")
> > +    return result
> > +
> > +
> > +def get_pipeline_status(project_id, commit_sha1):
> > +    """
> > +    Returns the JSON content of the pipeline status API response
> > +    """
> > +    url = '/api/v4/projects/{}/pipelines?sha={}'.format(project_id,
> > +                                                        commit_sha1)
> > +    connection = http.client.HTTPSConnection('gitlab.com')
> > +    connection.request('GET', url=url)
> > +    response = connection.getresponse()
> > +    if response.code != http.HTTPStatus.OK:
> > +        raise ValueError("Failed to receive a successful response")
> > +    json_response = json.loads(response.read())
> 
> a blank line separating the commentary block would slightly help readability
>

It would also be a good idea to follow PEP 257, but since there's currently
no check/enforcement, I'll defer to one it's introduced (hopefully soon).

> > +    # afaict, there should one one pipeline for the same project + commit
> 
> s/one one/be only one/
>

Ack, also picked up by Thomas (thanks again!).

> > +    # if this assumption is false, we can add further filters to the
> > +    # url, such as username, and order_by.
> > +    if not json_response:
> > +        raise ValueError("No pipeline found")
> > +    return json_response[0]
> > +
> > +
> > +def wait_on_pipeline_success(timeout, interval,
> > +                             project_id, commit_sha):
> > +    """
> > +    Waits for the pipeline to end up to the timeout given
> 
> "Waits for the pipeline to finish within the given timeout"
> 

Absolutely better, and also picked up by Thomas.

> > +    """
> > +    start = time.time()
> > +    while True:
> > +        if time.time() >= (start + timeout):
> > +            print("Waiting on the pipeline success timed out")
> 
> s/success//
> (the pipeline doesn't always have to finish with success)
>

You're right, your suggestion improves the message, indeed.

But, I think the wording is still confusing as it took me some time to
understand that this timeout was about how long this script will wait.
(my fault here).  So I'm going to propose that this changes to:

"Timeout (-t/--timeout) of %i seconds reached, won't wait any longer
for the pipeline to complete".

> > +            return False
> > +
> > +        status = get_pipeline_status(project_id, commit_sha)
> > +        if status['status'] == 'running':
> > +            time.sleep(interval)
> > +            print('running...')
> > +            continue
> > +
> > +        if status['status'] == 'success':
> > +            return True
> > +
> > +        msg = "Pipeline ended unsuccessfully, check: %s" % status['web_url']
> 
> I think more common expression is "Pipeline failed"
>

Agreed, and already addressed by Thomas (will I run out of thanks?).

> > +        print(msg)
> > +        return False
> > +
> ...
> 
> Code-wise looks OK to me, but since I don't know what Peter's requirements
> on/expectations of this script are, I can't do a more thorough review.
> 
> Regards,
> Erik

Thanks Erik!

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]