Fix REST-XML Codegen Bug, Move metrics to User-Agent header, Update SDKVersion to refer to goModuleVersion #1257
Conversation
skmcgrail
commented
May 13, 2021
skmcgrail
commented
- Make go1.16 available in Travis Java builds
- aws: Updated SDKVersion to refer to goModuleVersion constant.
- Add Changelog Annotation
- Regenerated Clients
- internal/repotools: Support specifying alternative package location for go_module_metadata.go
- Updated middleware to temporarily direct client metadata to user-agent
- Add serviceId and module version to user-agent header
- Fix deserialization of enum types marked with payload trait
…or go_module_metadata.go
| - sudo apt-get -y install golang | ||
| - go get golang.org/dl/go1.16.4 | ||
| - $(go env GOPATH)/bin/go1.16.4 download | ||
| - export PATH=$HOME/sdk/go1.16.4/bin:$PATH | ||
| - export GOROOT=$HOME/sdk/go1.16.4 |
There was a problem hiding this comment.
Should we also add this for go tip - go get golang.org/dl/gotip ?
Just to be aware ahead of time.
There was a problem hiding this comment.
Yeah we can use this strategy to replace our current gotip build process etc.
| "type": "feature", | ||
| "description": "`AddSDKAgentKey` and `AddSDKAgentKeyValue` in `aws/middleware` package have been updated to direct metadata to `User-Agent` HTTP header.", | ||
| "modules": [ | ||
| "." |
There was a problem hiding this comment.
Nit: Just for readability, it would be great if we can alias core as ..
There was a problem hiding this comment.
The reason for not doing this is mainly to avoid having to deal with that condition in multiple spots, "." automatically plays nice with path and filepath as you would expect when you are joining this relative locations to the repository path on the filesystem.
| package config | ||
|
|
||
| // goModuleVersion is the tagged release for this module | ||
| const goModuleVersion = "1.1.7" |
There was a problem hiding this comment.
Nice! Can we export these ? Would be helpful when logging.
| @@ -175,7 +183,7 @@ func writeModuleMetadata(dir string, goPackage string, version string) (err erro | |||
|
|
|||
| return metadataTemplate.Execute(f, metadata{ | |||
| Package: goPackage, | |||
| Version: version, | |||
| Version: strings.TrimPrefix(version, "v"), | |||
There was a problem hiding this comment.
Why do we trim v ? Is this to sort or something?
If we decide to export go module version - we may want to add that v back (to be idiomatic). In that case, may be we should handle prefix v and not trim?
There was a problem hiding this comment.
Mainly since the SDK version constant has always been reported without it. Same with the product version that is sent in the user-agent, so opt-ed for consistency here.
| [modules."."] | ||
| metadata_package = "aws" | ||
|
|
There was a problem hiding this comment.
Repeated comment: can add another module as core pointing to aws?
| @@ -41,13 +41,18 @@ public void writeAdditionalFiles( | |||
| GoDelegator goDelegator | |||
| ) { | |||
| ServiceTrait serviceTrait = settings.getService(model).expectTrait(ServiceTrait.class); | |||
| String servceId = serviceTrait.getSdkId().replace("-", "").replace(" ", "").toLowerCase(); | |||
There was a problem hiding this comment.
Nit: Java replace takes a regex. Right now it will traverse that string thrice I think.
| @@ -379,7 +379,7 @@ private void writePayloadBindingDeserializer( | |||
| } else { // string | |||
| writer.addUseImports(SmithyGoDependency.SMITHY_PTR); | |||
| if (targetShape.hasTrait(EnumTrait.class)) { | |||
| writer.write("v.$L = string(bs)", memberName); | |||
| writer.write("v.$L = $T(bs)", memberName, symbolProvider.toSymbol(targetShape)); | |||
There was a problem hiding this comment.
Nit: can use similar suggestion here - aws/smithy-go#296 (comment)
| @@ -106,7 +106,7 @@ protected void serializeMap(GenerationContext context, MapShape shape) { | |||
| writer.insertTrailingNewline(); | |||
|
|
|||
| // Serialize zero values as empty values. | |||
| GoValueAccessUtils.writeIfZeroValue(context.getModel(), writer, shape.getValue(), "v[i]", () -> { | |||
| GoValueAccessUtils.writeIfZeroValue(context.getModel(), writer, shape.getValue(), "v[key]", () -> { | |||
There was a problem hiding this comment.
This makes me wonder if queryParamsMap serialization need something similar. What is this changing exactly?
There was a problem hiding this comment.
i was not a valid variable here, looks like this was some sort of copy paste error, but there was no code-gen that exercised this path before. the variable in scope that it should have been using was key. Probably was copied from the code that handled lists.
…DKVersion to refer to goModuleVersion (aws#1257) * Fix deserialization of enum types marked with payload trait * Add serviceId and module version to user-agent header * Updated middleware to temporarily direct client metadata to user-agent * internal/repotools: Support specifying alternative package location for go_module_metadata.go * Regenerated Clients * Add Changelog Annotation * aws: Updated SDKVersion to refer to goModuleVersion constant. * Make go1.16 available in Travis Java builds
