-
Notifications
You must be signed in to change notification settings - Fork 26
Add config source interface and environment variable source for config resolution #640
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: config_resolution_main
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,7 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| from .sources import EnvironmentSource | ||
|
|
||
| __all__ = [ | ||
| "EnvironmentSource", | ||
| ] |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,35 @@ | ||||||||||||||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||||||||||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||||||||||
| import os | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class EnvironmentSource: | ||||||||||||||
| """Configuration from environment variables.""" | ||||||||||||||
|
|
||||||||||||||
| def __init__(self, prefix: str = "AWS_"): | ||||||||||||||
| """Initialize the EnvironmentSource with environment variable prefix. | ||||||||||||||
|
|
||||||||||||||
| :param prefix: Prefix for environment variables (default: 'AWS_') | ||||||||||||||
| """ | ||||||||||||||
| self._prefix = prefix | ||||||||||||||
|
|
||||||||||||||
| @property | ||||||||||||||
| def name(self) -> str: | ||||||||||||||
|
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. nit - It feels weird to have a
If you decide the keep the constant, it should be private and use from typing import Final
class EnvironmentSource:
_NAME: Final[str] = "environment"
@property
def name(self) -> str:
return self._NAME
Author
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. I will use the |
||||||||||||||
| """Returns the source name.""" | ||||||||||||||
| return "environment" | ||||||||||||||
|
|
||||||||||||||
| def get(self, key: str) -> str | None: | ||||||||||||||
| """Returns a configuration value from environment variables. | ||||||||||||||
|
|
||||||||||||||
| The key is transformed to uppercase and prefixed (e.g., 'region' → 'AWS_REGION'). | ||||||||||||||
|
|
||||||||||||||
| :param key: The standard configuration key (e.g., 'region', 'retry_mode'). | ||||||||||||||
|
|
||||||||||||||
| :returns: The value from the environment variable (or empty string if set to empty), | ||||||||||||||
| or None if the variable is not set. | ||||||||||||||
| """ | ||||||||||||||
| env_var = f"{self._prefix}{key.upper()}" | ||||||||||||||
| config_value = os.environ.get(env_var) | ||||||||||||||
| if config_value is None: | ||||||||||||||
| return None | ||||||||||||||
| return config_value.strip() | ||||||||||||||
|
Comment on lines
+32
to
+35
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. Alternate option, commit this if you think it's more readable. Functionally, these are equivalent, just does the same thing in two fewer lines
Suggested change
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. I think we should call For example, if a customer provides If we strip the config value prior to checking emptiness, it would correctly return None.
Author
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. We want to use whatever value is set for that environment variable. If a value is set to an empty string, it'll return an empty string. The docstring will be updated. |
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| import os | ||
| from unittest.mock import patch | ||
|
|
||
| from smithy_aws_core.config.sources import EnvironmentSource | ||
|
|
||
|
|
||
| class TestEnvironmentSource: | ||
| def test_source_name(self): | ||
| source = EnvironmentSource() | ||
| assert source.name == "environment" | ||
|
|
||
| def test_get_region_from_aws_region(self): | ||
| with patch.dict(os.environ, {"AWS_REGION": "us-west-2"}, clear=True): | ||
| source = EnvironmentSource() | ||
| value = source.get("region") | ||
| assert value == "us-west-2" | ||
|
|
||
| def test_get_returns_none_when_env_var_not_set(self): | ||
| with patch.dict(os.environ, {}, clear=True): | ||
| source = EnvironmentSource() | ||
| value = source.get("region") | ||
| assert value is None | ||
|
|
||
| def test_get_returns_none_for_unknown_key(self): | ||
| source = EnvironmentSource() | ||
| value = source.get("unknown_config_key") | ||
| assert value is None | ||
|
|
||
| def test_get_handles_empty_string_env_var(self): | ||
| with patch.dict(os.environ, {"AWS_REGION": ""}, clear=True): | ||
| source = EnvironmentSource() | ||
| value = source.get("region") | ||
| assert value == "" | ||
|
|
||
| def test_get_handles_whitespace_env_var(self): | ||
| with patch.dict(os.environ, {"AWS_REGION": " us-west-2 "}, clear=True): | ||
| source = EnvironmentSource() | ||
| value = source.get("region") | ||
| # Whitespaces should be stripped | ||
| assert value == "us-west-2" | ||
|
|
||
| def test_get_handles_whole_whitespace_env_var(self): | ||
| with patch.dict(os.environ, {"AWS_REGION": " "}, clear=True): | ||
| source = EnvironmentSource() | ||
| value = source.get("region") | ||
| # Whitespaces should be stripped | ||
| assert value == "" | ||
|
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. Shouldn't this case return None? This allows the region config value will resolve to an empty string which seems incorrect.
Author
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. It will return an empty string. We want to use whatever value is set for that environment variable. |
||
|
|
||
| def test_multiple_keys_with_different_env_vars(self): | ||
| env_vars = {"AWS_REGION": "eu-west-1", "AWS_RETRY_MODE": "standard"} | ||
| with patch.dict(os.environ, env_vars, clear=True): | ||
| source = EnvironmentSource() | ||
|
|
||
| region = source.get("region") | ||
| retry_mode = source.get("retry_mode") | ||
|
|
||
| assert region == "eu-west-1" | ||
| assert retry_mode == "standard" | ||
|
|
||
| def test_get_is_idempotent(self): | ||
| with patch.dict(os.environ, {"AWS_REGION": "ap-south-1"}, clear=True): | ||
| source = EnvironmentSource() | ||
| # Calling get on source multiple times should return the same value | ||
| value1 = source.get("region") | ||
| value2 = source.get("region") | ||
| value3 = source.get("region") | ||
|
|
||
| assert value1 == value2 == value3 == "ap-south-1" | ||
|
|
||
| def test_source_does_not_cache_env_vars(self): | ||
| source = EnvironmentSource() | ||
|
|
||
| # First read | ||
| with patch.dict(os.environ, {"AWS_REGION": "us-east-1"}, clear=True): | ||
| value1 = source.get("region") | ||
| assert value1 == "us-east-1" | ||
|
|
||
| # Environment changes | ||
| with patch.dict(os.environ, {"AWS_REGION": "us-west-2"}, clear=False): | ||
| value2 = source.get("region") | ||
| assert value2 == "us-west-2" | ||
|
|
||
| # Source reads from os.environ and not from cache | ||
| assert value1 != value2 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
|
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. nit - The |
||
| # SPDX-License-Identifier: Apache-2.0 | ||
| from typing import Any, Protocol, runtime_checkable | ||
|
|
||
|
|
||
| @runtime_checkable | ||
| class ConfigSource(Protocol): | ||
| """Protocol for configuration sources that provide values from various locations | ||
| like environment variables and configuration files. | ||
| """ | ||
|
|
||
| @property | ||
| def name(self) -> str: | ||
| """Returns a string identifying the source. | ||
|
|
||
| :returns: A string identifier for this source. | ||
| """ | ||
| ... | ||
|
|
||
| def get(self, key: str) -> Any | None: | ||
|
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. Looks like this method definition isn't future proofing us for when we'll need to make async calls to read the AWS config file
Author
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. This is intentionally sync. For the config file source, the plan is to read and cache the snapshot of the file during the first access and it’s just an in-memory dict lookup on all subsequent requests. Making this async would require maintaining parallel async/sync Config implementations and forceing users to await simple property access, which significantly complicates both the codebase and the customer interface. |
||
| """Returns a configuration value from the source. | ||
|
|
||
| :param key: The configuration key to retrieve (e.g., 'region') | ||
|
|
||
| :returns: The value associated with the key, or None if not found. | ||
| """ | ||
| ... | ||
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.
This assumes that all env vars will be prefixed with
AWS_which is the the case for now. However, if we ever need to add a Python specific environment variable, how can we do that in this system?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.
First, the prefix is configurable, so it can be changed as needed. If there are ever cases where the variables don't follow the straight forward
"SOME_PREFIX" + key.upper()format, we'll add a dictionary in source that maps to those values based on the requirements. For example: "region" could map to "AWS_DEFAULT_REGION". Support for use case like this will be added later based on the need.