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

HTTP Connectors Refactor pt.1 - Laying the groundwork #1144

Merged
merged 13 commits into from
Feb 4, 2022

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Feb 1, 2022

Motivation and Context

This is the first PR in a series that will change the way that HTTP connector are created, configured, and shared throughout the SDK. The goal is providing a better experience for users that want to easily configure various aspects of how the SDK makes HTTP requests.

Because this is such a large PR, here's a high-level breakdown of the changes:

  1. The default_providers module has be broken up into multiple files, one per submodule.
  2. The sts module has be broken up into multiple files, one per submodule.
  3. The aws_config::connector module now has it's own file.
  4. The http_provider module has been renamed to http_credential_provider to better reflect the fact that it contains the HttpCredentialProvider and related code.
  5. Imports in several aws_config modules have be separated into separate groups (libstd imports, same-crate imports, aws and smithy runtime crates). This is an unspoken pattern that we've been following elsewhere.
  6. BREAKING CHANGE The aws_smithy_client::timeout::Settings struct has been fully replaced by aws_smithy_types::timeout::TimeoutConfig. I should have done this when I originally added timeout support but better late than never, right? All structs and functions that referenced aws_smithy_client::timeout::Settings have been updated.
  7. BREAKING CHANGE MakeConnectorFn, HttpConnector, and HttpSettings have been moved from aws_config::provider_config to aws_smithy_client::http_connector. This is in preparation for a later PR that will change how connectors are created and configured. Also, I had to update the way that HttpSetting is created because it's a non-exhaustive struct.
  8. BREAKING CHANGE HttpConnector::make_connector has been renamed to HttpConnector::connector in adherence with how we tend to name the methods on providers that return the thing the provider provides. I'm open to workshopping this name or even reverting this change if y'all don't think it's helpful.
  9. As a result of the changes in point 7, aws_types now depends on aws_smithy_client.
  10. aws_types::Config now has a builder + setter method for setting an HttpConnector. Nothing uses this yet, but it will be used when I do the actual "Config owns Connector" refactor in a later PR. I'm OK with removing this change for now if y'all think it would mislead people.

NOTE: This PR breaks our example for setting timeouts, I'll create a PR to the docs repo addressing that once this one is merged. It'll be a one-line change.

The next PR in this series will focus on an RFC for "Config owns Connector" + an implementation.

Testing

  • I ran the existing tests we had locally and in CI.

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

update: replace timeout::Settings with TimeoutConfig
add: HttpConnector to aws_types::Config and Builder
refactor: divide default_provider.rs modules into separate files
@github-actions
Copy link

github-actions bot commented Feb 1, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 1, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

A new generated diff is ready to view.

@Velfi Velfi changed the title HTTP Connectors Refactor (DRAFT) HTTP Connectors Refactor pt.1 - Laying the groundwork Feb 2, 2022
@Velfi Velfi marked this pull request as ready for review February 2, 2022 19:53
@Velfi Velfi requested a review from a team as a code owner February 2, 2022 19:53
@jdisanti
Copy link
Collaborator

jdisanti commented Feb 2, 2022

I love the splitting out the modules! I think that's going to make it easier to find things.

As a result of the changes in point 6, aws_types now depends on aws_smithy_client.

I understand why this is needed, but at the same time, I'm slightly concerned about tying aws-types to aws-smithy-client since we aim to keep aws-types as stable as possible (from the RFC it was introduced in, "aws-types should be an interface-only crate that is extremely stable").

Should we have an aws-types-like stable interface crate in the Smithy runtime to avoid locking ourselves into certain API decisions? Seems like aws-smithy-types could be this crate, and it does have the TimeoutConfig, so maybe that location makes sense.

Imports in several aws_config modules have be separated into separate groups (libstd imports, same-crate imports, aws and smithy runtime crates). This is an unspoken pattern that we've been following elsewhere.

It's unfortunate that the options to automate this in rustfmt still haven't been stabilized--perhaps we should opt into them anyway. I want to avoid developers changing import style back and forth as my tendency is to lump them all together alphabetically. I don't have a strong preference on the style used; just that its consistency should be enforced/corrected by a computer.

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.

LGTM!

aws/rust-runtime/aws-config/src/lib.rs Outdated Show resolved Hide resolved
@Velfi Velfi enabled auto-merge (squash) February 3, 2022 22:14
@Velfi Velfi disabled auto-merge February 3, 2022 22:14
@github-actions
Copy link

github-actions bot commented Feb 3, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

A new doc preview is ready to view.

@Velfi Velfi enabled auto-merge (squash) February 3, 2022 22:33
@github-actions
Copy link

github-actions bot commented Feb 3, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

A new generated diff is ready to view.

@Velfi Velfi merged commit a1f0652 into main Feb 4, 2022
@Velfi Velfi deleted the refactor/connector-ownership branch February 4, 2022 20:17
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.

2 participants