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

Put HttpWaitStrategy behind a feature flag #704

Closed
rukai opened this issue Jul 21, 2024 · 3 comments · Fixed by #705
Closed

Put HttpWaitStrategy behind a feature flag #704

rukai opened this issue Jul 21, 2024 · 3 comments · Fixed by #705
Milestone

Comments

@rukai
Copy link
Contributor

rukai commented Jul 21, 2024

Most containers will output a log on stdout when they are ready to be used, so needing to send an http request is not always needed.
Putting it behind a feature flag would allow users who dont need this to skip the large reqwest crate and all its deps.

If desired this http_wait feature could be a default feature to make it conveniently enabled by default for users who need it.

I'm happy to make a PR if this is wanted.

@DDtKey
Copy link
Collaborator

DDtKey commented Jul 22, 2024

Hi @rukai 👋

This certainly sounds reasonable.
But in fact, reqwest does't increase compilation time that much. Almost all of its dependencies are common with the essential underlying Docker client - bollard.

In fact, this was the only reason why feature-gate was not used for the http strategy initially (although, might be a mistake).

However, this still includes reqwest itself, and these crates may use different major versions of their deps.

So the feature definitely makes sense and is cargo-way too.

The only question: do we want to make a breaking change for this or should keep enabled by default(for a while, at least)?
We can preserve compatibility and allow the extra dep to be disabled using default-features=false.

Although, changing this later may be confusing.

@rukai
Copy link
Contributor Author

rukai commented Jul 22, 2024

But in fact, reqwest does't increase compilation time that much.

I noticed many Cargo.lock entries dissapear after removing reqwest, although admittedly didnt pay attention to which dependencies they were or how big they were.

The only question: do we want to make a breaking change for this

IMO moving existing functionality behind a default feature flag is a breaking change, since anyone who uses default-features = false and uses the feature would be broken.

@DDtKey
Copy link
Collaborator

DDtKey commented Jul 22, 2024

IMO moving existing functionality behind a default feature flag is a breaking change, since anyone who uses default-features = false and uses the feature would be broken.

Agree, that's breaking change anyway.

So let's go the direct way - make it optional dependency. Hopefully it won't affect many users, because the main driver was testcontainers-modules (clickhouse module, particularly)
And module will be updated transparently (still breaking change though)

I noticed many Cargo.lock entries dissapear after removing reqwest, although admittedly didnt pay attention to which dependencies they were or how big they were

Yeah, I mostly meant big ones - hyper, http, serde and etc. But it's not that important. Still makes sense

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 a pull request may close this issue.

2 participants