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

[Erik Skultety <eskultet@redhat.com>]

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.

> +                            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/

> +    # 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...')
> +            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.

Regards,
Erik