Add config file support for config resolution#729
Conversation
SamRemis
left a comment
SamRemis
left a comment
There was a problem hiding this comment.
My bad, clicked approve by mistake. Only the test comments are blocking
cb70998 to
27cbdd5
Compare
jonathan343
left a comment
jonathan343
left a comment
There was a problem hiding this comment.
Thanks Ujjwal. Commenting on a few edge cases that seem like bugs to me. Still getting through others, but this is what I have so far.
| sub_properties={k: dict(v) for k, v in profile.sub_properties.items()}, | ||
| ) | ||
|
|
||
| for name, profile in credentials_profiles.items(): |
There was a problem hiding this comment.
Could we represent each section with a single standardized mapping? Storing scalar and grouped values separately allows the same property to hold conflicting values after merging.
For example:
# ~/.aws/config
[profile p]
x = config# ~/.aws/credentials
[p]
x =
nested = credentialsRunning this through the current implementation produces:
Section(
properties={"x": "config"},
sub_properties={"x": {"nested": "credentials"}},
)The credentials value should replace the config value entirely. Botocore’s final merged profile is:
{'x': {'nested': 'credentials'}}
Credentials should replace the complete x property, so get("p", "x") should return None. Botocore produces {"x": {"nested": "credentials"}} for this case. A single mapping such as dict[str, str | dict[str, str]] would prevent this conflicting state and make whole-property precedence straightforward.
| if stripped.startswith("profile"): | ||
| return | ||
| if stripped.startswith("sso-session"): | ||
| return | ||
| if stripped.startswith("services"): | ||
| return |
There was a problem hiding this comment.
These checks incorrectly discard valid credentials profiles such as:
[services-prod]
aws_access_key_id = keyCould we remove the three startswith() checks and rely directly on _is_valid_identifier() before merging the profile? This is simpler, matches botocore, and avoids dropping valid names that merely begin with a reserved term.
| """Config related exceptions""" | ||
|
|
||
|
|
||
| class ConfigParseError(Exception): |
There was a problem hiding this comment.
This is a general error comment, not specific to this file
Parse errors should identify which source file failed. load_config() parses both the config and credentials files, but the current exception only includes the line number.
For example, using an unclosed section header in either file:
[profile p
produces the same error in both cases:
ConfigParseError: Line 1: Section definition must end with ']'
Botocore distinguishes the files:
ConfigParseError: Unable to parse config file: /.../config
ConfigParseError: Unable to parse config file: /.../credentials
Could parse_config_file() add file_path when re-raising errors from _parse_content()? This would preserve the current line-specific detail while identifying the file:
Unable to parse config file '/.../credentials':
Line 1: Section definition must end with ']'
| return inner | ||
|
|
||
|
|
||
| def _parse_property_line(line: str, line_num: int) -> tuple[str | None, str]: |
There was a problem hiding this comment.
Ignoring an invalid property currently allows its continuation lines to modify the previous valid property.
For example:
[profile p]
region = us-east-1
invalid key = ignored
continuationproduces:
Section(
properties={"region": "us-east-1\ncontinuation"},
sub_properties={},
)The expected result is:
Section(
properties={"region": "us-east-1"},
sub_properties={},
)
Botocore does not filter the invalid property name, but it correctly associates continuation with invalid key rather than region.
Could we track when an invalid property has been ignored and also discard its continuation lines? This preserves our spec's silent-ignore behavior without corrupting the previous property.
3 participants
Issue #, if available:
Description of changes:
Add support for parsing and merging AWS config and credentials files (
~/.aws/configand~/.aws/credentials) as part of the config resolution pipeline.Changes:
profiles/sso-sessions/services
ParsedConfigFileclass for querying the merged resultconfig-file-parser-tests.jsonandconfig-file-location-tests.json)+ unit tests for the query layer and file I/O edge casesTesting:
~/.aws/configfile to verify end-to-end parsing behaviorBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.