Switch CLI over to botocore clients #1220
Conversation
This commit updates the events emitted in cli driver to emit service/operation object alternatives. This change allows handlers to be updated to not require service/operation objects. In many cases, some handlers took an `operation` object, but did not actually use this, so these changes were minimal. Note that this puts the CLI in an interim state. The plan is to not merge this into the develop branch until clidriver is switched over, but this will allow a common base for all customization switchovers to be based on.
We don't really need the help text for these tests
The cli.json no longer uses references like this anymore.
Tests need to be updated.
These are covered elsewhere in the codebase, or they no longer apply given changes to clidriver.
The biggest change by far is the fact the clients no longer accept snake_case for any of the parameters that make it to the .invoke() method. Botocore's operation objects were lenient and allowed for both. Many of the handlers relied on this fact.
Note that the docs still use operation and service objects
We must use the casing required by the model when we call into botocore. snake_casing cannot be used. I think we can clean up how this boolean remapping is done. Right now I have to reach into internal attributes to do this.
Not needed anymore.
| @@ -52,7 +52,8 @@ def _flush_stream(self, stream): | |||
|
|
|||
|
|
|||
| class FullyBufferedFormatter(Formatter): | |||
| def __call__(self, operation, response, stream=None): | |||
| def __call__(self, command_name, response, | |||
| is_response_paginated=False, stream=None): | |||
There was a problem hiding this comment.
I think I can clean up this interface more. Instead of having to pass a response object and a flag saying if it's a paginated response, I could just isinstance the response to check if it's paginated. Either that or do a duck type check for build_full_result and follow that code path if response has that object.
Thoughts?
There was a problem hiding this comment.
Cleaning it up would be nice. I thought the is_response_paginated interface was a little awkward when I was updating the emr commands that was using it.
I do not have a preference on what check you make. I feel that checking for build_full_response would be fine since you call the method within the if statement checking if you can paginate. That way you can avoid an extra import as well for isinstance
There was a problem hiding this comment.
Pushed a change that updates this.
Based on review feedback.
To be consistent with what botocore uses.
| :type operation_object: ``botocore.operation.Operation`` | ||
| :param operation_object: The operation object associated with | ||
| this object. | ||
|
|
There was a problem hiding this comment.
Missing docs for operation_model and event_emitter?
|
Very minor comments from me. I am also curious about the |
|
@danielgtaylor I believe I've incorporated all your feedback. |
|
Ok, I'd like to get a list of all the remaining work here. I believe I've incorporated all the feedback except for a few outstanding issues:
Anything else I've missed? cc @kyleknap @danielgtaylor |
|
I made replies. For the first bullet point, I do not think there is much we can do about I cannot think of any other outstanding points other than the one where Otherwise, looks good 🚢 |
|
Hmm, it's a little concerning to me that even after changing the code I didn't get a failing test. It sounds like there's nothing checking this part of the code. I would have expected a test failure either before/after the change. I'll look into possibly getting a test in place. |
|
I retract my previous statement :). @kyleknap points out that the |
|
@jamesls |
|
Ok, I've added the method back. I believe that's all the feedback. Anything else? |
|
LGTM 🚢-it |
|
I do not think there is anything more. Thanks for seeing it through! 🚢 |
This removes dependencies on botocore's service/operation objects. High level this is done in two ways:
botocore.model.*classes.cc @kyleknap @danielgtaylor