-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Json skeleton #963
Json skeleton #963
Conversation
return self._operation_caller.invoke( | ||
self._operation_object, call_parameters, parsed_globals) | ||
else: | ||
return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to add a comment about why we return 0
here, and determine whether zero should be used over e.g. None
. Does it matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is the return code, but I should check to make sure. Also, I agree that a comment would be good to explain why zero is returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked. The invoke()
method returns zero at the end.
https://github.com/aws/aws-cli/blob/develop/awscli/clidriver.py#L567
Overall I think this looks really good, just had a few questions about some edge cases and some uncovered use-cases. 👍 |
Other than the strange build failure, the code is ready to get looked at again. I incorporated all the feedback given except for the extra feature to |
I was just trying this out, and it looks like we have a bug for some existing CLI arguments where we can't set the required property:
I'm guessing this is because some customizations create their own custom argument classes they may not adhere to the CLIArgument interface. |
@@ -440,6 +457,9 @@ def arg_table(self): | |||
def __call__(self, args, parsed_globals): | |||
# Once we know we're trying to call a particular operation | |||
# of a service we can go ahead and load the parameters. | |||
event = 'building-argument-table-parser.%s.%s' % (self._parent_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a little confusing at first to call this event build-argument-table-parser, but we don't actually send the parser in the event. Compare to events like building-command-table and building-argument-table where we send the object being built. Maybe we can just rename this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense.
I wrote some some integration tests that test we get valid JSON output for any command supports generate-cli-skeleton: jamesls@c350ae3 Feel free to pull it into your branch to help with testing. The things it found were mostly:
|
a9329e9
to
6a42ca5
Compare
I updated the code such that it passes all of the generate cli skeleton integration tests. Changes include making it capable to set |
Added ``--generate-cli-skeleton`` to generate the JSON skeleton to fill out and ``--cli-input-json`` to use that skeleton as input to a CLI command.
791531c
to
2997d0b
Compare
2997d0b
to
7db6e6a
Compare
I further refactored the clidriver code to make the events more general. Ready to be looked at again. |
Looks good. I like the generalized events to clidriver. |
This pull request made a couple changes and feature additions.
I added two new custom arguments:
--generate-cli-skeleton
: This uses botocoreArgumentGenerator
to print a JSON skeleton to standard output that can be used as input to the cli. This JSON can be redirected to a file and then filled out and used as input to the CLI.--cli-input-json
: This takes JSON (typically generated from--generate-cli-skeleton
) from either a string or some file and uses that as the input to the command line operation. Additional arguments can be added to command line. If there are collisions between the two, the value from argument is used over the value from the input JSONTo create these features, I made the following changes to the cli driver:
enable/disable_call_operation
: These are methods that can be called to prevent theServiceOperation
from performing the botocore service operation call.building-argument-table-parser
. This gives us the ability to determine all of the arguments that are going to be passed to the parser (including arguments that were added through a hook). This was needed to ensure the parser does not throw errors for missing required args.calling-service-operation
. This allows us to see the entire configuration of theServiceOperation
before a call is made to botocore. That way we can edit the parameters that are going to be included in the call or even determine if we want the call to happen.cc @jamesls @danielgtaylor