#407 Print response data from Pagure for debugging
Merged 3 years ago by onosek. Opened 3 years ago by onosek.
onosek/fedpkg pagure_debug  into  master

file modified
+19 -9
@@ -782,6 +782,7 @@ 

  

      def request_repo(self):

          self._request_repo(

+             logger=self.log,

              repo_name=self.args.name,

              ns=self.args.new_repo_namespace,

              branch='master',
@@ -798,23 +799,26 @@ 

  

      def request_tests_repo(self):

          self._request_repo(

+             logger=self.log,

              repo_name=self.args.name,

              ns='tests',

              description=self.args.description,

              bug=self.args.bug,

              name=self.name,

              config=self.config,

-             anongiturl=self.cmd.anongiturl

+             anongiturl=self.cmd.anongiturl,

          )

  

      @staticmethod

-     def _request_repo(repo_name, ns, description, name, config, branch=None,

-                       summary=None, upstreamurl=None, monitor=None, bug=None,

-                       exception=None, anongiturl=None, initial_commit=True):

+     def _request_repo(logger, repo_name, ns, description, name, config,

+                       branch=None, summary=None, upstreamurl=None,

+                       monitor=None, bug=None, exception=None, anongiturl=None,

+                       initial_commit=True):

          """ Implementation of `request_repo`.

  

          Submits a request for a new dist-git repo.

  

+         :param logger: A logger object.

          :param repo_name: The repository name string.  Typically the

              value of `self.cmd.repo_name`.

          :param ns: The repository namespace string, i.e. 'rpms' or 'modules'.
@@ -906,7 +910,7 @@ 

          pagure_url = config_get_safely(config, pagure_section, 'url')

          pagure_token = config_get_safely(config, pagure_section, 'token')

          print(new_pagure_issue(

-             pagure_url, pagure_token, ticket_title, ticket_body, name))

+             logger, pagure_url, pagure_token, ticket_title, ticket_body, name))

  

      def request_branch(self):

          if self.args.repo_name_for_branch:
@@ -918,6 +922,7 @@ 

          except rpkgError:

              active_branch = None

          self._request_branch(

+             logger=self.log,

              service_levels=self.args.sl,

              all_releases=self.args.all_releases,

              branch=self.args.branch,
@@ -931,13 +936,14 @@ 

          )

  

      @staticmethod

-     def _request_branch(service_levels, all_releases, branch, active_branch,

-                         repo_name, ns, no_git_branch, no_auto_module,

-                         name, config):

+     def _request_branch(logger, service_levels, all_releases, branch,

+                         active_branch, repo_name, ns, no_git_branch,

+                         no_auto_module, name, config):

          """ Implementation of `request_branch`.

  

          Submits a request for a new branch of a given dist-git repo.

  

+         :param logger: A logger object.

          :param service_levels: A list of service level strings.  Typically the

              value of `self.args.service_levels`.

          :param all_releases: A boolean indicating if this request should be made
@@ -1055,7 +1061,8 @@ 

                  b, ns, repo_name)

  

              print(new_pagure_issue(

-                 pagure_url, pagure_token, ticket_title, ticket_body, name))

+                 logger, pagure_url, pagure_token, ticket_title, ticket_body,

+                 name))

  

              # For non-standard rpm branch requests, also request a matching new

              # module repo with a matching branch.
@@ -1069,6 +1076,7 @@ 

                  summary = ('Automatically requested module for '

                             'rpms/%s:%s.' % (repo_name, b))

                  fedpkgClient._request_repo(

+                     logger=logger,

                      repo_name=repo_name,

                      ns='modules',

                      branch='master',
@@ -1082,6 +1090,7 @@ 

                      config=config,

                  )

                  fedpkgClient._request_branch(

+                     logger=logger,

                      service_levels=service_levels,

                      all_releases=all_releases,

                      branch=b,
@@ -1109,6 +1118,7 @@ 

          distgit_token = config_get_safely(self.config, distgit_section, 'token')

  

          ret = do_fork(

+             logger=self.log,

              base_url=distgit_api_base_url,

              token=distgit_token,

              repo_name=self.cmd.repo_name,

file modified
+21 -11
@@ -84,9 +84,10 @@ 

          return None

  

  

- def new_pagure_issue(url, token, title, body, cli_name):

+ def new_pagure_issue(logger, url, token, title, body, cli_name):

      """

      Posts a new Pagure issue

+     :param logger: A logger object

      :param url: a string of the URL to Pagure

      :param token: a string of the Pagure API token that has rights to create

      a ticket
@@ -117,13 +118,18 @@ 

  

      base_error_msg = ('The following error occurred while creating a new '

                        'issue in Pagure: {0}')

+ 

+     try:

+         # Extract response error text (and debug data)

+         rv_json = rv.json()

+         logger.debug("Pagure API response: '{0}'".format(rv_json))

+         rv_error = rv_json.get('error')

+     except (ValueError, AttributeError):

+         rv_error = rv.text

+ 

      if not rv.ok:

          # Lets see if the API returned an error message in JSON that we can

          # show the user

-         try:

-             rv_error = rv.json().get('error')

-         except ValueError:

-             rv_error = rv.text

          # show hint for expired token

          if re.search(r"Invalid or expired token", rv_error, re.IGNORECASE):

              base_error_msg += '\nFor invalid or expired token refer to ' \
@@ -135,9 +141,10 @@ 

          url.rstrip('/'), rv.json()['issue']['id'])

  

  

- def do_fork(base_url, token, repo_name, namespace, cli_name):

+ def do_fork(logger, base_url, token, repo_name, namespace, cli_name):

      """

      Creates a fork of the project.

+     :param logger: A logger object

      :param base_url: a string of the URL repository

      :param token: a string of the API token that has rights to make a fork

      :param repo_name: a string of the repository name
@@ -166,15 +173,18 @@ 

                       'create a new fork. The error was: {0}'.format(str(error)))

          raise rpkgError(error_msg)

  

+     try:

+         # Extract response error text (and debug data)

+         rv_json = rv.json()

+         logger.debug("Pagure API response: '{0}'".format(rv_json))

+         rv_error = rv_json.get('error')

+     except (ValueError, AttributeError):

+         rv_error = rv.text

+ 

      base_error_msg = ('The following error occurred while creating a new fork: {0}')

      if not rv.ok:

          # Lets see if the API returned an error message in JSON that we can

          # show the user

-         try:

-             rv_error = rv.json().get('error')

-         except ValueError:

-             rv_error = rv.text

- 

          if re.search(r"Repo .+ already exists", rv_error, re.IGNORECASE):

              return False

  

file modified
+9 -4
@@ -355,22 +355,24 @@ 

  

      def test_raise_error_if_connection_error(self, post):

          post.side_effect = ConnectionError

+         logger = Mock()

  

          six.assertRaisesRegex(

              self, rpkgError, 'The connection to Pagure failed',

              utils.new_pagure_issue,

-             'http://distgit/', '123456', 'new package', {'repo': 'pkg1'}, 'fedpkg')

+             logger, 'http://distgit/', '123456', 'new package', {'repo': 'pkg1'}, 'fedpkg')

  

      def test_responses_not_ok_and_response_body_is_not_json(self, post):

          rv = Mock(ok=False, text='error')

          rv.json.side_effect = ValueError

          post.return_value = rv

+         logger = Mock()

  

          six.assertRaisesRegex(

              self, rpkgError,

              'The following error occurred while creating a new issue',

              utils.new_pagure_issue,

-             'http://distgit/', '123456', 'new package', {'repo': 'pkg1'}, 'fedpkg')

+             logger, 'http://distgit/', '123456', 'new package', {'repo': 'pkg1'}, 'fedpkg')

  

      def test_responses_not_ok_when_token_is_expired(self, post):

          rv = Mock(
@@ -379,23 +381,26 @@ 

                   'https://pagure.io/settings#api-keys to get or renew your API token.')

          rv.json.side_effect = ValueError

          post.return_value = rv

+         logger = Mock()

  

          six.assertRaisesRegex(

              self, rpkgError,

              'For invalid or expired token refer to "fedpkg request-repo -h" to set '

              'a token in your user configuration.',

              utils.new_pagure_issue,

-             'http://distgit/', '123456', 'new package', {'repo': 'pkg1'}, 'fedpkg')

+             logger, 'http://distgit/', '123456', 'new package', {'repo': 'pkg1'}, 'fedpkg')

  

      def test_create_pagure_issue(self, post):

          rv = Mock(ok=True)

          rv.json.return_value = {'issue': {'id': 1}}

          post.return_value = rv

+         logger = Mock()

  

          pagure_api_url = 'http://distgit'

          issue_ticket_body = {'repo': 'pkg1'}

  

-         issue_url = utils.new_pagure_issue(pagure_api_url,

+         issue_url = utils.new_pagure_issue(logger,

+                                            pagure_api_url,

                                             '123456',

                                             'new package',

                                             issue_ticket_body,

When fedpkg communicates with a web API (Pagure or DistGit), prints
(json) response data for debugging. The response is shown when there
is not OK result and verbose mode has to be enabled.

Fixes: #396
JIRA: RHELCMP-486
Signed-off-by: Ondrej Nosek onosek@redhat.com

1) Is it better to show the response data always (of course with the "debug" mode enabled) or just when non-OK result returns (default)?
2) Actually, "debug" mode is enabled with '--verbose' | '-v' param and not with '--debug'. Maybe some general reconsideration of these params is needed.

:thumbsup: I wouldn't worry about printing the response all the time. It may be interesting even on success.

rebased onto a2746ffd88ab76c99030ab6e58877428d018cfd8

3 years ago

rebased onto c0556976eb81208687d44ea9da805b04334aef05

3 years ago

The response is shown always (in "debug" mode).

rebased onto 257185a

3 years ago

Pull-Request has been merged by onosek

3 years ago