Support granular control of specifying runtime crate versions #1635

weihanglo · 2022-08-15T12:02:11Z

Motivation and Context

Related: #1416

Support a more granular control on specifying runtime crate versions in smithy-build.json runtime config.

The current behaviour is not ideal. You need to specify { version: "DEFAULT" } to get the default version. You can only specify the same version for all runtime crates, which is somehow inconvenient.

Description

This PR mainly deals with two things:

Make default default. You don't need to specify anything to become default.
Give users the ability to specify different versions for different runtime crates.

BREAKING: after this PR, in smity-build.json the path
rust-codegen.runtimeConfig.version no longer exists. Instead, a new
field versions comes in. It's an objct mapping a runtime crate name
to a version string. There is also a special key DEFAULT, which is
presetted as detected runtime version but open to override. Crates
without version specified would be set as the same version as which
associated to key DEFAULT.

An example smithy-build.json might look like:

{
  "version": "1.0",
  "projections": {
    "simple": {
      "plugins": {
        "rust-codegen": {
          "runtimeConfig": {
            "versions": {
               "DEFAULT": "0.48", // Set your own default version.
                                  // Leave blank for codegen auto-detecting a version.
               "aws-smithy-http": "0.47", // Can specify a different version.
               "aws-smithy-types": "0.46",
               "aws-smithy-json": "0.45",
            }
          },
          "service": ...
        }
      }
    }
  }
}

Testing

Tweak the value in buildSrc/src/main/kotlin/CodegenTestCommon.kt and see if your runtime crate version changed.

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

**BREAKING**: after this PR, `in smity-build.json` the path `rust-codegen.runtimeConfig.version` no longer exists. Instead, a new field `versions` comes in. It's an object mapping a runtime crate name to a version string. There is also a special key `DEFAULT`, which is presetted as detected runtime version but open to override. Crates without version specified would be set as the same version as which associated to key `DEFAULT`. Signed-off-by: Weihang Lo <[email protected]>

Signed-off-by: Weihang Lo <[email protected]>

weihanglo · 2022-08-15T13:47:52Z

Open question:

Shall we support more customizations such like setting relative path for each runtime crate?
Shall we be compatible with the old version field without breaking everyone's build script?

Signed-off-by: Weihang Lo <[email protected]>

jdisanti · 2022-08-15T17:47:40Z

CHANGELOG.next.toml

+[[smithy-rs]]
+message = "Support granular control of specifying runtime crate versions"
+references = ["smithy-rs#1416"]
+meta = { "breaking" = true, "tada" = true, "bug" = false }


For breaking changes, add instructions to the message on what changes need to be made when upgrading. You can switch the message to triple quotes if it needs to be multi-line.

Updated. Please review again. Thanks!

CHANGELOG.next.toml

Signed-off-by: Weihang Lo <[email protected]>

david-perez

I think the implementation is correct, but it would help if there'd be unit tests for these main scenarios (correct me if I'm wrong if this is not what's meant to happen):

If user specifies relativePath in runtimeConfig, then that always takes precedence over versions.
User does not specify the versions object. The version number of the code-generator should be used as the version for all runtime crates.
User specifies versions key but leaves the object empty. The version number of the code-generator should be used as the version for all runtime crates.
User specifies versions object, setting DEFAULT. That version is used for all runtime crates.
User specifies versions object, setting explicit version numbers for some runtime crates. Then the rest of the runtime crates use the code-generator's version as their version.
User specifies versions object, setting DEFAULT and setting version numbers for some runtime crates. Then the specified version in DEFAULT is used for all runtime crates, except for those where the user specified a value for in the map.
If the user specifies a version in the versions map for a crate whose name does not correspond to a runtime crate, we fail.

I think all unit tests can start with RuntimeConfig.fromNode(), and then you assert that the correct RuntimeConfig object was parsed.

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt

david-perez · 2022-08-16T11:29:40Z

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt

+ * A mapping for crate name to user specified version.
+ */
+@JvmInline
+value class CrateVersionMap(


TIL about value class. Very interesting.

This is the first time we introduce them in the codebase. I wonder why we're not using them? They seem to be better at encoding "transparent" newtypes, like here, something for which we are relying on data class.

cc someone who actually knows Kotlin @rcoh.

no reason not to use them, TIL about them TBH, they seem handy!

We often end up with more than one field which is why data class is usually useful but for single-member types this seems great!

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt

CHANGELOG.next.toml

david-perez · 2022-08-16T11:52:46Z

I'm thinking that it would also be cool if we allowed the user to specify concrete versions for third-party crates e.g. the http crate. That would allow them to mitigate a CVE-like situation by rolling back that crate version without us having to publish a new version of smithy-rs.

We can probably tackle it in another PR.

rcoh · 2022-08-16T13:50:02Z

I'm thinking that it would also be cool if we allowed the user to specify concrete versions for third-party crates e.g. the http crate. That would allow them to mitigate a CVE-like situation by rolling back that crate version without us having to publish a new version of smithy-rs.

@jdisanti and I discussed this recently—I think it would be a great idea and it would also allow us to unify dependency versions across the entire codebase

Co-authored-by: david-perez <[email protected]>

Signed-off-by: Weihang Lo <[email protected]>

If there is nothing provided. Fallback to use code generator version. Signed-off-by: Weihang Lo <[email protected]>

Signed-off-by: Weihang Lo <[email protected]>

weihanglo · 2022-08-17T11:48:28Z

Tests added. Thanks @david-perez for the suggestion.

If the user specifies a version in the versions map for a crate whose name does not correspond to a runtime crate, we fail.

This has yet been implemented. Shall we add the check?

CHANGELOG.next.toml

codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypesTest.kt

Co-authored-by: david-perez <[email protected]>

david-perez · 2022-08-17T17:39:13Z

If the user specifies a version in the versions map for a crate whose name does not correspond to a runtime crate, we fail.

This has yet been implemented. Shall we add the check?

We can leave it if you want. I was thinking that it would be a UX improvement (think a frustrated user who misspelled a crate name in the versions map and wondering why they're not seeing the override in the code generated crate).

But delivering this feature would entail us having to maintain a list of the crates that we're publishing and keeping it up to date when there are additions or removals. Unless we automate this check in CI, we're likely going to forget.

Signed-off-by: Weihang Lo <[email protected]>

weihanglo · 2022-08-17T17:44:35Z

All review commented addressed. Thank you!

If the user specifies a version in the versions map for a crate whose name does not correspond to a runtime crate, we fail.

This has yet been implemented. Shall we add the check?

We can leave it if you want. I was thinking that it would be a UX improvement (think a frustrated user who misspelled a crate name in the versions map and wondering why they're not seeing the override in the code generated crate).

But delivering this feature would entail us having to maintain a list of the crates that we're publishing and keeping it up to date when there are additions or removals. Unless we automate this check in CI, we're likely going to forget.

That's exactly what I worry about. We could have a separate PR to do that if we want.

github-actions · 2022-08-17T18:24:36Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-08-17T22:09:21Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

weihanglo force-pushed the weihanglo/runtime-config branch 3 times, most recently from f8bc6bf to 23d40b3 Compare August 15, 2022 12:42

weihanglo added 2 commits August 15, 2022 13:57

Update changelog

0088847

Signed-off-by: Weihang Lo <[email protected]>

weihanglo force-pushed the weihanglo/runtime-config branch from 23d40b3 to 0088847 Compare August 15, 2022 13:24

fix sdk runtime generation

fa0b1bc

This comment was marked as outdated.

Sign in to view

Only detect default runtime version for crates.io deps

f591868

Signed-off-by: Weihang Lo <[email protected]>

This comment was marked as outdated.

Sign in to view

weihanglo marked this pull request as ready for review August 15, 2022 17:35

weihanglo requested a review from a team as a code owner August 15, 2022 17:35

jdisanti reviewed Aug 15, 2022

View reviewed changes

Add how to migrate from version to versions for runtimeConfig

7c5d8f9

Signed-off-by: Weihang Lo <[email protected]>

weihanglo requested a review from jdisanti August 16, 2022 05:43

This comment was marked as outdated.

Sign in to view

david-perez requested changes Aug 16, 2022

View reviewed changes

Apply suggestions from code review

2b7043f

Co-authored-by: david-perez <[email protected]>

This comment was marked as outdated.

Sign in to view

weihanglo added 4 commits August 16, 2022 20:58

Mention fallback version mechanism for runtime crates

b31e0b4

Signed-off-by: Weihang Lo <[email protected]>

Fix missing local in code comment

f4db8b6

Remove path/versions check as we accept empty definition

8a96998

If there is nothing provided. Fallback to use code generator version. Signed-off-by: Weihang Lo <[email protected]>

Test for RuntimeConfig and RunTimeCrateLocation

5be9b3c

Signed-off-by: Weihang Lo <[email protected]>

weihanglo requested a review from david-perez August 17, 2022 11:48

This comment was marked as outdated.

Sign in to view

david-perez reviewed Aug 17, 2022

View reviewed changes

Apply suggestions from code review

60b78db

Co-authored-by: david-perez <[email protected]>

Conform with existing multiline string style

71feb35

Signed-off-by: Weihang Lo <[email protected]>

david-perez approved these changes Aug 17, 2022

View reviewed changes

david-perez added the needs-sdk-review label Aug 17, 2022

jdisanti approved these changes Aug 17, 2022

View reviewed changes

jdisanti removed the needs-sdk-review label Aug 17, 2022

weihanglo enabled auto-merge (squash) August 17, 2022 21:27

weihanglo disabled auto-merge August 17, 2022 21:28

weihanglo enabled auto-merge (squash) August 17, 2022 21:29

Merge branch 'main' into weihanglo/runtime-config

f4cded2

weihanglo merged commit e185990 into main Aug 17, 2022

weihanglo deleted the weihanglo/runtime-config branch August 17, 2022 22:20

david-perez mentioned this pull request Aug 26, 2022

When runtimeConfig is _not_ specified, the default behavior is still not using the latest runtime crate versions on crates.io #1676

Closed

weihanglo mentioned this pull request Aug 26, 2022

missing RuntimeConfig node should behave the same as {} #1678

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support granular control of specifying runtime crate versions #1635

Support granular control of specifying runtime crate versions #1635

weihanglo commented Aug 15, 2022 •

edited

Loading

weihanglo commented Aug 15, 2022

This comment was marked as outdated.

This comment was marked as outdated.

jdisanti Aug 15, 2022

weihanglo Aug 16, 2022

This comment was marked as outdated.

david-perez left a comment

david-perez Aug 16, 2022 •

edited

Loading

rcoh Aug 16, 2022

david-perez commented Aug 16, 2022

rcoh commented Aug 16, 2022

This comment was marked as outdated.

weihanglo commented Aug 17, 2022

This comment was marked as outdated.

david-perez commented Aug 17, 2022

weihanglo commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

Support granular control of specifying runtime crate versions #1635

Support granular control of specifying runtime crate versions #1635

Conversation

weihanglo commented Aug 15, 2022 • edited Loading

Motivation and Context

Description

Testing

Checklist

weihanglo commented Aug 15, 2022

This comment was marked as outdated.

This comment was marked as outdated.

jdisanti Aug 15, 2022

Choose a reason for hiding this comment

weihanglo Aug 16, 2022

Choose a reason for hiding this comment

This comment was marked as outdated.

david-perez left a comment

Choose a reason for hiding this comment

david-perez Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

rcoh Aug 16, 2022

Choose a reason for hiding this comment

david-perez commented Aug 16, 2022

rcoh commented Aug 16, 2022

This comment was marked as outdated.

weihanglo commented Aug 17, 2022

This comment was marked as outdated.

david-perez commented Aug 17, 2022

weihanglo commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

weihanglo commented Aug 15, 2022 •

edited

Loading

david-perez Aug 16, 2022 •

edited

Loading