Add async resolver for AWS config values#751
Conversation
# Conflicts: # packages/smithy-aws-core/src/smithy_aws_core/config/exceptions.py
| ) | ||
|
|
||
|
|
||
| def validate_retry_mode(retry_mode: str): |
There was a problem hiding this comment.
I'm realizing now that by only accepting standard, we don't allow customers to use existing config files and environments they use for products like boto3. I'm thinking if someone sets legacy can just map that to standard and throw a warning telling them that we don't support legacy (and never will). I think that is better than erroring out. Especially if they have legacy set for something like boto3 which does support it. Maybe doing the same with adaptive make sense until we actually support it. Because we have provenance now, we can event check: if they pass in "adaptive" to the config object itself, that is a hard failure as it is now. However, if it comes from the env or config file, we warn and default to standard as mentioned above. And of course, typos and garbage inputs should be completely rejected anywhere.
Let me know if you think there is a better path forward
| ) | ||
|
|
||
|
|
||
| def validate_retry_strategy_options(value: object) -> None: |
There was a problem hiding this comment.
Edit: See this comment first
This seems to allow retry_strategy_options=RetryStrategyOptions(retry_mode="bogus", max_attempts=-5) which is something we should reject. Why not re-use the validation methods below for mode and max attempts?
| raise ConfigValidationError( | ||
| f"max_attempts must be a number, got {type(max_attempts).__name__}", | ||
| ) |
There was a problem hiding this comment.
Anytime we catch an exception and raise a new one, we should use raise ... from e
|
|
||
|
|
||
| @dataclass(kw_only=True) | ||
| class AsyncAwsConfig: |
There was a problem hiding this comment.
We should expose any public classes / methods we expect customers to use directly in config/__init__.py like below:
from smithy_aws_core.config.aws_config import AsyncAwsConfig
__all__ = [
"AsyncAwsConfig",
...
]AsyncAwsConfig for sure should go there + anything else you think people will use directly.
| """ | ||
|
|
||
| region: str | None = None | ||
| retry_strategy_options: RetryStrategyOptions | None = None |
There was a problem hiding this comment.
retry_mode and max_attempts are resolved independently but bundled into this one field, which causes two problems:
- Provenance: mixed sources collapse via
_strongest_source, soretry_modefrom env +max_attemptsfrom profile reports justENV. - Validation: You are currently doing an isinstance check which doesn't validated the nested values that we really care about
I also think from the customer perspective, retry_mode and max_attempts is more straight forward and doesn't require you having to import and create a new RetryStrategyOptions object.
Can we split them into two top-level fields? Is there something preventing that?
|
|
||
| class EndpointResolutionError(SmithyError): | ||
| """Exception type for all exceptions raised by endpoint resolution.""" | ||
|
|
There was a problem hiding this comment.
I think you may have misunderstood what I was suggesting previously. The errors should live in smithy_aws_core since that's what uses them. However, they should inherit from SmithyError which lives in smithy_core. This pattern exists in smithy_aws_event_stream as well if that helps you better understand: https://github.com/smithy-lang/smithy-python/blob/develop/packages/smithy-aws-event-stream/src/smithy_aws_event_stream/exceptions.py
There was a problem hiding this comment.
nit, not blocking - the rest of this package seems to use relative imports instead of absolute ones. We should be consistent
2 participants
Issue #, if available:
Description of changes:
This change implements the async config resolution pipeline with
AsyncAwsConfigusing aresolve()classmethod as the only construction path. It addsSharedConfigContextfor holding memoized config file data,FieldSpecfor per-field resolution metadata, and provenance tracking viaConfigSource. Resolvers forregionandretry_strategy_optionsare included along with validators for both fields.Testing:
Note: This is a revision to the old PR (#738) that was automatically closed after the feature branch was merged.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.