-
Notifications
You must be signed in to change notification settings - Fork 29
Update the codegen files that generate config.py to support config resolution. #646
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
Changes from 1 commit
09d2f20
559b3d7
29afa10
9f179b4
5f96840
9d14553
c519501
c827bc2
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 |
|---|---|---|
|
|
@@ -20,5 +20,13 @@ private AwsConfiguration() {} | |
| .type(Symbol.builder().name("str").build()) | ||
| .documentation(" The AWS region to connect to. The configured region is used to " | ||
| + "determine the service endpoint.") | ||
| .nullable(false) | ||
| .useDescriptor(true) | ||
| .validator(Symbol.builder() | ||
| .name("validate_host") | ||
| .namespace("smithy_aws_core.config.validators", ".") | ||
| .addDependency(AwsPythonDependency.SMITHY_AWS_CORE) | ||
| .build()) | ||
| .defaultValue("'us-east-1'") | ||
|
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. Where is this requirement coming from? I think setting a default region on behalf of customers isn't a good idea.
Contributor
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. Noted. Decided to remove the default value for region after discussion. |
||
| .build(); | ||
| } | ||
|
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. General Question: How are we distinguishing between values set by the user and values not set by the user? For example if we ever have a default value set to "my_default_value", how will we know this was set by us or the user?
Contributor
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 don't have a way to that now. We discussed about using a sentinel value to differentiate it when necessary. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -69,6 +69,17 @@ public final class ConfigGenerator implements Runnable { | |||||
| .build()) | ||||||
| .documentation( | ||||||
| "The retry strategy or options for configuring retry behavior. Can be either a configured RetryStrategy or RetryStrategyOptions to create one.") | ||||||
| .nullable(false) | ||||||
| .useDescriptor(true) | ||||||
| .validator(Symbol.builder() | ||||||
| .name("validate_retry_strategy") | ||||||
| .namespace("smithy_aws_core.config.validators", ".") | ||||||
| .build()) | ||||||
| .customResolver(Symbol.builder() | ||||||
| .name("resolve_retry_strategy") | ||||||
| .namespace("smithy_aws_core.config.custom_resolvers", ".") | ||||||
| .build()) | ||||||
| .defaultValue("RetryStrategyOptions(retry_mode=\"standard\", max_attempts=3)") | ||||||
| .build(), | ||||||
| ConfigProperty.builder() | ||||||
| .name("endpoint_uri") | ||||||
|
|
@@ -322,6 +333,8 @@ private void generateConfig(GenerationContext context, PythonWriter writer) { | |||||
| writer.onSection(new AddAuthHelper()); | ||||||
| } | ||||||
|
|
||||||
| writer.onSection(new AddGetSourceHelper()); | ||||||
|
|
||||||
| var model = context.model(); | ||||||
| var service = context.settings().service(model); | ||||||
|
|
||||||
|
|
@@ -335,6 +348,37 @@ private void generateConfig(GenerationContext context, PythonWriter writer) { | |||||
| } | ||||||
|
|
||||||
| var finalProperties = List.copyOf(properties); | ||||||
|
|
||||||
| // Add imports for config resolution | ||||||
| writer.addDependency(SmithyPythonDependency.SMITHY_CORE); | ||||||
| writer.addImport("smithy_core.config.property", "ConfigProperty"); | ||||||
| writer.addImport("smithy_core.config.resolver", "ConfigResolver"); | ||||||
|
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. This is adding an AWS resolver to generic clients that never gets used, please make sure this only goes in AWS clients |
||||||
| writer.addImport("smithy_aws_core.config.sources", "EnvironmentSource"); | ||||||
|
|
||||||
| // Add validator and resolver imports for properties that use descriptors | ||||||
| // Also add imports for types used in default values | ||||||
| for (ConfigProperty property : finalProperties) { | ||||||
| if (property.useDescriptor()) { | ||||||
| if (property.validator().isPresent()) { | ||||||
| var validatorSymbol = property.validator().get(); | ||||||
| writer.addImport(validatorSymbol.getNamespace(), validatorSymbol.getName()); | ||||||
| } | ||||||
| if (property.customResolver().isPresent()) { | ||||||
| var resolverSymbol = property.customResolver().get(); | ||||||
| writer.addImport(resolverSymbol.getNamespace(), resolverSymbol.getName()); | ||||||
| } | ||||||
| // Add imports for types referenced in default values | ||||||
| if (property.defaultValue().isPresent()) { | ||||||
| var defaultValue = property.defaultValue().get(); | ||||||
| // Check if default value uses RetryStrategyOptions | ||||||
| if (defaultValue.contains("RetryStrategyOptions")) { | ||||||
| writer.addDependency(SmithyPythonDependency.SMITHY_CORE); | ||||||
| writer.addImport("smithy_core.retries", "RetryStrategyOptions"); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| final String serviceId = context.settings() | ||||||
| .service(context.model()) | ||||||
| .getTrait(ServiceTrait.class) | ||||||
|
|
@@ -349,6 +393,8 @@ class $L: | |||||
|
|
||||||
| ${C|} | ||||||
|
|
||||||
| ${C|} | ||||||
|
|
||||||
| def __init__( | ||||||
| self, | ||||||
| *, | ||||||
|
|
@@ -358,21 +404,65 @@ def __init__( | |||||
| """, | ||||||
| configSymbol.getName(), | ||||||
| serviceId, | ||||||
| writer.consumer(w -> writeDescriptorDeclarations(w, finalProperties)), | ||||||
| writer.consumer(w -> writePropertyDeclarations(w, finalProperties)), | ||||||
| writer.consumer(w -> writeInitParams(w, finalProperties)), | ||||||
| writer.consumer(w -> initializeProperties(w, finalProperties))); | ||||||
| writer.popState(); | ||||||
| } | ||||||
|
|
||||||
| // Write descriptor declarations for properties using ConfigProperty descriptor | ||||||
| private void writeDescriptorDeclarations(PythonWriter writer, Collection<ConfigProperty> properties) { | ||||||
|
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. If we include the type annotations like this: Then we will give the type hint for IDEs. We could also use a pyi file for this. |
||||||
| boolean hasDescriptors = properties.stream().anyMatch(ConfigProperty::useDescriptor); | ||||||
|
|
||||||
| if (!hasDescriptors) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| writer.write("# Config properties using descriptors (lazy resolution with caching)"); | ||||||
| for (ConfigProperty property : properties) { | ||||||
| if (property.useDescriptor()) { | ||||||
| writer.writeInline("$L = ConfigProperty('$L'", | ||||||
| property.name(), | ||||||
| property.name()); | ||||||
|
|
||||||
| if (property.validator().isPresent()) { | ||||||
| writer.writeInline(", validator=$L", property.validator().get().getName()); | ||||||
| } | ||||||
|
|
||||||
| if (property.customResolver().isPresent()) { | ||||||
| writer.writeInline(", resolver_func=$L", property.customResolver().get().getName()); | ||||||
| } | ||||||
|
|
||||||
| if (property.defaultValue().isPresent()) { | ||||||
| writer.writeInline(", default_value=$L", property.defaultValue().get()); | ||||||
| } | ||||||
|
|
||||||
| writer.write(")"); | ||||||
| } | ||||||
| } | ||||||
| writer.write(""); | ||||||
| } | ||||||
|
|
||||||
| private void writePropertyDeclarations(PythonWriter writer, Collection<ConfigProperty> properties) { | ||||||
| for (ConfigProperty property : properties) { | ||||||
| // Skip descriptor properties - they're declared above | ||||||
| if (property.useDescriptor()) { | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| var formatString = property.isNullable() | ||||||
| ? "$L: $T | None" | ||||||
| : "$L: $T"; | ||||||
| writer.write(formatString, property.name(), property.type()); | ||||||
| writer.writeDocs(property.documentation(), context); | ||||||
| writer.write(""); | ||||||
| } | ||||||
|
|
||||||
| // Add _resolver declaration | ||||||
| writer.write("_resolver: ConfigResolver"); | ||||||
| writer.writeDocs("Shared resolver for all Config instances.", context); | ||||||
|
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.
Suggested change
This is no longer shared, right?
Contributor
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. Yeah, its not shared. I realized it after publishing this PR and decided to wait until the first round of review is completed.
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. It looks like we have a double comment here? Did you intend to delete line 435? |
||||||
| writer.write(""); | ||||||
| } | ||||||
|
|
||||||
| private void writeInitParams(PythonWriter writer, Collection<ConfigProperty> properties) { | ||||||
|
|
@@ -381,9 +471,48 @@ private void writeInitParams(PythonWriter writer, Collection<ConfigProperty> pro | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Handle descriptor properties efficiently with a loop | ||||||
| private void initializeProperties(PythonWriter writer, Collection<ConfigProperty> properties) { | ||||||
| // First, initialize the resolver | ||||||
| writer.write("# Create resolver with environment source"); | ||||||
| writer.write("self._resolver = ConfigResolver(sources=[EnvironmentSource()])"); | ||||||
| writer.write(""); | ||||||
|
|
||||||
| // Then, handle descriptor properties efficiently with a loop | ||||||
| var descriptorProperties = properties.stream() | ||||||
|
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. This is in the generic codegen and is not aws specific - are we adding descriptors and everything to white label SDKs? |
||||||
| .filter(ConfigProperty::useDescriptor) | ||||||
| .map(ConfigProperty::name) | ||||||
| .toList(); | ||||||
|
|
||||||
| if (!descriptorProperties.isEmpty()) { | ||||||
| writer.write("# Set instance values for descriptor properties"); | ||||||
|
|
||||||
| // Generate the list of descriptor property names | ||||||
| writer.writeInline("descriptor_keys = ["); | ||||||
|
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. It's a little odd that we are duplicating the descriptor keys here. Did you consider making the descriptors into a dictionary like so: That way we could use descriptors.keys() instead of two lists. |
||||||
| var iter = descriptorProperties.iterator(); | ||||||
| while (iter.hasNext()) { | ||||||
| writer.writeInline("'$L'", iter.next()); | ||||||
| if (iter.hasNext()) { | ||||||
| writer.writeInline(", "); | ||||||
| } | ||||||
| } | ||||||
| writer.write("]"); | ||||||
| writer.write("for key in descriptor_keys:"); | ||||||
| writer.indent(); | ||||||
| writer.write("value = locals().get(key)"); | ||||||
| writer.write("if value is not None:"); | ||||||
| writer.indent(); | ||||||
| writer.write("setattr(self, key, value)"); | ||||||
| writer.dedent(); | ||||||
| writer.dedent(); | ||||||
| writer.write(""); | ||||||
| } | ||||||
|
|
||||||
| // Finally, initialize non-descriptor properties normally | ||||||
| for (ConfigProperty property : properties) { | ||||||
| property.initialize(writer); | ||||||
| if (!property.useDescriptor()) { | ||||||
| property.initialize(writer); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -418,4 +547,46 @@ def set_auth_scheme(self, scheme: AuthScheme[Any, Any, Any, Any]) -> None: | |||||
| """); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Helper to add get_source method for descriptor properties | ||||||
| private static final class AddGetSourceHelper implements CodeInterceptor<ConfigSection, PythonWriter> { | ||||||
| @Override | ||||||
| public Class<ConfigSection> sectionType() { | ||||||
| return ConfigSection.class; | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public void write(PythonWriter writer, String previousText, ConfigSection section) { | ||||||
| // Check if there are any descriptor properties | ||||||
| boolean hasDescriptors = section.properties() | ||||||
| .stream() | ||||||
| .anyMatch(ConfigProperty::useDescriptor); | ||||||
|
|
||||||
| if (!hasDescriptors) { | ||||||
| // No descriptor properties, just write previous text | ||||||
| writer.write(previousText); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| // First write the previous text | ||||||
| writer.write(previousText); | ||||||
|
|
||||||
| // Add the get_source helper method | ||||||
| writer.write(""" | ||||||
|
|
||||||
| def get_source(self, key: str) -> str | None: | ||||||
| \"""Get the source that provided a configuration value. | ||||||
|
|
||||||
| Args: | ||||||
| key: The configuration key (e.g., 'region', 'retry_strategy') | ||||||
|
|
||||||
| Returns: | ||||||
| The source name ('instance', 'environment', etc.), | ||||||
| or None if the key hasn't been resolved yet. | ||||||
| \""" | ||||||
| cached = self.__dict__.get(f'_cache_{key}') | ||||||
| return cached[1] if cached else None | ||||||
| """); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
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.
The new generated
Configclasses seem to be missing docstrings for the new descriptors config options. We should make sure that gets addressed in the next revision of this PR.