[v2] Add ability to set properties in subsections#10109
[v2] Add ability to set properties in subsections#10109kdaily wants to merge 5 commits intofeature-configure-subsectionsfrom
Conversation
4aa4c78 to
7eddac8
Compare
7eddac8 to
dc2611d
Compare
This change adds new parameters to the `aws configure set`` command to specify a sub-section for setting a property. These parameters are analogous to the existing `--profile` parameter. A parameter will be added for each sub-section type and take a value of the subsection name. Following is the generic pattern for the `aws configure set` command: ``` aws configure set --<sub-section-type> <sub-section-name> \ <property> <value> ``` For example, the following command should set the property `sso_region` to the value `us-west-2` in the `sso-session` sub-section named `my-sso-session`: ``` aws configure set --sso-session my-sso-session \ sso_region us-west-2 ``` Following is an example setting a nested property in a sub-section: ``` aws configure set \ --<sub-section-type> <sub-section-name> \ <nested-section>.<property> value ``` The only sub-section types allowed are `services` and `sso-session`.
dc2611d to
3360369
Compare
ashovlin
left a comment
There was a problem hiding this comment.
Can we also add a new example or two?
| }, | ||
| { | ||
| 'name': 'sso-session', | ||
| 'help_text': 'The name of the SSO session sub-section to configure.', |
There was a problem hiding this comment.
This reads odd via aws configure set help. We might need to make this help_text more generic if there isn't a way to separate the two new options better?
OPTIONS
varname (string) The name of the config value to set.
value (string) The value to set.
--sso-session | --services (string) The name of the SSO session
sub-section to configure.
There was a problem hiding this comment.
Ah, I see the issue. The intention was to highlight the mutual exclusivity of the parameter. I think updating the wording is probably the best resolution, but I could split them as well and then rely on the implementation erorring if more than one is provided:
OPTIONS
varname (string) The name of the config value to set.
value (string) The value to set.
--services (string) The name of the services
sub-section to configure.
--sso-session (string) The name of the SSO session
sub-section to configure.
There was a problem hiding this comment.
Fixed in 1271886. I kept them in the same parameter group and updated the wording.
| if hasattr(args, section_properties['param_name']): | ||
| param_value = getattr(args, section_properties['param_name']) | ||
| if param_value is not None: | ||
| if not re.match(r"[\w\d_\-/.%@:\+]+", param_value): |
There was a problem hiding this comment.
- This is different than the regex in the internal spec
[A-Za-z0-9_\-/.%@:\+]+- is accepting unicode via\wokay? - It doesn't look like
aws configure ssovalidates this, justRequiredInputValidator. Is there a chance someone has already configured something invalid? Though since this is brand new workflow, I think it's okay to start by matching the spec, and relaxing if needed. - Can you add a comment explaining what we're validating here? By the time I got to the regex, I wasn't sure what
param_valuewas anymore.
There was a problem hiding this comment.
This is different than the regex in the internal spec [A-Za-z0-9_-/.%@:+]+ - is accepting unicode via \w okay?
We currently accept profile and SSO session names with unicode.
Can you add a comment explaining what we're validating here? By the time I got to the regex, I wasn't sure what param_value was anymore.
Yep, will update.
| if '.' in varname: | ||
| parts = varname.split('.') | ||
| if len(parts) > 2: | ||
| return 0 |
There was a problem hiding this comment.
Could we add a comment explaining this validation?
And if it's what I assume, that we don't have any valid properties deeper than --services <name> s3.endpoint_url, should we raise a validation error rather than silently not writing?
There was a problem hiding this comment.
Agree we could raise validation error. The current implementation silently sets the parameter at the first level. For example:
aws --profile brandbrandnew configure set foo.bar.baz "qux"
Results in:
[profile brandbrandnew]
foo = qux
Remove the mapping between the subsection type and the parameter name. This is a transformation between '-' separated and '_' separated, so use a utility method to do this. Rename parameters to improve understanding of the code.
Only one level of nested properties is supported, e.g. 'aaa.bbb'. If the user supplies more than that (.aaa.bbb.ccc') an error is raised.
2f42777 to
a1d5d81
Compare
a1d5d81 to
1271886
Compare
Added in be92fb1 |
This change adds new parameters to the
aws configure setcommand to specify a sub-section for setting a property. These parameters are analogous to the existing--profileparameter. A parameter will be added for each sub-section type and take a value of the subsection name. Following is the generic pattern for theaws configure setcommand:For example, the following command should set the property
sso_regionto the valueus-west-2in thesso-sessionsub-section namedmy-sso-session:Following is an example setting a nested property in a sub-section:
The only sub-section types allowed are
servicesandsso-session.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.