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

migrate SDKs from http::Request to azure_core::Request #833

Merged
merged 35 commits into from
Jun 21, 2022

Conversation

ctaggart
Copy link
Contributor

@ctaggart ctaggart commented Jun 18, 2022

This completes the migration from http::Request to azure_core::Request. This should make moving to HTTP pipelines easier. Some highlights:

  • rewrote execute_request_check_status function
    • instead of http::Response<Bytes>, it returns a new azure_core::CollectedResponse
  • replaced http:HeaderMap with azure_core::headers::Headers
    • migrated several helper functions like get_as_str
    • added some or_err variants like get_as_str_or_err
  • replaced some helper functions for http:request::Builder with functions in azure_core::Request
    • add_optional_header
    • add_optional_header_ref
    • add_mandatory_header

@cataggar cataggar changed the title migrate from http:Request to azure_core::Request migrate from http:Request to azure_core::Request Jun 18, 2022
@ctaggart ctaggart changed the title migrate from http:Request to azure_core::Request migrate from http::Request to azure_core::Request Jun 19, 2022
@cataggar cataggar changed the title migrate from http::Request to azure_core::Request migrate SDKs from http::Request to azure_core::Request Jun 19, 2022
@cataggar cataggar marked this pull request as ready for review June 20, 2022 13:16
@cataggar cataggar requested a review from rylev June 20, 2022 13:16
@cataggar cataggar requested a review from bmc-msft June 20, 2022 15:00
sdk/core/src/http_client.rs Outdated Show resolved Hide resolved
sdk/core/src/http_client.rs Show resolved Hide resolved
@cataggar cataggar requested a review from bmc-msft June 20, 2022 18:36
@cataggar cataggar requested a review from bmc-msft June 20, 2022 22:25
sdk/core/src/headers/mod.rs Show resolved Hide resolved
sdk/core/src/headers/mod.rs Show resolved Hide resolved
sdk/core/src/options.rs Show resolved Hide resolved
sdk/core/src/error/http_error.rs Outdated Show resolved Hide resolved
sdk/core/src/request_options/next_marker.rs Outdated Show resolved Hide resolved
sdk/data_tables/src/continuation_next_table_name.rs Outdated Show resolved Hide resolved

let request = request.body("".to_owned())?;
request.set_body("");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? Is this not the default?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure.

common_storage_response_headers: headers.try_into()?,
etag: etag_from_headers(headers)?.into(),
location: headers
.get("location")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.get("location")
.get_as_parsed::<Url>::("location")

Copy link
Member

Choose a reason for hiding this comment

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

No get_as_parsed yet. Would be nice.

sdk/storage/src/core/headers/mod.rs Outdated Show resolved Hide resolved
@cataggar cataggar mentioned this pull request Jun 21, 2022
@bmc-msft bmc-msft merged commit cd89ca1 into Azure:main Jun 21, 2022
@ctaggart ctaggart deleted the http_client branch August 2, 2022 02:59
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.

4 participants