[PATCH 0/7] gitlab pipeline check/watch script improvements

[PATCH 0/7] gitlab pipeline check/watch script improvements

[Cleber Rosa]

This is a collection of refactors and improvements based on feedback
received.

The biggest difference is that now the script can be used right after
a git push, despite the pipeline having been created yet or not.  The
proper status of the pipeline (not created, pending, etc) will be
given as output.

Cleber Rosa (7):
  scripts/ci/gitlab-pipeline-status: make branch name configurable
  scripts/ci/gitlab-pipeline-status: improve message regarding timeout
  scripts/ci/gitlab-pipeline-status: give early feedback on running
    pipelines
  scripts/ci/gitlab-pipeline-status: refactor parser creation
  scripts/ci/gitlab-pipeline-status: handle keyboard interrupts
  scripts/ci/gitlab-pipeline-status: use more descriptive exceptions
  scripts/ci/gitlab-pipeline-status: wait for pipeline creation

 scripts/ci/gitlab-pipeline-status | 63 +++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 19 deletions(-)

-- 
2.25.4

[PATCH 1/7] scripts/ci/gitlab-pipeline-status: make branch name configurable

[Cleber Rosa]

With the utility function `get_local_staging_branch_commit()`, the
name of the branch is hard coded (including in the function name).

For extensibility reasons, let's make that configurable.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/ci/gitlab-pipeline-status | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index 348a49b6a4..194dd4d0bb 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -23,20 +23,20 @@ import time
 import sys
 
 
-def get_local_staging_branch_commit():
+def get_local_branch_commit(branch='staging'):
     """
     Returns the commit sha1 for the *local* branch named "staging"
     """
-    result = subprocess.run(['git', 'rev-parse', 'staging'],
+    result = subprocess.run(['git', 'rev-parse', branch],
                             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 branch named 'staging'")
+    if result == branch:
+        raise ValueError("There's no local branch named '%s'" % branch)
     if len(result) != 40:
-        raise ValueError("Branch staging HEAD doesn't look like a sha1")
+        raise ValueError("Branch '%s' HEAD doesn't look like a sha1" % branch)
     return result
 
 
@@ -110,7 +110,7 @@ def main():
                               'for https://gitlab.com/qemu-project/qemu, that '
                               'is, "%(default)s"'))
     try:
-        default_commit = get_local_staging_branch_commit()
+        default_commit = get_local_branch_commit()
         commit_required = False
     except ValueError:
         default_commit = ''
-- 
2.25.4

Re: [PATCH 1/7] scripts/ci/gitlab-pipeline-status: make branch name configurable

[Philippe Mathieu-Daudé]

On 9/4/20 6:42 PM, Cleber Rosa wrote:
> With the utility function `get_local_staging_branch_commit()`, the
> name of the branch is hard coded (including in the function name).
> 
> For extensibility reasons, let's make that configurable.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  scripts/ci/gitlab-pipeline-status | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
> index 348a49b6a4..194dd4d0bb 100755
> --- a/scripts/ci/gitlab-pipeline-status
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -23,20 +23,20 @@ import time
>  import sys
>  
>  
> -def get_local_staging_branch_commit():
> +def get_local_branch_commit(branch='staging'):
>      """
>      Returns the commit sha1 for the *local* branch named "staging"
>      """
> -    result = subprocess.run(['git', 'rev-parse', 'staging'],
> +    result = subprocess.run(['git', 'rev-parse', branch],
>                              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 branch named 'staging'")
> +    if result == branch:
> +        raise ValueError("There's no local branch named '%s'" % branch)
>      if len(result) != 40:
> -        raise ValueError("Branch staging HEAD doesn't look like a sha1")
> +        raise ValueError("Branch '%s' HEAD doesn't look like a sha1" % branch)
>      return result
>  
>  
> @@ -110,7 +110,7 @@ def main():
>                                'for https://gitlab.com/qemu-project/qemu, that '
>                                'is, "%(default)s"'))
>      try:
> -        default_commit = get_local_staging_branch_commit()
> +        default_commit = get_local_branch_commit()
>          commit_required = False
>      except ValueError:
>          default_commit = ''
> 

[PATCH 2/7] scripts/ci/gitlab-pipeline-status: improve message regarding timeout

[Cleber Rosa]

The script has its own timeout, which is about how long the script
will wait (when called with --wait) for the pipeline to complete, and
not necessarily for the pipeline to complete.

Hopefully this will new wording will be clearer.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/ci/gitlab-pipeline-status | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index 194dd4d0bb..2a36f74696 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -69,7 +69,10 @@ def wait_on_pipeline_success(timeout, interval,
     start = time.time()
     while True:
         if time.time() >= (start + timeout):
-            print("Waiting on the pipeline timed out")
+            msg = ("Timeout (-t/--timeout) of %i seconds reached, "
+                   "won't wait any longer for the pipeline to complete")
+            msg %= timeout
+            print(msg)
             return False
 
         status = get_pipeline_status(project_id, commit_sha)
-- 
2.25.4

Re: [PATCH 2/7] scripts/ci/gitlab-pipeline-status: improve message regarding timeout

[Philippe Mathieu-Daudé]

On 9/4/20 6:42 PM, Cleber Rosa wrote:
> The script has its own timeout, which is about how long the script
> will wait (when called with --wait) for the pipeline to complete, and
> not necessarily for the pipeline to complete.
> 
> Hopefully this will new wording will be clearer.

too many wills?

> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  scripts/ci/gitlab-pipeline-status | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
> index 194dd4d0bb..2a36f74696 100755
> --- a/scripts/ci/gitlab-pipeline-status
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -69,7 +69,10 @@ def wait_on_pipeline_success(timeout, interval,
>      start = time.time()
>      while True:
>          if time.time() >= (start + timeout):
> -            print("Waiting on the pipeline timed out")
> +            msg = ("Timeout (-t/--timeout) of %i seconds reached, "
> +                   "won't wait any longer for the pipeline to complete")
> +            msg %= timeout
> +            print(msg)

Isn't it more pythonic / cheaper to declare the msg format out
of the loop and call directly:

               print(msg % timeout)

?

>              return False
>  
>          status = get_pipeline_status(project_id, commit_sha)
> 

[PATCH 3/7] scripts/ci/gitlab-pipeline-status: give early feedback on running pipelines

[Cleber Rosa]

When waiting for a pipeline to run and finish, it's better to give
early feedback, and then sleep and wait, than the other wait around.

Specially for the first iteration, it's frustrating to see nothing
while the script is sleeping.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/ci/gitlab-pipeline-status | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index 2a36f74696..18609553be 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -77,8 +77,8 @@ def wait_on_pipeline_success(timeout, interval,
 
         status = get_pipeline_status(project_id, commit_sha)
         if status['status'] == 'running':
-            time.sleep(interval)
             print('running...')
+            time.sleep(interval)
             continue
 
         if status['status'] == 'success':
-- 
2.25.4

Re: [PATCH 3/7] scripts/ci/gitlab-pipeline-status: give early feedback on running pipelines

[Philippe Mathieu-Daudé]

On 9/4/20 6:42 PM, Cleber Rosa wrote:
> When waiting for a pipeline to run and finish, it's better to give
> early feedback, and then sleep and wait, than the other wait around.
> 
> Specially for the first iteration, it's frustrating to see nothing
> while the script is sleeping.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  scripts/ci/gitlab-pipeline-status | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
> index 2a36f74696..18609553be 100755
> --- a/scripts/ci/gitlab-pipeline-status
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -77,8 +77,8 @@ def wait_on_pipeline_success(timeout, interval,
>  
>          status = get_pipeline_status(project_id, commit_sha)
>          if status['status'] == 'running':
> -            time.sleep(interval)
>              print('running...')
> +            time.sleep(interval)
>              continue
>  
>          if status['status'] == 'success':
> 

[PATCH 4/7] scripts/ci/gitlab-pipeline-status: refactor parser creation

[Cleber Rosa]

Out of the main function.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/ci/gitlab-pipeline-status | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index 18609553be..8355b6a427 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -89,10 +89,7 @@ def wait_on_pipeline_success(timeout, interval,
         return False
 
 
-def main():
-    """
-    Script entry point
-    """
+def create_parser():
     parser = argparse.ArgumentParser(
         prog='pipeline-status',
         description='check or wait on a pipeline status')
@@ -127,7 +124,13 @@ def main():
     parser.add_argument('--verbose', action='store_true', default=False,
                         help=('A minimal verbosity level that prints the '
                               'overall result of the check/wait'))
+    return parser
 
+def main():
+    """
+    Script entry point
+    """
+    parser = create_parser()
     args = parser.parse_args()
 
     try:
-- 
2.25.4

[PATCH 5/7] scripts/ci/gitlab-pipeline-status: handle keyboard interrupts

[Cleber Rosa]

So that exits based on user requests are handled more gracefully.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/ci/gitlab-pipeline-status | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index 8355b6a427..ced488f27c 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -132,7 +132,7 @@ def main():
     """
     parser = create_parser()
     args = parser.parse_args()
-
+    success = False
     try:
         if args.wait:
             success = wait_on_pipeline_success(
@@ -145,9 +145,11 @@ def main():
                                          args.commit)
             success = status['status'] == 'success'
     except Exception as error:      # pylint: disable=W0703
-        success = False
         if args.verbose:
             print("ERROR: %s" % error.args[0])
+    except KeyboardInterrupt:
+        if args.verbose:
+            print("Exiting on user's request")
 
     if success:
         if args.verbose:
-- 
2.25.4

[PATCH 6/7] scripts/ci/gitlab-pipeline-status: use more descriptive exceptions

[Cleber Rosa]

For two very different error conditions.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/ci/gitlab-pipeline-status | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index ced488f27c..628150ce0b 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -23,6 +23,14 @@ import time
 import sys
 
 
+class CommunicationFailure(Exception):
+    """Failed to communicate to gitlab.com APIs."""
+
+
+class NoPipelineFound(Exception):
+    """Communication is successfull but pipeline is not found."""
+
+
 def get_local_branch_commit(branch='staging'):
     """
     Returns the commit sha1 for the *local* branch named "staging"
@@ -50,14 +58,14 @@ def get_pipeline_status(project_id, commit_sha1):
     connection.request('GET', url=url)
     response = connection.getresponse()
     if response.code != http.HTTPStatus.OK:
-        raise ValueError("Failed to receive a successful response")
+        raise CommunicationFailure("Failed to receive a successful response")
     json_response = json.loads(response.read())
 
     # As far as I can tell, there should be only one pipeline for the same
     # project + commit. 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")
+        raise NoPipelineFound("No pipeline found")
     return json_response[0]
 
 
-- 
2.25.4

[PATCH 7/7] scripts/ci/gitlab-pipeline-status: wait for pipeline creation

[Cleber Rosa]

When called in wait mode, this script will also wait for the pipeline
to be get to a "running" state.  Because many more status may be seen
until a pipeline gets to "running", and those need to be handle too.

Reference: https://docs.gitlab.com/ee/api/pipelines.html#list-project-pipelines
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/ci/gitlab-pipeline-status | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index 628150ce0b..bac8233079 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -83,13 +83,22 @@ def wait_on_pipeline_success(timeout, interval,
             print(msg)
             return False
 
-        status = get_pipeline_status(project_id, commit_sha)
-        if status['status'] == 'running':
-            print('running...')
+        try:
+            status = get_pipeline_status(project_id, commit_sha)
+        except NoPipelineFound:
+            print('Pipeline has not been found, it may not have been created yet.')
+            time.sleep(1)
+            continue
+
+        pipeline_status = status['status']
+        status_to_wait = ('created', 'waiting_for_resource', 'preparing',
+                          'pending', 'running')
+        if pipeline_status in status_to_wait:
+            print('%s...' % pipeline_status)
             time.sleep(interval)
             continue
 
-        if status['status'] == 'success':
+        if pipeline_status == 'success':
             return True
 
         msg = "Pipeline failed, check: %s" % status['web_url']
-- 
2.25.4