Skip to content
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

[Breaking] Moves httpChecksum trait to aws.protocols namespace. #938

Closed

Conversation

skotambkar
Copy link
Contributor

Description of changes:

  • Updates the httpChecksum trait to follow the revised design.
  • As discussed, the Trait is now moved to aws.protocols#httpChecksum namespace.
  • Removed previous implementations (trait class and validator) from smithy.model package.
  • Removed references to old trait implementation defined assmithy.api#httpChecksum.
  • Moved docs to aws-core.rst and updated their content.

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

…trait to follow the new design. Removes stale code and updates implementation, tests and documentation
@skotambkar skotambkar requested a review from a team as a code owner October 13, 2021 12:47
Copy link
Member

@mtdowling mtdowling left a comment

Choose a reason for hiding this comment

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

I didn't quite finish reviewing yet and need to finish aws-core.rst and HttpChecksumTraitValidator.java. I'm leaving my review now before the weekend. I'll pick this up next week.

HttpChecksumTrait trait = operationShape.expectTrait(HttpChecksumTrait.class);
headers.addAll(getChecksumHeaders(trait.getRequestProperties()));
} else if (operationShape.hasTrait(HttpChecksumRequiredTrait.class)) {
if (operationShape.hasTrait(HttpChecksumRequiredTrait.class)) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this more nuanced? Don't we need to inject all of the x-amz-checksum-* headers here? And isn't httpChecksumRequired deprecated now?

Copy link
Contributor

Choose a reason for hiding this comment

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

The new x-amz-checksum-* headers should be applied in smithy-aws-apigateway-openapi

docs/source/1.0/spec/aws/aws-core.rst Show resolved Hide resolved
docs/source/1.0/spec/aws/aws-core.rst Show resolved Hide resolved
docs/source/1.0/spec/aws/aws-core.rst Show resolved Hide resolved
docs/source/1.0/spec/aws/aws-core.rst Show resolved Hide resolved
docs/source/1.0/spec/aws/aws-core.rst Show resolved Hide resolved
Copy link
Contributor

@kstich kstich left a comment

Choose a reason for hiding this comment

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

Partial review, other verification of this will require some of the spec updates to validate against.

HttpChecksumTrait trait = operationShape.expectTrait(HttpChecksumTrait.class);
headers.addAll(getChecksumHeaders(trait.getRequestProperties()));
} else if (operationShape.hasTrait(HttpChecksumRequiredTrait.class)) {
if (operationShape.hasTrait(HttpChecksumRequiredTrait.class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The new x-amz-checksum-* headers should be applied in smithy-aws-apigateway-openapi

docs/source/1.0/spec/aws/aws-core.rst Show resolved Hide resolved
@kstich
Copy link
Contributor

kstich commented Nov 5, 2021

Closing in favor of #972

@kstich kstich closed this Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants