Add a --debug argument and some logging when retrieving CI status #2467

pypingou · 2018-07-03T12:55:06Z

No description provided.

bowlofeggs · 2018-07-24T16:58:35Z

bodhi/client/bindings.py

+            ch = logging.StreamHandler()
+            ch.setLevel(logging.DEBUG)
+            log.addHandler(ch)
+            log.setLevel(logging.DEBUG)


libraries should not set up logging handlers or set log levels. This library is not just used by the bodhi cli and so it should be unopinionated about logging. It's OK for the lib to log things at debug level, but the logger should not be configured by it.

Any preference where we could configure it?
I thought this was a good place since it was common with all the actions performed by the CLI, but I'm fine w/ changing this :)

bowlofeggs · 2018-07-25T18:40:28Z

On 07/25/2018 11:03 AM, Pierre-Yves Chibon wrote: Any preference where we could configure it?

We should do it in bodhi/client/__init__.py. One way I can think of to do it as a common action so you don't have to repeat code is you could set up a validator on the --debug flag that does this if --debug is set. There's already a validator in there on at least one flag as an example.

pypingou · 2018-07-26T11:23:54Z

One way I can think of to do it as a common action so you don't have to repeat code is you could set up a validator on the --debug flag that does this if --debug is set. There's already a validator in there on at least one flag as an example.

Is the validator you have in mind the callback used for the --url argument? If not then I need to look further for this validator as I think I didn't find it then :s

I took a little bit of a different approach in the mean while, I've added the --debug argument to the cli() method, so all actions have it, but it needs to be specified at the beginning of the argument list, for example:
bodhi --debug updates waive python-arrow-0.8.0-5.fc28 --show --staging
works while:
bodhi updates waive python-arrow-0.8.0-5.fc28 --show --staging --debug does not.

Again, if you prefer the validator approach I'll look into it some more :)

bowlofeggs · 2018-07-26T18:40:08Z

On 07/26/2018 07:23 AM, Pierre-Yves Chibon wrote: Is the validator you have in mind the |callback| used for the |--url| argument? If not then I need to look further for this validator as I think I didn't find it then :s

Yeah it's the callback system.

I took a little bit of a different approach in the mean while, I've added the |--debug| argument to the |cli()| method, so all actions have it, but it needs to be specified at the beginning of the argument list, for example: |bodhi --debug updates waive python-arrow-0.8.0-5.fc28 --show --staging| works while: |bodhi updates waive python-arrow-0.8.0-5.fc28 --show --staging --debug| does not. Again, if you prefer the validator approach I'll look into it some more :)

I think I prefer the validator approach since it will be more consistent with how the rest of Bodhi's flags work, unless you can find a way to make it work both ways (I don't know a way to do it off the top of my head, but it might be possible.)

pypingou · 2018-07-27T07:28:41Z

I've pushed a commit porting --debug to use a callback mechanism as --url does. You will see it requires more changes than the cli() approach. If you're fine with that approach, I'll merge the third commit into the first one to clean up the history :)

pypingou · 2018-07-27T07:29:53Z

Rebased to fix the conflict

pypingou · 2018-07-27T10:17:22Z

--hm these tests are definitively passing locally :(--

Nevermind the last comment from jenkins confused me, they are also passing on jenkins now it seems :)

bowlofeggs · 2018-07-27T16:26:34Z

bodhi/client/__init__.py

    client = bindings.BodhiClient(base_url=url, staging=kwargs['staging'])
+


This is an unrelated style change, so let's leave it as it is.

bowlofeggs · 2018-07-27T16:57:59Z

bodhi/client/__init__.py

@@ -295,7 +327,7 @@ def new(user, password, url, **kwargs):
                       True.
        kwargs (dict): Other keyword arguments passed to us by click.
    """
-
+    kwargs.pop('debug')


Is this what you meant by being a larger change? If so, a cleaner way to do this is to add debug as a parameter to each of these functions so it doesn't end up in kwargs.

Good idea, will fix

bowlofeggs

Just a few more minor changes.

bowlofeggs · 2018-08-01T14:23:55Z

bodhi/client/__init__.py

@@ -244,7 +274,8 @@ def composes():
 @staging_option
 @click.option('-v', '--verbose', is_flag=True, default=False, help='Display more information.')
 @url_option
-def list_composes(url, staging, verbose):
+@debug_option
+def list_composes(url, staging, verbose, debug):


Note that we still document parameters in the "developer docs" section below, here and elsewhere in the file.

bowlofeggs · 2018-08-01T14:24:19Z

bodhi/client/__init__.py

@@ -295,7 +327,6 @@ def new(user, password, url, **kwargs):
                       True.
        kwargs (dict): Other keyword arguments passed to us by click.
    """
-


This is an unrelated style change.

will remove

bowlofeggs · 2018-08-01T14:32:17Z

bodhi/client/__init__.py

 @add_options(pagination_options)
 @handle_errors
-def query(url, mine=False, rows=None, **kwargs):
+def query(url, mine=False, rows=None, debug=False, **kwargs):


Let's not define the default for debug in two places. If you move this to be after url, you don't need to give it a default.

bowlofeggs · 2018-08-01T14:34:27Z

bodhi/tests/client/test___init__.py

+                mock.MagicMock(return_value='a_csrf_token'))
+    @mock.patch('bodhi.client.bindings.BodhiClient.send_request',
+                return_value=client_test_data.EXAMPLE_UPDATE_MUNCH, autospec=True)
+    def test_severity_and_debug_flags(self, send_request):


This test is really about testing the debug flag, so let's name it test_debug_flag for simplicity.

Will adjust

pypingou

thanks for the review

pypingou · 2018-08-01T15:30:47Z

bodhi/client/__init__.py

@@ -244,7 +274,8 @@ def composes():
 @staging_option
 @click.option('-v', '--verbose', is_flag=True, default=False, help='Display more information.')
 @url_option
-def list_composes(url, staging, verbose):
+@debug_option
+def list_composes(url, staging, verbose, debug):


pypingou · 2018-08-01T15:31:09Z

bodhi/client/__init__.py

@@ -295,7 +327,6 @@ def new(user, password, url, **kwargs):
                       True.
        kwargs (dict): Other keyword arguments passed to us by click.
    """
-


will remove

pypingou · 2018-08-01T15:31:19Z

bodhi/client/__init__.py

 @add_options(pagination_options)
 @handle_errors
-def query(url, mine=False, rows=None, **kwargs):
+def query(url, mine=False, rows=None, debug=False, **kwargs):


pypingou · 2018-08-01T15:32:59Z

bodhi/tests/client/test___init__.py

+                mock.MagicMock(return_value='a_csrf_token'))
+    @mock.patch('bodhi.client.bindings.BodhiClient.send_request',
+                return_value=client_test_data.EXAMPLE_UPDATE_MUNCH, autospec=True)
+    def test_severity_and_debug_flags(self, send_request):


Will adjust

pypingou · 2018-08-01T15:35:25Z

I think I got all of your comments, I also merged the new approach into the old one making the history cleaner.

bowlofeggs · 2018-08-01T16:06:14Z

I forgot to mention when reviewing this one - can you also squash your commits into 1? Thanks!

bowlofeggs

Just 3 small things I'd like to fix, then we'll be good to merge.

bowlofeggs · 2018-08-15T15:16:31Z

bodhi/client/__init__.py

+    Args:
+        ctx (click.core.Context): The Click context. Unused.
+        param (click.core.Option): The option being handled. Unused.
+        value (unicode): The value of the --debug flag.


This is a bool, not a unicode.

bowlofeggs · 2018-08-15T15:16:43Z

bodhi/client/__init__.py

+        param (click.core.Option): The option being handled. Unused.
+        value (unicode): The value of the --debug flag.
+    Returns:
+        unicode: The value of the --debug flag.


This is also a bool.

bowlofeggs · 2018-08-15T15:19:23Z

bodhi/client/__init__.py

 url_option = click.option('--url', envvar='BODHI_URL', default=bindings.BASE_URL,
                          help=('URL of a Bodhi server. Ignored if --staging is set. Can be set '
                                'with BODHI_URL environment variable'),
                          callback=_warn_if_url_and_staging_set)
 staging_option = click.option('--staging', help='Use the staging bodhi instance',
                              is_flag=True, default=False)
-


Unrelated style change.

bowlofeggs · 2018-08-15T15:23:47Z

I will fix these three things myself.

This creates a stream handler to the log and set its level to 'DEBUG'. This way we can use more log.info()/log.debug() in the binding files and ask users to run the CLI with --debug to give us more context as to what is going on. Signed-off-by: Pierre-Yves Chibon <[email protected]>

This way when running with --debug, we should be able to have more clues as to what is going on (if anything). Signed-off-by: Pierre-Yves Chibon <[email protected]>

Signed-off-by: Pierre-Yves Chibon <[email protected]>

pypingou · 2018-08-16T10:08:17Z

Thanks @bowlofeggs :)

bowlofeggs · 2018-08-28T17:33:16Z

These patches are planned to be included in the upcoming 3.10.0 release: #2556.

bowlofeggs · 2018-08-31T16:40:44Z

These patches are available in Copr:

https://copr.fedorainfracloud.org/coprs/bowlofeggs/bodhi-pre-release/

pypingou force-pushed the debug_mode branch from 97b3227 to 27555b8 Compare July 3, 2018 14:07

fedora-infra deleted a comment from centos-ci Jul 3, 2018

pypingou force-pushed the debug_mode branch 2 times, most recently from 03201bf to 90e07fb Compare July 3, 2018 14:53

fedora-infra deleted a comment from centos-ci Jul 3, 2018

bowlofeggs suggested changes Jul 24, 2018

View reviewed changes

pypingou force-pushed the debug_mode branch from 90e07fb to fa15397 Compare July 26, 2018 11:19

pypingou requested a review from a team as a code owner July 26, 2018 11:19

fedora-infra deleted a comment from centos-ci Jul 26, 2018

pypingou force-pushed the debug_mode branch from fa15397 to 04f3540 Compare July 26, 2018 13:23

pypingou force-pushed the debug_mode branch from 9ca477b to 9489a18 Compare July 27, 2018 07:29

pypingou force-pushed the debug_mode branch from 9489a18 to 5600453 Compare July 27, 2018 08:08

fedora-infra deleted a comment from centos-ci Jul 27, 2018

bowlofeggs suggested changes Jul 27, 2018

View reviewed changes

pypingou force-pushed the debug_mode branch from 5600453 to 1f6902f Compare July 30, 2018 09:57

bowlofeggs suggested changes Aug 1, 2018

View reviewed changes

pypingou commented Aug 1, 2018

View reviewed changes

pypingou force-pushed the debug_mode branch from 1f6902f to 4ba6b51 Compare August 1, 2018 15:34

pypingou force-pushed the debug_mode branch from 4ba6b51 to 02b8c6f Compare August 1, 2018 15:35

bowlofeggs suggested changes Aug 15, 2018

View reviewed changes

bowlofeggs self-assigned this Aug 15, 2018

pypingou added 3 commits August 15, 2018 11:25

Log some information when retrieving CI status from greenwave via bodhi

a906100

This way when running with --debug, we should be able to have more clues as to what is going on (if anything). Signed-off-by: Pierre-Yves Chibon <[email protected]>

Replace deprecated log.warn() but the correct log.warning()

d85afb7

Signed-off-by: Pierre-Yves Chibon <[email protected]>

bowlofeggs force-pushed the debug_mode branch from 02b8c6f to d85afb7 Compare August 15, 2018 15:25

bowlofeggs approved these changes Aug 15, 2018

View reviewed changes

mergify bot merged commit 31c1081 into develop Aug 15, 2018

bowlofeggs deleted the debug_mode branch August 15, 2018 16:46

		client = bindings.BodhiClient(base_url=url, staging=kwargs['staging'])

Add a --debug argument and some logging when retrieving CI status #2467

Add a --debug argument and some logging when retrieving CI status #2467

Conversation

pypingou commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs commented Jul 25, 2018 via email

pypingou commented Jul 26, 2018

bowlofeggs commented Jul 26, 2018 via email

pypingou commented Jul 27, 2018

pypingou commented Jul 27, 2018

pypingou commented Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pypingou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pypingou commented Aug 1, 2018

bowlofeggs commented Aug 1, 2018 via email

bowlofeggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs commented Aug 15, 2018

pypingou commented Aug 16, 2018

bowlofeggs commented Aug 28, 2018

bowlofeggs commented Aug 31, 2018

pypingou commented Jul 27, 2018 •

edited

Loading