-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 support for per sub-command aliases #7399
Conversation
Codecov Report
@@ Coverage Diff @@
## v2 #7399 +/- ##
==========================================
+ Coverage 93.71% 93.74% +0.02%
==========================================
Files 351 352 +1
Lines 36175 36670 +495
Branches 5202 5274 +72
==========================================
+ Hits 33903 34375 +472
- Misses 1648 1663 +15
- Partials 624 632 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
The [CLI alias feature](https://docs.aws.amazon.com/cli/latest/userguide/cli-usage-alias.html) currently supports creating aliases as top level commands, at the same level that service commands currently reside. For example, given this alias file: ``` [toplevel] iam-roles = iam list-roles ``` you can run `aws iam-roles`, which will be equivalent to `aws iam list-roles`. This helps reduce the amount of typing for commonly used commands. However, this requires all aliases be in the to level namespace, which has hundreds of services. This makes it hard to remember and look up what aliases you have available. This PR adds support for creating aliases within namespaces of existing commands/subcommands. There is some implementation specific details with respect to event names that we needed to rely onin order to make aliases of nested commands work (i.e. `aws foo bar <myalias>`). The implementation is different from the approach taken by the `[toplevel]` aliases for several reasons: * The toplevel aliases make the assumption that a single parser can be used for any of the alias values. This is essentially the "global args" parser, so this holds true for top level aliase but isn't guaranteed for subcommands, custom commands, etc. * We need to ensure we scope the alias expansion to the current namespace we're traversing so we need an approach that enforces this. * There may be additional logic that happens when invoking a command, even within a given namespace. The safer option is to re-execute all this logic after expanding the alias. So the high level idea is to take the reference to the parent command object, and reinvoke it with the expanded alias value, after picking off, and updating, and global parameters provided in the alias value. This approach also has the benefit of not modifying the existing code (except for extract pieces out into a shared base class) so it's conservative in terms of the likelihood of breaking existing aliases. Closes aws#7386
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.
The approach looks good so far. I wrote a bunch of aliases in testing it out and did not run into any unexpected behavior. Just had some small comments until we get to addressing merging command line provided arguments to leaf custom commands.
This enables you to use the bag of options aliases and still add additional params on the command line when you run the command.
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.
Looks fine. Just had a couple more comments and questions
copied.required = False | ||
arg_table_copy[key] = copied | ||
return arg_table_copy | ||
|
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 looks like there is two additional new lines here.
maybe_parsed_subcommand = self._parse_potential_subcommand( | ||
args, self._subcommand_table | ||
) | ||
if maybe_parsed_subcommand is not None: |
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.
With this if statement now added, does this line ever get used? https://github.com/aws/aws-cli/pull/7399/files#diff-ae84646454f40103f091a890d7a1f826eaebc333454d1a5772ce31c721881cd6R211 It seems like it would not (e.g. in the s3
subcommand).
This is already handled with the SubCommandArgParser so it's no longer needed. The BasicCommand could also have the last elif clause removed, but it's there as an extra safety net as we'd by relying on the SubCommandArgParser to verify that the subcommand table is not None instead of explicitly checking this in BasicCommand directly.
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.
Looks good 🚢
The CLI alias feature
currently supports creating aliases as top level commands, at the
same level that service commands currently reside. For example,
given this alias file:
you can run
aws iam-roles
, which will be equivalent toaws iam list-roles
. This helps reduce the amount of typing forcommonly used commands.
However, this requires all aliases be in the to level namespace,
which has hundreds of services. This makes it hard to remember
and look up what aliases you have available.
This PR adds support for creating aliases within namespaces of
existing commands/subcommands.
There is some implementation specific details with respect to
event names that we needed to rely onin order to make aliases of nested
commands work (i.e.
aws foo bar <myalias>
).The implementation is different from the approach taken by
the
[toplevel]
aliases for several reasons:used for any of the alias values. This is essentially the "global
args" parser, so this holds true for top level aliase but isn't
guaranteed for subcommands, custom commands, etc.
namespace we're traversing so we need an approach that enforces this.
even within a given namespace. The safer option is to re-execute
all this logic after expanding the alias.
So the high level idea is to take the reference to the parent command
object, and reinvoke it with the expanded alias value, after
picking off, and updating, and global parameters provided in the
alias value.
This approach also has the benefit of not modifying the existing
code (except for extract pieces out into a shared base class) so
it's conservative in terms of the likelihood of
breaking existing aliases.
Closes #7386