fix(codegen): consistently add region as config option when generated client is Sigv4 but not AWS #6156
Conversation
| : "The AWS region to use as signing region for AWS Auth") | ||
| .write("region?: string | __Provider<string>;\n"); | ||
| } | ||
| if (isSigV4Service(settings, model) || isAwsService(settings, model)) { |
There was a problem hiding this comment.
changed condition to AWS or SigV4
| @@ -130,7 +128,7 @@ public List<ExtensionConfigurationInterface> getExtensionConfigurationInterfaces | |||
| Model model, | |||
| TypeScriptSettings settings | |||
| ) { | |||
| if (isAwsService(settings, model)) { | |||
| if (isSigV4Service(settings, model) || isAwsService(settings, model)) { | |||
There was a problem hiding this comment.
changed condition to AWS or SigV4 for consistency.
| return MapUtils.of("region", writer -> { | ||
| writer.addDependency(TypeScriptDependency.INVALID_DEPENDENCY); | ||
| writer.addImport("invalidProvider", "invalidProvider", | ||
| if (isSigV4Service(settings, model) || isAwsService(settings, model)) { |
There was a problem hiding this comment.
changed condition to AWS or SigV4 for consistency.
… client is Sigv4 but not AWS
… client is Sigv4 but not AWS
| writer | ||
| .addImport("AwsCredentialIdentityProvider", null, TypeScriptDependency.SMITHY_TYPES) | ||
| .writeDocs(""" | ||
| if (isSigV4Service(service) && !areAllOptionalAuthOperations(model, service)) { |
There was a problem hiding this comment.
the condition for adding AwsCredentialIdentityProvider is now only isSigV4 and not both isSigV4 and isAws.
Should this instead remain
isAwsService(service) && isSigV4Service(service)
?
There was a problem hiding this comment.
getRuntimeConfigWriters seems to write credentialDefaultProvider only is isAwsService. So maybe that condition should remain here?
| @@ -30,6 +31,7 @@ export const getRuntimeConfig = (config: CodeCatalystClientConfig) => { | |||
| config?.defaultUserAgentProvider ?? | |||
| defaultUserAgent({ serviceId: clientSharedValues.serviceId, clientVersion: packageInfo.version }), | |||
| maxAttempts: config?.maxAttempts ?? DEFAULT_MAX_ATTEMPTS, | |||
| region: config?.region ?? invalidProvider("Region is missing"), | |||
There was a problem hiding this comment.
this is okay to add to CodeCatalyst, because even now it throws an error on missing region (due to the config resolver and not the runtimeConfig default).
| if (!settings.getExperimentalIdentityAndAuth()) { | ||
| // This additional ID&A check is applied because | ||
| // the AwsSdkCustomizeSigV4Auth class also adds region | ||
| // under these conditions. |
There was a problem hiding this comment.
So we want to write region independent of experimentalIdentityAndAuth (when isSigV4Service or isAwsService). With the documentation changing if it isAwsService (client will send requests) v/s just isSigV4Service (signing region). So it seems we can just do this from one place without looking at experimentalIdentityAndAuth.
There was a problem hiding this comment.
moving this to one location then
| writer | ||
| .addImport("AwsCredentialIdentityProvider", null, TypeScriptDependency.SMITHY_TYPES) | ||
| .writeDocs(""" | ||
| if (isSigV4Service(service) && !areAllOptionalAuthOperations(model, service)) { |
There was a problem hiding this comment.
getRuntimeConfigWriters seems to write credentialDefaultProvider only is isAwsService. So maybe that condition should remain here?
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Issue
N/A, internal code generation
Description
The goal is to set the region configuration and all related codegen on a consistent condition, i.e. the service being either AWS service or being a Sigv4 service. (Non-AWS sigv4 services exist).
Testing
Restored unit tests removed in #5497 (committed as 6c9f913 in #5543)