Add ConfigResolver with necessary tests#641
Add ConfigResolver with necessary tests#641ubaskota merged 3 commits intosmithy-lang:config_resolution_mainfrom
Conversation
packages/smithy-aws-core/src/smithy_aws_core/config/resolver.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks Ujjwal.
I left some minor comments and suggestions. However, my biggest concern with this PR is the approach for config resolver sharing.
- It doesn't follow any of the approaches discussed in previous designs
- In cases where multiple threads hit the function before the first result is cached, it will result in each building their own resolver which is unnecessary/duplicate work
packages/smithy-aws-core/src/smithy_aws_core/config/resolver.py
Outdated
Show resolved
Hide resolved
packages/smithy-aws-core/src/smithy_aws_core/config/__init__.py
Outdated
Show resolved
Hide resolved
packages/smithy-aws-core/src/smithy_aws_core/config/resolver.py
Outdated
Show resolved
Hide resolved
| """Initialize the resolver with sources in precedence order. | ||
| :param sources: List of configuration sources in precedence order. The first | ||
| source in the list has the highest priority. The list is copied to |
There was a problem hiding this comment.
Copying it seems a little odd to me; we could simplify this a bit by removing the copy. This kind of goes against the concept of "responsible users" in Python.
The short version is that the Python community is full of responsible users who generally understand that if a property or method is prefixed with an underscore they should not set/use it directly. Those who ignore that are doing so at their own risk.
While I don't expect this to ever matter in practice, the copy here is a net negative for me; it creates additional overhead (both memory-wise and cognitive overhead) which complicated the code without adding value.
That said, it's a single line and not worth a lot of back and forth.
| assert region == ("us-west-2", "source_one") | ||
| assert retry_mode == ("adaptive", "source_two") | ||
|
|
||
| def test_returns_non_string_values(self): |
There was a problem hiding this comment.
Nit: this test is a bit odd to me; I'm not sure what's different about string values that makes them special. I'd think this would be more like "test_resolved_returns_different_types", then we test a string, an int, a bool, and whatever else.
These test make it look like string is the default type that a resolver will return, and I don't think that's true.
Issue #, if available:
Description of changes:
Implements
ConfigResolverto resolve configuration values from multiple sources in precedence order. The resolver iterates through sources and returns the first non-None value found, along with the source name for provenance tracking. Returns(None, None)when no source provides a value.Tests
Added unit tests covering:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.