-
Notifications
You must be signed in to change notification settings - Fork 88
chore: Build service clients with exact dependencies #1996
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
Conversation
@@ -25,7 +25,6 @@ software.amazon.smithy.aws.swift.codegen.model.AWSEndpointTraitTransformer | |||
software.amazon.smithy.aws.swift.codegen.model.AWSDeprecatedShapeRemover | |||
software.amazon.smithy.aws.swift.codegen.AWSClientConfigurationIntegration | |||
software.amazon.smithy.swift.codegen.swiftintegrations.InitialRequestIntegration | |||
software.amazon.smithy.aws.swift.codegen.swiftintegrations.RegistryConfigIntegration |
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 integration used to write a .swiftpm/configuration/registries.json
file for use with Swift Package Registry. It is no longer used and is deleted.
// Exclude test files for internal clients | ||
val fileIsInternalClientGeneratedTests = it.internalClient && details.relativePath.startsWith("Tests") | ||
|
||
fileIsSmokeTests || fileIsInternalClientGeneratedTests |
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.
Here: When copying the internal clients into the SDK, copy the whole project but leave out the Tests/
folder.
This ensures the internal clients' dependencies JSON file is added to the project.
let jsonFileData = FileManager.default.contents(atPath: jsonFilePath) ?? Data("[]".utf8) | ||
let dependencies = try! JSONDecoder().decode([String].self, from: jsonFileData) | ||
return "[" + dependencies.map { ".\($0)" }.joined(separator: ", ") + "]" | ||
} |
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 method above takes the JSON in the generated code and makes it into a literal array of dependencies that can be used in a target.
|
||
private func buildInternalAWSSSOOIDCDependencies() -> String { | ||
buildInternalClientDependencies(name: "AWSSSOOIDC") | ||
} |
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 three methods above create the dependency lists for the 3 internal clients.
@@ -4,35 +4,42 @@ | |||
|
|||
extension Target.Dependency { | |||
// AWS modules |
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.
In the file below:
- "Canned" lists of client dependencies are replaced with custom lists for each client target
- Dependency static vars are renamed to match the name of the target exactly; this makes codegen much simpler
- A few new dependency targets were added because they were being implicitly depended on before but now it will be explicit
- Adjustments to internal client paths
"SmithyRetries", | ||
"SmithyRetriesAPI", | ||
"SmithyTestUtil" | ||
] |
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.
Above is an example of the new JSON that is generated for each client, naming its dependencies.
"AWSWorkSpacesWeb", | ||
"AWSWorkspacesInstances", | ||
"AWSXRay", | ||
let serviceTargets: [String: [Target.Dependency]] = [ |
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.
Info: Fed this to bedrock to do a quick count of dependencies, and result is the following:
- 14 dependencies for 267 targets (most common)
- 15 dependencies for 104 targets
- 16 dependencies for 15 targets
- 17 dependencies for 13 targets
- 13 dependencies for 7 targets
- 18 dependencies for 2 targets
- 19 dependencies for 1 target
As for 5 dependencies not used by 267 targets, here's more info:
The 267 targets typically use these dependencies:
.AWSClientRuntime
.AWSSDKChecksums
.AWSSDKHTTPAuth
.AWSSDKIdentity
.ClientRuntime
.Smithy
.SmithyHTTPAPI
.SmithyHTTPAuthAPI
.SmithyIdentity
.SmithyJSON
.SmithyReadWrite
.SmithyRetries
.SmithyRetriesAPI
.SmithyTestUtil
The additional 5 dependencies that appear in other targets but not in this majority group are:
.SmithyTimestamps
.SmithyWaitersAPI
.SmithyXML
.SmithyEventStreams/.SmithyEventStreamsAPI
.SmithyFormURL
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.
Thanks for the summary, there's a lot of variation.
In practice, every service will link SmithyJSON, SmithyXML, and SmithyFormURL indirectly because all of those are used by the 3 internal service clients. Would be nice if STS moved from AwsQuery to JSON...
private func buildInternalAWSSTSDependencies() -> String { | ||
buildInternalClientDependencies(name: "AWSSTS") | ||
} | ||
|
||
private func buildInternalAWSSSODependencies() -> String { | ||
buildInternalClientDependencies(name: "AWSSSO") | ||
} | ||
|
||
private func buildInternalAWSSSOOIDCDependencies() -> String { | ||
buildInternalClientDependencies(name: "AWSSSOOIDC") |
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.
Please update this doc as needed: https://quip-amazon.com/2Ou5ABMKxHXP/Adding-new-service-client-dependent-credential-resolvers
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.
Thanks for the reminder! The process had already changed when #1995 merged so this definitely needs revision.
Description of changes
When defining a service client target in aws-sdk-swift, the exact dependencies for that target are specified so that no extra services will be linked.
Dependencies.json
for each target, containing the target's dependencies as determined by theSwiftWriter
that generated the target.Package.swift
dependencies for each target, which are rendered intoPackage.swift
.Package.swift.txt
files are removed, along with their generator, and a few other components that were intended for use with Swift Package Registry.New/existing dependencies impact assessment, if applicable
No new dependencies were added to this change.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.