[PATCH 0/3] gitlab-pipeline-status script: provide more information on errors

[PATCH 0/3] gitlab-pipeline-status script: provide more information on errors

[Cleber Rosa]

When things go wrong with the GitLab API requests, it's useful to give
users more information about the possible causes.

Cleber Rosa (3):
  scripts/ci/gitlab-pipeline-status: split utlity function for HTTP GET
  scripts/ci/gitlab-pipeline-status: give more information on failures
  scripts/ci/gitlab-pipeline-status: give more info when pipeline not
    found

 scripts/ci/gitlab-pipeline-status | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

-- 
2.25.4

[PATCH 1/3] scripts/ci/gitlab-pipeline-status: split utlity function for HTTP GET

[Cleber Rosa]

This simply splits out the code that does an HTTP GET so that it
can be used for other API requests.

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

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index 78e72f6008..0c1e8bd8a7 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -48,18 +48,25 @@ def get_local_branch_commit(branch):
     return result
 
 
-def get_pipeline_status(project_id, commit_sha1):
+def get_json_http_response(url):
     """
-    Returns the JSON content of the pipeline status API response
+    Returns the JSON content of an HTTP GET request to gitlab.com
     """
-    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 CommunicationFailure("Failed to receive a successful response")
-    json_response = json.loads(response.read())
+    return json.loads(response.read())
+
+
+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)
+    json_response = get_json_http_response(url)
 
     # 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
-- 
2.25.4

[PATCH 2/3] scripts/ci/gitlab-pipeline-status: give more information on failures

[Cleber Rosa]

When an HTTP GET request fails, it's useful to go beyond the "not
successful" message, and show the code returned by the server.

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

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index 0c1e8bd8a7..ad62ab3cfc 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -56,7 +56,9 @@ def get_json_http_response(url):
     connection.request('GET', url=url)
     response = connection.getresponse()
     if response.code != http.HTTPStatus.OK:
-        raise CommunicationFailure("Failed to receive a successful response")
+        msg = "Received unsuccessful response: %s (%s)" % (response.code,
+                                                           response.reason)
+        raise CommunicationFailure(msg)
     return json.loads(response.read())
 
 
-- 
2.25.4

[PATCH 3/3] scripts/ci/gitlab-pipeline-status: give more info when pipeline not found

[Cleber Rosa]

This includes both input parameters (project id and commit) in the
message so to make it easier to debug returned API calls.

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

diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
index ad62ab3cfc..924db327ff 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -74,7 +74,9 @@ def get_pipeline_status(project_id, commit_sha1):
     # 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 NoPipelineFound("No pipeline found")
+        msg = "No pipeline found for project %s and commit %s" % (project_id,
+                                                                  commit_sha1)
+        raise NoPipelineFound(msg)
     return json_response[0]
 
 
-- 
2.25.4

Re: [PATCH 1/3] scripts/ci/gitlab-pipeline-status: split utlity function for HTTP GET

[Wainer dos Santos Moschetta]

Hi,

On 2/22/21 4:32 PM, Cleber Rosa wrote:
> This simply splits out the code that does an HTTP GET so that it
> can be used for other API requests.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   scripts/ci/gitlab-pipeline-status | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)

Only fix the "utlity" typo in $SUBJECT:

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

>
> diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
> index 78e72f6008..0c1e8bd8a7 100755
> --- a/scripts/ci/gitlab-pipeline-status
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -48,18 +48,25 @@ def get_local_branch_commit(branch):
>       return result
>   
>   
> -def get_pipeline_status(project_id, commit_sha1):
> +def get_json_http_response(url):
>       """
> -    Returns the JSON content of the pipeline status API response
> +    Returns the JSON content of an HTTP GET request to gitlab.com
>       """
> -    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 CommunicationFailure("Failed to receive a successful response")
> -    json_response = json.loads(response.read())
> +    return json.loads(response.read())
> +
> +
> +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)
> +    json_response = get_json_http_response(url)
>   
>       # 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

Re: [PATCH 2/3] scripts/ci/gitlab-pipeline-status: give more information on failures

[Wainer dos Santos Moschetta]

On 2/22/21 4:32 PM, Cleber Rosa wrote:
> When an HTTP GET request fails, it's useful to go beyond the "not
> successful" message, and show the code returned by the server.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   scripts/ci/gitlab-pipeline-status | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
> index 0c1e8bd8a7..ad62ab3cfc 100755
> --- a/scripts/ci/gitlab-pipeline-status
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -56,7 +56,9 @@ def get_json_http_response(url):
>       connection.request('GET', url=url)
>       response = connection.getresponse()
>       if response.code != http.HTTPStatus.OK:
> -        raise CommunicationFailure("Failed to receive a successful response")
> +        msg = "Received unsuccessful response: %s (%s)" % (response.code,
> +                                                           response.reason)
> +        raise CommunicationFailure(msg)
>       return json.loads(response.read())
>   
>   

Re: [PATCH 3/3] scripts/ci/gitlab-pipeline-status: give more info when pipeline not found

[Wainer dos Santos Moschetta]

On 2/22/21 4:32 PM, Cleber Rosa wrote:
> This includes both input parameters (project id and commit) in the
> message so to make it easier to debug returned API calls.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   scripts/ci/gitlab-pipeline-status | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
> index ad62ab3cfc..924db327ff 100755
> --- a/scripts/ci/gitlab-pipeline-status
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -74,7 +74,9 @@ def get_pipeline_status(project_id, commit_sha1):
>       # 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 NoPipelineFound("No pipeline found")
> +        msg = "No pipeline found for project %s and commit %s" % (project_id,
> +                                                                  commit_sha1)
> +        raise NoPipelineFound(msg)
>       return json_response[0]
>   
>   

Re: [PATCH 0/3] gitlab-pipeline-status script: provide more information on errors

[Wainer dos Santos Moschetta]

Hi Cleber,

In case you need to send a v2, mind to add the following patch together?

commit 3c4ed8a78e096e4d7df0398c29887a9d468ae120 (HEAD -> gitlab_runners)
Author: Wainer dos Santos Moschetta <wainersm@redhat.com>
Date:   Tue Feb 23 11:26:08 2021 -0300

     scripts/ci/gitlab-pipeline-status: Handle ValueError exceptions nicely

     With this change, when getting the local branch, it will handle nicely
     any threw ValueError exception instead of print the stack trace.

     Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

diff --git a/scripts/ci/gitlab-pipeline-status 
b/scripts/ci/gitlab-pipeline-status
index 924db327ff..6177df973a 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -160,7 +160,11 @@ def main():
      args = parser.parse_args()

      if not args.commit:
-        args.commit = get_local_branch_commit(args.branch)
+        try:
+            args.commit = get_local_branch_commit(args.branch)
+        except ValueError as error:
+            print("ERROR: %s" % error)
+            sys.exit(1)

      success = False
      try:

On 2/22/21 4:32 PM, Cleber Rosa wrote:
> When things go wrong with the GitLab API requests, it's useful to give
> users more information about the possible causes.
>
> Cleber Rosa (3):
>    scripts/ci/gitlab-pipeline-status: split utlity function for HTTP GET
>    scripts/ci/gitlab-pipeline-status: give more information on failures
>    scripts/ci/gitlab-pipeline-status: give more info when pipeline not
>      found
>
>   scripts/ci/gitlab-pipeline-status | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
>

Re: [PATCH 0/3] gitlab-pipeline-status script: provide more information on errors

[Erik Skultety]

On Tue, Feb 23, 2021 at 11:52:17AM -0300, Wainer dos Santos Moschetta wrote:
> Hi Cleber,
> 
> In case you need to send a v2, mind to add the following patch together?
> 
> commit 3c4ed8a78e096e4d7df0398c29887a9d468ae120 (HEAD -> gitlab_runners)
> Author: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Date:   Tue Feb 23 11:26:08 2021 -0300
> 
>     scripts/ci/gitlab-pipeline-status: Handle ValueError exceptions nicely
> 
>     With this change, when getting the local branch, it will handle nicely
>     any threw ValueError exception instead of print the stack trace.
> 
>     Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> 
> diff --git a/scripts/ci/gitlab-pipeline-status
> b/scripts/ci/gitlab-pipeline-status
> index 924db327ff..6177df973a 100755
> --- a/scripts/ci/gitlab-pipeline-status
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -160,7 +160,11 @@ def main():
>      args = parser.parse_args()
> 
>      if not args.commit:
> -        args.commit = get_local_branch_commit(args.branch)
> +        try:
> +            args.commit = get_local_branch_commit(args.branch)
> +        except ValueError as error:
> +            print("ERROR: %s" % error)
> +            sys.exit(1)

1 is the default error code, so you should pass the error message to sys.exit
directly without the print. If you don't want that, at least redirect the
print to sys.stderr.

Erik

Re: [PATCH 0/3] gitlab-pipeline-status script: provide more information on errors

[Wainer dos Santos Moschetta]

Hi,

On 2/23/21 12:19 PM, Erik Skultety wrote:
> On Tue, Feb 23, 2021 at 11:52:17AM -0300, Wainer dos Santos Moschetta wrote:
>> Hi Cleber,
>>
>> In case you need to send a v2, mind to add the following patch together?
>>
>> commit 3c4ed8a78e096e4d7df0398c29887a9d468ae120 (HEAD -> gitlab_runners)
>> Author: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> Date:   Tue Feb 23 11:26:08 2021 -0300
>>
>>      scripts/ci/gitlab-pipeline-status: Handle ValueError exceptions nicely
>>
>>      With this change, when getting the local branch, it will handle nicely
>>      any threw ValueError exception instead of print the stack trace.
>>
>>      Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>
>> diff --git a/scripts/ci/gitlab-pipeline-status
>> b/scripts/ci/gitlab-pipeline-status
>> index 924db327ff..6177df973a 100755
>> --- a/scripts/ci/gitlab-pipeline-status
>> +++ b/scripts/ci/gitlab-pipeline-status
>> @@ -160,7 +160,11 @@ def main():
>>       args = parser.parse_args()
>>
>>       if not args.commit:
>> -        args.commit = get_local_branch_commit(args.branch)
>> +        try:
>> +            args.commit = get_local_branch_commit(args.branch)
>> +        except ValueError as error:
>> +            print("ERROR: %s" % error)
>> +            sys.exit(1)
> 1 is the default error code, so you should pass the error message to sys.exit
> directly without the print. If you don't want that, at least redirect the
> print to sys.stderr.

There are occurrences of the same pattern on the script, so I better 
send a separate series to change them all. Thanks for pointing it out Erik!

- Wainer

>
> Erik

Re: [PATCH 1/3] scripts/ci/gitlab-pipeline-status: split utlity function for HTTP GET

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> This simply splits out the code that does an HTTP GET so that it
> can be used for other API requests.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

fix sp as per Wainer, otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  scripts/ci/gitlab-pipeline-status | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
> index 78e72f6008..0c1e8bd8a7 100755
> --- a/scripts/ci/gitlab-pipeline-status
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -48,18 +48,25 @@ def get_local_branch_commit(branch):
>      return result
>  
>  
> -def get_pipeline_status(project_id, commit_sha1):
> +def get_json_http_response(url):
>      """
> -    Returns the JSON content of the pipeline status API response
> +    Returns the JSON content of an HTTP GET request to gitlab.com
>      """
> -    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 CommunicationFailure("Failed to receive a successful response")
> -    json_response = json.loads(response.read())
> +    return json.loads(response.read())
> +
> +
> +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)
> +    json_response = get_json_http_response(url)
>  
>      # 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


-- 
Alex Bennée

Re: [PATCH 2/3] scripts/ci/gitlab-pipeline-status: give more information on failures

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> When an HTTP GET request fails, it's useful to go beyond the "not
> successful" message, and show the code returned by the server.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  scripts/ci/gitlab-pipeline-status | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/ci/gitlab-pipeline-status b/scripts/ci/gitlab-pipeline-status
> index 0c1e8bd8a7..ad62ab3cfc 100755
> --- a/scripts/ci/gitlab-pipeline-status
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -56,7 +56,9 @@ def get_json_http_response(url):
>      connection.request('GET', url=url)
>      response = connection.getresponse()
>      if response.code != http.HTTPStatus.OK:
> -        raise CommunicationFailure("Failed to receive a successful response")
> +        msg = "Received unsuccessful response: %s (%s)" % (response.code,
> +                                                           response.reason)
> +        raise CommunicationFailure(msg)
>      return json.loads(response.read())


-- 
Alex Bennée

Re: [PATCH 0/3] gitlab-pipeline-status script: provide more information on errors

[Thomas Huth]

On 22/02/2021 20.32, Cleber Rosa wrote:
> When things go wrong with the GitLab API requests, it's useful to give
> users more information about the possible causes.
> 
> Cleber Rosa (3):
>    scripts/ci/gitlab-pipeline-status: split utlity function for HTTP GET
>    scripts/ci/gitlab-pipeline-status: give more information on failures
>    scripts/ci/gitlab-pipeline-status: give more info when pipeline not
>      found
> 
>   scripts/ci/gitlab-pipeline-status | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
> 

Thanks, I've queued the three patches now to my testing-next branch:

https://gitlab.com/thuth/qemu/-/commits/testing-next/

  Thomas