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
Add a -j/--json option for euca-describe-* commands #82
Conversation
This is an interesting idea! If the option is going to go into I guess I should ask, though: what do you want to accomplish with JSON-formatted output? If we're going to be adding alternative output formats then we might want a more extensible way of invoking them than a dedicated option for each one. The One other thing: all euca2ools changes that affect the command line interface must come with documentation. Would you please update the relevant man pages to reflect your changes? |
Arg('-j', '--json', | ||
action='store_const', const='true', route_to=None, | ||
help='''Output in json format''') | ||
] |
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.
In this project the first Arg in the list appears immediately after the opening square brace and closing braces appear immediately after whatever precedes them. Please remove the whitespace after the [
and before the ]
.
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.
OK, Done
Thanks for the prompt review! Yeah, your suggestion worked out well and seems cleaner. The rational is to allow easier parsing of the euca commands. We have a variety of tools for accounting and monitoring and it was pretty tedious to parse the formatted output. Since the euca commands already have the data stored internally in a nice format, it was straightforward to output as json. We only needed 2 or 3 of the commands but I figured I might as well do them all. Sure will do docs once you confirm the changes are ok and implementation is stable. |
@@ -72,9 +73,19 @@ class EC2Request(AWSQueryRequest, TabifyingMixin): | |||
AUTH_CLASS = requestbuilder.auth.aws.HmacV4Auth | |||
METHOD = 'POST' | |||
|
|||
ARGS = [Arg('-j', '--json', | |||
action='store_const', const='true', route_to=None, | |||
help='''Output in json format''')] |
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.
Let's make this more extensible by making it an option that takes an arg, like --pretty=json
, so we don't have to clutter the namespace as we add new output formats in the future.
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.
This has been fixed. Added pretty=json that accepts a list of choices (only json is supported for now).
As expected, that makes this much cleaner. :-) See the comments above. I will be out of town for the next week, so I apologize in advance if it takes a while before you get another response. |
How go things, @minnus? |
Outputs json format instead of the standard text formatting for these tools.
Make the code simpler by moving configuration to __init__ and renaming the subclass print_result method.
Per gholms, json may not always appear in self.args since requests can invoke others by passing args directly to their initializers.
@gholms Sorry for the delay on this PR. I'm continuing @minnus work and would be happy to make any additional fixes necessary to get this accepted. We have several tools for accounting and monitoring our euca cloud that makes use of the pretty=json format. Would be great to see this incorporated into the upstream euca2ools. I attempted to tidy things up according to your previous comments above. Let us know how things look. Thanks! |
@gholms any update on what we would need for this. I would also find this useful |
@gholms any updates on this? I have been useing the patch to update my local copy and it seems to work, would be awesome if this was standard. |
Outputs json format instead of the standard text formatting
for these tools.
Addresses some of : TOOLS-626
Not sure if the arg parsing should go in init but it was the easiest way to get this going instead of adding it to each subcommand.