Skip to content

chore: merge ssdk branch into main #2279

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

Merged
merged 6 commits into from
Apr 29, 2021
Merged

Conversation

JordonPhillips
Copy link
Member

@JordonPhillips JordonPhillips commented Apr 19, 2021

This rebases the ssdk branch so that it can be merged into main. Keeping the ssdk branch as a separate feature branch has become cumbersome and will only become more problematic over time.

Note: this build won't pass until this is merged in: smithy-lang/smithy-typescript#315


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

JordonPhillips and others added 4 commits April 19, 2021 16:30
* feat(config-resolver): make region optional for non-AWS client

* feat(codegen): skip integrations that are not relevant for non-AWS services

This is an initial version to get a working version of generated code
that compiles without manual edits in smithy-typescript-ssdk-demo. I
expect to make updates to this logic.

* chore(codegen): address code comments

The minor ones from adamthom-amzn#1

* fix(codegen): use SigV4Trait check instead of ServiceTrait

AddAwsRuntimeConfigTest is checking for some behaviors from
AddAwsAuthPlugin too, which was failing with missing aws.auth#sigv4
trait after my change. Added the trait for now to the test, but unit
tests will need to be added/refactored for all these changes.

* chore(codegen): move isAwsService check to utils class

* chore(codegen): code style formatting

* chore(codegen): check SigV4 trait for including region
@JordonPhillips JordonPhillips changed the title Merge ssdk branch into main chore: merge ssdk branch into main Apr 19, 2021
@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • CodeBuild project: sdk-staging-test
  • Commit ID: 45d352d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@56dae4e). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 422db80 differs from pull request most recent head 3daceed. Consider uploading reports for the commit 3daceed to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2279   +/-   ##
=======================================
  Coverage        ?   59.90%           
=======================================
  Files           ?      467           
  Lines           ?    24583           
  Branches        ?     5834           
=======================================
  Hits            ?    14726           
  Misses          ?     9857           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56dae4e...3daceed. Read the comment docs.

gosar added 2 commits April 26, 2021 18:05
… non-AWS client (aws#2287)

* chore(config-resolver): refactor EndpointsConfig for non AWS services

This reverts an earlier change to EndpointsConfig.ts and instead
provides the new functionality in separate
CustomEndpointsConfig.ts. This will be used as a separate plugin for
endpoint resolution in non AWS clients.

* feat(codegen): Use separate CustomEndpointsConfig for non-AWS clients

This depends on newly added CustomEndpointsConfig in
@aws-sdk/config-resolver package.
Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Looks good. only have a few comments.

.write("region?: string | __Provider<string>;\n");
if (isAwsService(settings, model)) {
writer.writeDocs("Unique service identifier.\n@internal")
.write("serviceId?: string;\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a code suggestion but an idea: why not make serviceId as a required parameter just like custom endpoints? Since it's required to differentiate a request always.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's complicated because the service id is on an aws trait, which we don't want to require.

Comment on lines +93 to +97
@Override
protected void writeDefaultErrorHeaders(GenerationContext context, StructureShape error) {
super.writeDefaultErrorHeaders(context, error);
context.getWriter().write("'x-amzn-errortype': $S,", error.getId().getName());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is used for the ssdk when serializing errors

@JordonPhillips JordonPhillips merged commit 60a4215 into aws:main Apr 29, 2021
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants