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 new_http_client & fix cosmos_client #274

Merged
merged 5 commits into from
Jun 1, 2021

Conversation

ctaggart
Copy link
Contributor

@ctaggart ctaggart commented May 31, 2021

let http_client: Arc<Box<dyn HttpClient>> = Arc::new(Box::new(reqwest::Client::new()));

let http_client = azure_core::new_http_client();
  • removes the unnecessary Box in the Arc<Box. It seems to work fine without. Did I miss something?
  • disables checking of Hyper implementation of HttpClient until it is fixed fix & enable HttpClient for Hyper #275

@ctaggart ctaggart changed the title make cosmos tests pass again add new_http_client & fix cosmos_client May 31, 2021
use azure_core::*;

#[derive(Clone, Debug)]
enum TransportStack {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why you would have an HttpClient or a Pipeline. May be it can this can be reintroduced in a subsequent pull request.

@ctaggart ctaggart marked this pull request as ready for review May 31, 2021 17:48
@ctaggart ctaggart requested review from rylev, MindFlavor and heaths May 31, 2021 22:54
Copy link
Contributor

@rylev rylev 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!

@@ -51,52 +46,76 @@ impl CosmosOptions {
}
}

/// Create a Pipeline from CosmosOptions
fn new_pipeline_from_options(options: CosmosOptions) -> Pipeline {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I personally like keeping the struct definition and the impl together which would mean moving these functions below the impl block. That's definitely not a blocker though.

Copy link
Contributor Author

@ctaggart ctaggart Jun 1, 2021

Choose a reason for hiding this comment

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

I'm fine with either. I'll change it to be in the impl. Let's change it to a impl when the pipeline stuff gets refactored.

@ctaggart ctaggart merged commit 114df85 into Azure:master Jun 1, 2021
@ctaggart ctaggart deleted the pipeline_problems branch June 1, 2021 16:05
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.

fix & enable HttpClient for Hyper cosmos create_database unable to get pipeline in cosmos e2e tests
2 participants