Skip to content

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Oct 21, 2023

Issue #

n/a

Description of changes

#1072 added a BOM and version catalog artifact. This PR adds smithy-kotlin to our BOM and re-exports smithy-kotlin runtime from the aws-sdk-kotlin version catalog for convenience.

e.g.

dependencies {
     implementation(awsSdk.runtime.smithy.kotlin.http.client.engine.crt)
     implementation(awsSdk.services.s3)
}

Alternatives

smithy-kotlin also publishes a version catalog. Originally I had wanted to get rid of a redeclaring these in the SDK libs.versions.toml and just use that artifact directly for both the aws-runtime build files and in the BOM/version catalog from the SDK. Unfortunately version catalogs can only be created from settings.gradle.kts (with libs.versions.toml being created implicitly by convention). This doesn't play well with composite builds (you can't use the version catalog from an include build of smithy-kotlin).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aajtodd aajtodd requested a review from a team as a code owner October 21, 2023 16:02
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

}
}

val ignoredSmithyKotlin = setOf(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is it worth ignoring based on patterns like "contains test" or "contains codegen"? It's less explicit but maybe more likely to prevent the accidental inclusion of some new module...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was thinking the same but at the end of the day a BOM isn't including those dependencies just setting the versions to use and so even if it were included customers aren't likely depending on test deps unless they manually added them (unlikely). I can go either way though I just didn't want to inadvertently ignore something we actually want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the pattern-based approach more personally. I think it's better to accidentally omit something and add it later than to accidentally include something and remove it later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could go either way but this explicit list seems fine given the low potential for impact

@aajtodd aajtodd added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Oct 23, 2023
}
}

val ignoredSmithyKotlin = setOf(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could go either way but this explicit list seems fine given the low potential for impact

@aajtodd aajtodd merged commit 01c34c9 into main Oct 24, 2023
@aajtodd aajtodd deleted the feat-bom branch October 24, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants