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

[Thomas Huth]

On 09/07/2020 12.13, Philippe Mathieu-Daudé wrote:
> On 7/9/20 10:55 AM, 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|)
>>
>>> +"""
>>> +
>>> +# 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 script can be used by any fork, not only Peter.
> So having a parameter (default to 'staging') is a requisite IMO.
> 
>>> +                            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.
>>
>>> +    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
>>
>>> +    # afaict, there should one one pipeline for the same project + commit
>>
>> s/one one/be only one/
> 
> 'afaict' is not a word.
> 
>>
>>> +    # 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"
>>
>>> +    """
>>> +    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)
>>
>>> +            return False
>>> +
>>> +        status = get_pipeline_status(project_id, commit_sha)
>>> +        if status['status'] == 'running':
>>> +            time.sleep(interval)
>>> +            print('running...')
> 
> If we want to automate the use of this script by a daemon, it would
> be better to use the logging class. Then maybe 'running...' is for
> the DEBUG level, Other print() calls can be updated to WARN/INFO
> levels.
> 
>>> +            continue
>>> +
>>> +        if status['status'] == 'success':
>>> +            return True
>>> +
>>> +        msg = "Pipeline ended unsuccessfully, check: %s" % 
>>> status['web_url']
>>
>> I think more common expression is "Pipeline failed"
>>
>>> +        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.

Ok, I'll add some of the trivial suggestions and take this patch through
my gitlab tree, so Peter can start integrating the gitlab CI in his
merge workflow. We can then later add additional features like the
parameter for the branch name or the logging class.
(I'll skip the second patch for now since there were more questions
raised there which should likely be answered / discussed first)

 Thomas