-
Notifications
You must be signed in to change notification settings - Fork 870
Add CBOR Protocol Support for Request Marshalling and Response Unmarshalling #3942
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
base: development
Are you sure you want to change the base?
Conversation
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.
We are missing the generator change that when a service says they are going to use Cbor that the generated nuspec for that service includes a reference to the new AWSSDK.Extensions.CborProtocol
extension package.
I modified SQS in my local workspace to use Cbor and confirmed the marshallers were using Cbor but the generated AWSSDK.SQS.nuspec
only list AWSSDK.Core
as a dependency.
generator/.DevConfigs/6a90e4df-251e-45f8-808a-1a088cf0b25e.json
Outdated
Show resolved
Hide resolved
generator/.DevConfigs/6a90e4df-251e-45f8-808a-1a088cf0b25e.json
Outdated
Show resolved
Hide resolved
@@ -67,6 +69,10 @@ | |||
{"<#=member.PropertyName#>", payload => new <#=member.DetermineType()#>Unmarshaller().Unmarshall(context);}, | |||
<# | |||
} | |||
else if (protocol == "Cbor") | |||
{ | |||
// TODO |
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.
Make the generator fail if we go in here to avoid us accidently shipping a service update and thinking it was successful
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.
Currently working on event streaming and planning to add the code once a new PR is approved.
@@ -104,6 +110,10 @@ | |||
{ "<#=member.PropertyName#>", payload => new <#=this.Config.ClassName#>EventStreamException(Encoding.UTF8.GetString(payload.Payload), new <#=member.ModelShape#>Unmarshaller().Unmarshall(EventStreamUtils.ConvertMessageTo<#=protocol#>Context(payload))) }, | |||
<# | |||
} | |||
else if (protocol == "Cbor") | |||
{ | |||
// TODO |
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.
Make the generator fail if we go in here to avoid us accidently shipping a service update and thinking it was successful
@normj This is a huge miss, thank you for pointing it out , will work on updating |
…hy Version 1.58.0
…and Smithy v1.58.0
…e file updates. (#3914) * Updated notice files for system.formats.cbor. * Updated the build to package AWSSDK.Extensions.CborProtocol.
290a127
to
a7e190d
Compare
Description
This PR is for
CBOR
protocol feature branch todevelopment
.It includes the following PRs that were reviewed as part of this feature branch:
Motivation and Context
DOTNET-7384
Testing
AWSSDK.Extensions.CborProtocol
project.CloudWatch
andSecretsManager
operations with CBOR.Screenshots (if appropriate)
Types of changes
Checklist
License