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

Add initial RFC document (endpoints 2.0) #1637

Merged
merged 13 commits into from
Dec 21, 2022
Merged

Add initial RFC document (endpoints 2.0) #1637

merged 13 commits into from
Dec 21, 2022

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Aug 16, 2022

Motivation and Context

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

Rendered

design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
Comment on lines 337 to 342
ShapeType.STRING -> StringShape.builder().id("smithy.api#String").build()
ShapeType.BOOLEAN -> BooleanShape.builder().id("smithy.api#Boolean").build()
else -> TODO("unsupported type")
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to have a comment here linking to the spec for allowed types once that's published

design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti
Copy link
Collaborator

Rendered

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

Looks great! I have mostly minor questions and corrections.

design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
### Overriding Endpoints

In the general case, users will not be impacted by Endpoints 2.0 with one exception: today, users can provide a
global endpoint provider that can override different services. There is a single `ResolveAwsEndpoint` trait that
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this trait live?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

currently it's in aws_smithy_http but it's possible it should end up in aws_smithy_types

design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
Comment on lines 150 to 151
// update the URI on the returned endpoint to localhost while preserving the other properties
base_endpoint.builder().uri("http://localhost:8888").build()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What other properties?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the properties which contain auth info e.g.

// module: `aws_smithy_types::endpoint`
// potential optimization to reduce / remove allocations for keys which are almost always static
// this can also just be `String`
type MaybeStatic<T> = Cow<'static, T>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is always used for str, why make it generic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no reason, can remove

// - builder, designed to be invoked / used by generated code
```

> **What's an Endpoint property?**
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to be higher up since endpoint properties are mentioned in code samples above.

design/src/rfcs/rfc_0020_endpoints_20.md Outdated Show resolved Hide resolved
- [ ] Endpoint rules code generator
- [ ] Endpoint params code generator
- [ ] Endpoint tests code generator
- [ ] Implement ruleset standard library functions as inlineables. Note: pending future refactoring work, the `aws.`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the standard rule functions going into inlineables to keep them hidden?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh mentioned this pull request Aug 17, 2022
4 tasks
@82marbag 82marbag changed the title Add initial RFC document Add initial RFC document (endpoints 2.0) Aug 22, 2022
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti added the rfc Marks a PR as an RFC label Nov 10, 2022
@rcoh rcoh marked this pull request as ready for review November 14, 2022 19:49
@rcoh rcoh requested review from a team as code owners November 14, 2022 19:49
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@@ -0,0 +1,599 @@
<!-- Give your RFC a descriptive name saying what it would accomplish or what feature it defines -->
RFC: Endpoints 2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this file needs to be deleted?

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh enabled auto-merge (squash) December 21, 2022 15:25
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh merged commit 2e3fa57 into main Dec 21, 2022
@rcoh rcoh deleted the endpoints-20-rfc branch December 21, 2022 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Marks a PR as an RFC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants