Add config file support for config resolution#729
Conversation
| ) | ||
|
|
||
| return ( | ||
| os.path.expanduser(os.path.expandvars(config_path)), |
There was a problem hiding this comment.
[Trying to reason about this, genuine question]
This method strikes me as a little odd. If someone passes both the credentials and config path, we don't take them literally and just expand the user for them.
I don't know if this needs to be a public method. We could have all of this logic inline in load_config, or make it private, no?
There was a problem hiding this comment.
[change requested]
To keep the public facing interface smaller, could we move this logic inline to load_config?
Also, for now can we get rid of the expandvars? We shouldn't expand environment vars if other SDKs don't, and I don't see it in the specification.
| ) | ||
|
|
||
| return ( | ||
| os.path.expanduser(os.path.expandvars(config_path)), |
There was a problem hiding this comment.
[change requested]
To keep the public facing interface smaller, could we move this logic inline to load_config?
Also, for now can we get rid of the expandvars? We shouldn't expand environment vars if other SDKs don't, and I don't see it in the specification.
| """Read file content asynchronously. | ||
|
|
||
| Returns None if the file doesn't exist or can't be opened. | ||
| Per the SEP: inaccessible files are treated as empty. |
There was a problem hiding this comment.
[Change requested]
There's a couple places we reference "SEP", which we shouldn't do in public code. Can you reword this?
| ) | ||
|
|
||
|
|
||
| async def load_config( |
There was a problem hiding this comment.
[discussion]
While I like this as an entry point, there's a few things I'd want to consider changing, but you're more into this than me so maybe I'm missing something.
First, the ParsedConfigFile is a strange name for an object that holds two parsed files (config + credentials). Maybe we should rename to MergedConfig?
Second (and this one is more spitballing), how would you feel about accepting a path object instead of a string so we don't need to normalize?
| def parse_content(content: str) -> RawParsedSections: | ||
| """Parse config file content from a string into raw sections. | ||
|
|
||
| Note: This function is public only for direct use by unit tests |
There was a problem hiding this comment.
[Still thinking about how to fix this one, but commenting cause I think it's right]
If this should be internal, we should make it an internal method. If that makes it harder to test, that's a sign that we should reconsider our testing strategy. This probably looks like writing to temp files as the config files for testing.
| raise ConfigParseError(f"Line {line_num}: Section definition must end with ']'") | ||
| inner = stripped[1:bracket_end].strip() | ||
| if not inner: | ||
| raise ConfigParseError(f"Line {line_num}: Section name cannot be empty") |
There was a problem hiding this comment.
[Discussion for posterity, no change requested]
I feel weird commenting this because I like what you did here better than the specification, but the spec does say that this is wrong for sso-session profiles:
If the session name is empty, the section and all following properties must be ignored.
Service profiles have a similar line:
If the services configuration name is empty, the section and all following properties MUST be ignored.
I'm not asking you to change this for two reasons:
- If we ever want to conform with the specification, we can always come back and update this code
- It's a better customer experience to let them know we are ignoring their profile's content.
Maybe there's something I'm missing as to why that was part of the spec, but for now, let's leave it as is unless you (or anyone else) disagrees. Also, depending on how you read the word "ignored", we could consider raising an error ignoring this since those values would never be used...
| from smithy_aws_core.config.exceptions import ConfigParseError | ||
|
|
||
| # Type aliases | ||
| type PropertyValue = str | dict[str, str] |
There was a problem hiding this comment.
[Discussion]
Instead of having ProfileProperties, a ProfileMap could have a Profile; each Profile could have ScalarProperties and GroupedProperties. The union feels a bit awkward to have to consider while coding around the StandardizedOutput object.
What do you think?
| try: | ||
| content = await asyncio.to_thread(Path(path).read_text, encoding="utf-8") | ||
| return content | ||
| except (FileNotFoundError, PermissionError, OSError, UnicodeDecodeError): |
There was a problem hiding this comment.
[Suggestion]
For usability, maybe we should add a warning on PermissionError and OSError so the user knows that we couldn't read from the requested file. Also, a UnicodeDecodeError should raise and not silently fail.
Thoughts?
2 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.