Add output getter and query prompter #5724
Conversation
Codecov Report
@@ Coverage Diff @@
## prompt-output-panel #5724 +/- ##
=======================================================
- Coverage 93.48% 93.45% -0.04%
=======================================================
Files 254 255 +1
Lines 20206 20340 +134
=======================================================
+ Hits 18889 19008 +119
- Misses 1317 1332 +15
Continue to review full report at Codecov.
|
awscli/autoprompt/output.py
Outdated
| else: | ||
| query = parsed.global_params.get('query') | ||
| if query: | ||
| query = re.sub(r'([\{\[])\d+?([\}\]])', '\g<1>\g<2>', query) |
There was a problem hiding this comment.
I'm still on the fence on whether we should be ignoring any digits in the query. I recently ran into an issue where, I inputted:
aws codebuild batch-get-projects --query projects[0].source.buildspec
And the sample output, showed up as:
[
"buildspec"
]
which was wrong because it really ends up not being enclosed by list brackets e.g.:
"buildspec"
I'm mainly concerned that if we do not use the literal expression that was provided, there will be other instances like this where the output structure/format displayed in the panel is just wrong compared to the actual output structure.
There was a problem hiding this comment.
What about to change any number in brackets to 0
awscli/autoprompt/output.py
Outdated
| parsed.lineage, parsed.current_command, operation) | ||
| if operation_model: | ||
| output_shape = operation_model.output_shape | ||
| if output_shape: |
There was a problem hiding this comment.
We should also probably check if the shape has no members. I ran the aws codebuild delete-project command and the output panel was blank when I was expecting it to show No output.
| args = argparse.Namespace(query=query, color='off') | ||
| argument_generator = ArgumentGenerator(use_member_names=True) | ||
| response = argument_generator.generate_skeleton(output_shape) | ||
| formatter = get_formatter(output, args) |
There was a problem hiding this comment.
As a reminder, it would be great to make it so that yaml does not use !!omap in its representation for dictionary.
There was a problem hiding this comment.
I remember. I still can't understand why they appear in this output but don't appear in the normal one.
There was a problem hiding this comment.
We may have to update the ArgumentGenerator in botocore. I know it uses ordered dictionaries when really do not have to do that anymore because dictionaries are now ordered for the minimum python version that we support.
awscli/autocomplete/local/fetcher.py
Outdated
| service_name = self.SERVICE_MAPPER.get(lineage[1], lineage[1]) | ||
| service_model = self._cli_driver.session.get_service_model( | ||
| service_name) | ||
| operation = ''.join([part.capitalize() | ||
| for part in current_command.split('-')]) | ||
| return service_model.operation_model(operation) |
There was a problem hiding this comment.
There are other services that are renamed and therefore do not match what the client name is in botocore. I wonder instead if we can just grab the service_model or operation_model attached to the command object to make this handle renames more generically?
awscli/autoprompt/output.py
Outdated
| try: | ||
| self._current_expression = jmespath.compile(query) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Instead of swallowing the exception. I wonder if it makes sense to actually display the parsing error in the output panel? Mainly there is a couple of times where I provided an invalid jmespath expression in auto-prompt mode but I did not know until I actually ran the command. It would have been nice if there was some output to tell me that I formatted it wrong.
vz10
force-pushed
the
prompt-output-panel
branch
2 times, most recently
from
27f59d6
to
f28a1a9
Compare
| def get_output(self, parsed): | ||
| operation_model = self._cli_driver_fetcher.get_operation_model( | ||
| parsed.lineage, parsed.current_command) | ||
| if operation_model: |
There was a problem hiding this comment.
Not sure where this is happening now but for some command, it shows up as No output but after I add a space after the command the output shows up. Once command was: aws deploy get-deployment If I do not include a space, it suggests No output, but if I add a space aws deploy get-deployment then it shows the normal output.
There was a problem hiding this comment.
Following up here... It looks like if the command is a substring of another command (e.g. get-deployment is a substring of get-deployment-config) that is when it shows up of having No output when toggling to the command that is a substring in the operation list.
It looks like this is an issue with the doc panel as well. Is this something easy to fix?
awscli/autoprompt/output.py
Outdated
| try: | ||
| self._current_expression = jmespath.compile(query) | ||
| except jmespath.exceptions.ParseError as e: | ||
| error_message = e.msg |
There was a problem hiding this comment.
I like where this is going. It would be nice if this error was a bit more formatted instead of just the literal error message. Maybe something like:
ERROR: Unable to display output.
Bad value for --query: <query expression>
<Add helper string for where the error is>
<Insert error message here>
In the end, I'd imagine it would behave similarly to what the CLI does with its error messages when it encounters invalid JMESPath expressions. So for example:
aws ec2 describe-regions --query Regions[0]..Endpoint
Bad value for --query Regions[0]..Endpoint: Expecting: ['quoted_identifier', 'unquoted_identifier', 'lbracket', 'lbrace'], got: dot: Parse error at column 11, token "." (DOT), for expression:
"Regions[0]..Endpoint"
^
f28a1a9
to
5c59f33
Compare
vz10
force-pushed
the
prompt-output-panel
branch
2 times, most recently
from
1d67d8c
to
573237c
Compare
vz10
force-pushed
the
prompt-output-panel
branch
from
a173bb1
to
59f931b
Compare
vz10
force-pushed
the
prompt-output-panel
branch
2 times, most recently
from
b2b4dc9
to
ea6a482
Compare
There was a problem hiding this comment.
Looks good. I just had a bit more suggestions/questions, but I think we are getting close. I think one thing I notice was there were no tests for autoprompt/output.py. Was that intentional? I think there is a fair amount of behavior in that module we would want to have tests for.
| operation_model) | ||
|
|
||
| def _get_query_and_last_key(self, query): | ||
| query = re.sub(r'([\{\[])\d+?([\}\]])', '\g<1>0\g<2>', query) |
There was a problem hiding this comment.
A comment here explaining what the query does would be nice.
| def __init__(self, cli_driver_fetcher=None, | ||
| response_filter=startswith_filter): | ||
| self._filter = response_filter | ||
| self._cli_driver_fetcher = cli_driver_fetcher |
There was a problem hiding this comment.
One thing that I ran into was that I tried this with the tab completer (e.g. `aws_completer). Specifically, I tried:
aws ec2 describe-regions --query <TAB>
And it printed out the following Traceback:
Traceback (most recent call last):
File "/Users/kyleknap/.pyenv/versions/pyinstaller-3.7.2/bin/aws_completer", line 7, in <module>
exec(compile(f.read(), __file__, 'exec'))
File "/Users/kyleknap/GitHub/aws-cli/bin/aws_completer", line 36, in <module>
main()
File "/Users/kyleknap/GitHub/aws-cli/bin/aws_completer", line 28, in main
autocomplete(command_line, command_index)
File "/Users/kyleknap/GitHub/aws-cli/awscli/autocomplete/main.py", line 55, in autocomplete
results = completer.autocomplete(command_line, position)
File "/Users/kyleknap/GitHub/aws-cli/awscli/autocomplete/completer.py", line 44, in autocomplete
result = completer.complete(parsed)
File "/Users/kyleknap/GitHub/aws-cli/awscli/autocomplete/local/basic.py", line 527, in complete
operation_model = self._cli_driver_fetcher.get_operation_model(
AttributeError: 'NoneType' object has no attribute 'get_operation_model'
I think at a minimum it should be not allow an error to propagate like this.
awscli/autocomplete/local/basic.py
Outdated
| parsed_response = parsed_response[0] | ||
| if isinstance(parsed_response, dict): | ||
| results = parsed_response.keys() | ||
| except Exception: |
There was a problem hiding this comment.
What is the exception we are expecting to catch here? We should probably be specific here so we do not accidentally swallow any unintended issues.
There was a problem hiding this comment.
Actually I don't expect any other exceptions here added it just in case.
There was a problem hiding this comment.
Does it makes sense to log it then? I just want to have just some mechanism to figure out what is going wrong if we do a catch an unexpected exception.
| def get_output(self, parsed): | ||
| operation_model = self._cli_driver_fetcher.get_operation_model( | ||
| parsed.lineage, parsed.current_command) | ||
| if operation_model: |
There was a problem hiding this comment.
Following up here... It looks like if the command is a substring of another command (e.g. get-deployment is a substring of get-deployment-config) that is when it shows up of having No output when toggling to the command that is a substring in the operation list.
It looks like this is an issue with the doc panel as well. Is this something easy to fix?
awscli/autoprompt/output.py
Outdated
| query = re.sub(r'([\{\[])\d+?([\}\]])', '\g<1>0\g<2>', query) | ||
| query = re.sub(r'([\{\[])[^\]^\}]*$', '', query).strip('.') |
There was a problem hiding this comment.
Comments on what these regex's do would be nice.
awscli/autoprompt/output.py
Outdated
| try: | ||
| self._current_expression = jmespath.compile(query) | ||
| except jmespath.exceptions.ParseError as e: | ||
| error_message = "Bad query %s: %s" % (query, str(e)) |
There was a problem hiding this comment.
For the error message, Let's make it instead:
error_message = "Bad value for --query: %s\n\n%s" % (query, str(e))I think the main points is that we should use -- to indicate it is the --query parameter and it will add newlines to the error to help make it a little more readable.
awscli/autoprompt/output.py
Outdated
| @@ -0,0 +1,88 @@ | |||
| # Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
The year should be 2020 here.
vz10
force-pushed
the
prompt-output-panel
branch
5 times, most recently
from
6a55e13
to
1bb052a
Compare
awscli/autocomplete/local/basic.py
Outdated
| parsed_response = parsed_response[0] | ||
| if isinstance(parsed_response, dict): | ||
| results = parsed_response.keys() | ||
| except Exception: |
There was a problem hiding this comment.
Does it makes sense to log it then? I just want to have just some mechanism to figure out what is going wrong if we do a catch an unexpected exception.
| } | ||
| }) | ||
| self.parser = parser.CLIParser(index) | ||
| self.driver = create_clidriver() |
There was a problem hiding this comment.
We should make sure that for these tests we are using a stubbed environment. It is possible to set in your ~/.aws/config file the output format. So we do not want to unintentionally be using that when some of these tests assume that the fallback is configured to json.
awscli/autoprompt/output.py
Outdated
| if output not in self._output_formats: | ||
| output = self._session.get_config_variable('output') |
There was a problem hiding this comment.
One subtle thing that I missed in previous reviews that I don't think we discussed. Do we want to be reverting back to the configured variable for invalid output formats, or should we print out an error message saying that the output format is invalid (similar to what we do with the --query expression)? The main benefit of what we have now is that while someone is typing the value to --output won't show an error message, but I wonder if that is a strong enough of a reason to warrant this behavior? Like if I had a typo in the output value, I would prefer that the output panel tell me about it before I try to execute the command. Thoughts?
awscli/autocomplete/parser.py
Outdated
| @@ -126,8 +126,9 @@ class CLIParser(object): | |||
| not a general purpose AWS CLI parser. | |||
|
|
|||
| """ | |||
| def __init__(self, index): | |||
| def __init__(self, index, first_wins=False): | |||
There was a problem hiding this comment.
I feel like we can make a better name here. Mainly it is a little ambiguous about what is first and how does it win. How about return_first_command_match? It's a little lengthy but I think it represents what we are going for. I also think it might be worth adding docstrings to describe the parameter because it is a pretty specific parameter.
vz10
force-pushed
the
prompt-output-panel
branch
from
1bb052a
to
d37bee2
Compare
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.