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 an api-version header to all requests #856

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

ahl
Copy link
Collaborator

@ahl ahl commented Jul 9, 2024

Closes #564.

@ahl ahl requested a review from davepacheco July 9, 2024 05:17
@@ -452,6 +452,14 @@ impl Generator {
let dur = std::time::Duration::from_secs(15);

reqwest::ClientBuilder::new()
.default_headers(

Choose a reason for hiding this comment

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

I think this won't cover the new_with_client() case, right?

Copy link
Collaborator Author

@ahl ahl Aug 31, 2024

Choose a reason for hiding this comment

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

Oof. Good point! And 1. new_with_client() is actually the case that matter and 2. we can't modify a reqwest::Client nor can we convert it into a builder.

@ahl
Copy link
Collaborator Author

ahl commented Aug 31, 2024

@davepacheco I think I need to change this progenitor interface more significantly--once we have a reqwest::Client e.g. from new_with_client there isn't anything we can do to glue in new our header. Some options:

// 1
Client::new_with_builder(reqwest::Client::builder().custom_settings());
// 2
Client::new_map(|builder| builder.customsettings());

Are there others? I think I prefer 2 in that it (I think) makes it clear to the user that the Client is in charge and is allowing you to make changes.

@davepacheco
Copy link

Yeah I was afraid of that.

Elsewhere I've had stuff like this accept a ClientBuilder (so, your option #1). Here's an example (this doesn't use the progenitor client, it's just an example of something that needs a reqwest client but wants to customize it). I think this communicates "you can provide your settings, and I will modify further[, and mine take precedence]". It looks nice but it's not as crisp as it looks because it's not clear which settings the function will customize and even if it were, it wouldn't necessarily be stable. I don't think there's a way around that, though.

In your option 2, it's possible to allow either one's settings to override the other's. I'm not sure if that's better or worse.

I don't have a strong feeling but I think (1) would be more composable? Looking at where my example is used, there's:
https://github.com/oxidecomputer/omicron/blob/5bf5f09545b7b74f809c099db5c0b315ea06f9be/end-to-end-tests/src/helpers/ctx.rs#L244-L255

Another example is in the Omicron TLS certificate tests, where I have this helper that keeps track of a few things needed to create a working client:
https://github.com/oxidecomputer/omicron/blob/5bf5f09545b7b74f809c099db5c0b315ea06f9be/nexus/tests/integration_tests/certificates.rs#L640-L649
https://github.com/oxidecomputer/omicron/blob/5bf5f09545b7b74f809c099db5c0b315ea06f9be/nexus/tests/integration_tests/certificates.rs#L683-L690

I think you could do all of this with your (2) but it would mean passing closures around through a few layers instead of ReqwestBuilders, so maybe it makes no difference, but seems a little more annoying to me.

@ahl ahl marked this pull request as draft October 1, 2024 21:14
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.

progenitor clients should specify API version
2 participants