chore: add scaffolding task #1202

aajtodd · 2024-02-05T20:04:32Z

Issue #

fixes #1189

Description of changes

Adds a new scaffold task for service client codegen to generate package metadata once which will be re-used across future build/publish. This allows us to change the logic behind inference over time without breaking existing packages while also serving as a hedge against accidentally breaking existing packages/artifacts.

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

github-actions · 2024-02-05T20:14:01Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

github-actions · 2024-02-05T20:39:18Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

ianbotsf

I think the term "scaffold" is not clear enough on its own. May I suggest "scaffold-new-packages", "update-package-manifest", etc., as terms which are more clear for a task name or a task class name.

ianbotsf · 2024-02-05T21:58:37Z

build-support/src/main/kotlin/aws/sdk/kotlin/gradle/sdk/Naming.kt

@@ -37,3 +37,16 @@ internal fun sdkIdToArtifactName(sdkId: String): String = sdkId.replace(" ", "")
 * catapult! See AwsSdkCatapultWorkspaceTools:lib/source/merge/smithy-model-handler.ts
 */
 fun sdkIdToModelFilename(sdkId: String): String = sdkId.trim().replace("""[\s]+""".toRegex(), "-").lowercase()
+
+// FIX - replace with case utils from smithy-kotlin once we verify we can change the implementation


Nit: FIX → FIXME

ianbotsf · 2024-02-05T22:07:45Z

build-support/src/main/kotlin/aws/sdk/kotlin/gradle/sdk/tasks/Scaffold.kt

+        check(modelFile.isPresent || modelDir.isPresent) { "one of `model` or `model-dir` is required" }
+        check(!(modelFile.isPresent && modelDir.isPresent)) { "only one of `model` or `model-dir` can be set" }


Style: Could combine:

check(modelFile.isPresent != modelDir.isPresent) { "Exactly one of `model` or `model-dir` must be set" }

ianbotsf · 2024-02-05T22:12:43Z

build-support/src/test/kotlin/aws/sdk/kotlin/gradle/sdk/AwsServiceTest.kt

    val modelContents = """
-        ${"$"}version: "2.0"
+        ${"$"}version: "2"
        namespace gradle.test


Question: Why did this change?

I've slept and hacked since then and I don't recall. I want to say something about the model discovery and validation triggering something here and so I just fixed it to make smithy happy again.

services still model this as 2.0, so if it breaks in this test, would it also break once we try to use it for a real service?

probably not, because the CI which bootstraps services passes, still weird.

In Smithy IDL, both of these statements are valid but seem to mean the exact same thing—tooling must support an IDL version of anywhere from 2.0 (inclusive) to 3.0 (exclusive). I'd be curious what broke and if there's maybe a bug somewhere else.

ianbotsf · 2024-02-05T22:14:35Z

build-support/src/test/kotlin/aws/sdk/kotlin/gradle/sdk/AwsServiceTest.kt

            val expected = AwsService(
                "gradle.test#TestService",
-                "aws.sdk.kotlin.services.testgradle",
+                "aws.sdk.kotlin.services.testgradle2",
                "1.2.3",


Question: Is this rename significant?

It's proving that the namespace is taken from the package manifest given vs the inferred namespace from sdkId.

ianbotsf · 2024-02-05T22:20:53Z

build-support/src/main/kotlin/aws/sdk/kotlin/gradle/sdk/PackageManifest.kt

+@Serializable
+data class PackageMetadata(
+    public val sdkId: String,
+    public val namespace: String,
+    public val artifactName: String,
+    public val brazilName: String,
+) {


Suggestion: Add KDocs. In particular, it's not clear that artifactName means "Maven artifact name (i.e., the 'A' in 'GAV')" solely by looking at the code.

aajtodd · 2024-02-06T15:14:48Z

I think the term "scaffold" is not clear enough on its own. May I suggest "scaffold-new-packages", "update-package-manifest", etc., as terms which are more clear for a task name or a task class name.

As the task name? It is qualified by the project :codegen:sdk:scaffold when invoked to be fair which does give it some additional context.

Task names are (err should be) camel cased though so it would be one of:

scaffold
scaffoldNewPackages
updatePackageManifest
etc

I chose scaffold as it sets up any scaffolding for new services. Currently that is only updating the package manifest, in the future it could be more though. Not a hill I want to die on though so if you feel strongly (or anyone else does) just weigh in with your preference.

lauzadis · 2024-02-06T15:29:20Z

I'd vote for updatePackageManifest for ultimate clarity

lauzadis · 2024-02-06T15:19:22Z

build-support/src/test/kotlin/aws/sdk/kotlin/gradle/sdk/AwsServiceTest.kt

    val modelContents = """
-        ${"$"}version: "2.0"
+        ${"$"}version: "2"
        namespace gradle.test


services still model this as 2.0, so if it breaks in this test, would it also break once we try to use it for a real service?

probably not, because the CI which bootstraps services passes, still weird.

lauzadis · 2024-02-06T15:26:35Z

codegen/sdk/build.gradle.kts

@@ -185,6 +192,11 @@ tasks.register("bootstrap") {
    finalizedBy(stageSdks)
 }

+tasks.register<Scaffold>("scaffold") {
+    group = "codegen"
+    description = "Add a new service to packages.json manifest"


nit: it can add or update multiple services

github-actions · 2024-02-06T19:45:05Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

sonarqubecloud · 2024-02-06T20:15:30Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

10 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-02-06T20:18:09Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

aajtodd requested a review from a team as a code owner February 5, 2024 20:04

aajtodd added 5 commits February 5, 2024 15:07

chore: add scaffolding task

6059781

allow directory as input

99e08e4

allow discovery of new packages

40c6c6e

use package metadata to generate artifact name and namespace

841de9a

bootstrap package manifest from existing models

60ee9fa

aajtodd force-pushed the publishing-qa-scaffold branch from ec25475 to 60ee9fa Compare February 5, 2024 20:08

fix lint

7758025

ianbotsf approved these changes Feb 5, 2024

View reviewed changes

lauzadis approved these changes Feb 6, 2024

View reviewed changes

feedback

e170b84

aajtodd added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Feb 6, 2024

fix lint

574264a

fix windows tempdir issue with unclosed test resource

0a67cac

ianbotsf approved these changes Feb 6, 2024

View reviewed changes

lauzadis approved these changes Feb 6, 2024

View reviewed changes

aajtodd merged commit fc8538e into main Feb 6, 2024
15 of 16 checks passed

aajtodd deleted the publishing-qa-scaffold branch February 6, 2024 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add scaffolding task #1202

chore: add scaffolding task #1202

aajtodd commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

ianbotsf left a comment

ianbotsf Feb 5, 2024

ianbotsf Feb 5, 2024

ianbotsf Feb 5, 2024

aajtodd Feb 6, 2024

lauzadis Feb 6, 2024

ianbotsf Feb 6, 2024

ianbotsf Feb 5, 2024

aajtodd Feb 6, 2024

ianbotsf Feb 5, 2024

aajtodd commented Feb 6, 2024

lauzadis commented Feb 6, 2024

lauzadis Feb 6, 2024

lauzadis Feb 6, 2024

github-actions bot commented Feb 6, 2024

sonarqubecloud bot commented Feb 6, 2024

github-actions bot commented Feb 6, 2024

		check(modelFile.isPresent \|\| modelDir.isPresent) { "one of `model` or `model-dir` is required" }
		check(!(modelFile.isPresent && modelDir.isPresent)) { "only one of `model` or `model-dir` can be set" }

chore: add scaffolding task #1202

chore: add scaffolding task #1202

Conversation

aajtodd commented Feb 5, 2024 • edited Loading

Issue #

Description of changes

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

ianbotsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aajtodd commented Feb 6, 2024

lauzadis commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2024

sonarqubecloud bot commented Feb 6, 2024

Quality Gate passed

github-actions bot commented Feb 6, 2024

aajtodd commented Feb 5, 2024 •

edited

Loading