-
Notifications
You must be signed in to change notification settings - Fork 31
Add config file support for config resolution #729
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import os | ||
|
|
||
| from smithy_aws_core.config.file_parser import ( | ||
| FileType, | ||
| parse_config_file, | ||
| standardize, | ||
| ) | ||
| from smithy_aws_core.config.parsed_config_file import ParsedConfigFile | ||
|
|
||
| _DEFAULT_CONFIG_FILE = "~/.aws/config" | ||
| _DEFAULT_CREDENTIALS_FILE = "~/.aws/credentials" | ||
| _CONFIG_FILE_ENV_VAR = "AWS_CONFIG_FILE" | ||
| _CREDENTIALS_FILE_ENV_VAR = "AWS_SHARED_CREDENTIALS_FILE" | ||
|
|
||
|
|
||
| def resolve_config_paths( | ||
| config_file_path: str | None = None, | ||
| credentials_file_path: str | None = None, | ||
| ) -> tuple[str, str]: | ||
| """Resolve the final config and credentials file paths. | ||
|
|
||
| Resolution order for each path: | ||
| 1. Explicit argument (if provided) | ||
| 2. Environment variable (AWS_CONFIG_FILE / AWS_SHARED_CREDENTIALS_FILE) | ||
| 3. Default (~/.aws/config / ~/.aws/credentials) | ||
|
|
||
| The ~ is expanded to the user's home directory. | ||
|
|
||
| :param config_file_path: Override path for config file. | ||
| :param credentials_file_path: Override path for credentials file. | ||
| :returns: Tuple of (resolved_config_path, resolved_credentials_path). | ||
| """ | ||
| config_path = ( | ||
| config_file_path or os.environ.get(_CONFIG_FILE_ENV_VAR) or _DEFAULT_CONFIG_FILE | ||
| ) | ||
| credentials_path = ( | ||
| credentials_file_path | ||
| or os.environ.get(_CREDENTIALS_FILE_ENV_VAR) | ||
| or _DEFAULT_CREDENTIALS_FILE | ||
| ) | ||
|
|
||
| return ( | ||
| os.path.expanduser(os.path.expandvars(config_path)), | ||
| os.path.expanduser(os.path.expandvars(credentials_path)), | ||
| ) | ||
|
|
||
|
|
||
| async def load_config( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 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? |
||
| config_file_path: str | None = None, | ||
| credentials_file_path: str | None = None, | ||
| ) -> ParsedConfigFile: | ||
| """Load and merge AWS config and credentials files. | ||
|
|
||
| Parses both files, standardizes them, and returns a merged | ||
| ParsedConfigFile ready for querying. | ||
|
|
||
| :param config_file_path: Override path for config file. | ||
| Defaults to AWS_CONFIG_FILE env var or ~/.aws/config. | ||
| :param credentials_file_path: Override path for credentials file. | ||
| Defaults to AWS_SHARED_CREDENTIALS_FILE env var or ~/.aws/credentials. | ||
| :returns: A ParsedConfigFile with merged profiles from both files. | ||
| """ | ||
| config_path, credentials_path = resolve_config_paths( | ||
| config_file_path, credentials_file_path | ||
| ) | ||
|
|
||
| raw_config = await parse_config_file(config_path) | ||
| raw_credentials = await parse_config_file(credentials_path) | ||
|
|
||
| std_config = standardize(raw_config, FileType.CONFIG) | ||
| std_credentials = standardize(raw_credentials, FileType.CREDENTIALS) | ||
|
|
||
| return ParsedConfigFile(std_config, std_credentials) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Config related exceptions""" | ||
|
|
||
|
|
||
| class ConfigParseError(Exception): | ||
| """Raised when a config file cannot be parsed due to invalid syntax.""" |
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.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.