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

Make sure compiling without tls support is possible #253

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jcfj
Copy link

@jcfj jcfj commented May 29, 2023

Without this PR, compiling the current master with no-default-features fails with

error[E0599]: no method named `danger_accept_invalid_certs` found for struct `ClientBuilder` in the current scope
   --> /tmp/fork-dkregistry-rs/src/v2/config.rs:104:14
    |
103 |           let client = reqwest::ClientBuilder::new()
    |  ______________________-
104 | |             .danger_accept_invalid_certs(self.accept_invalid_certs)
    | |             -^^^^^^^^^^^^^^^^^^^^^^^^^^^ method not found in `ClientBuilder`
    | |_____________|
    | 

For more information about this error, try `rustc --explain E0599`.

I've opted for making the Client builder functions that make no sense without tls (insecure_registry and accept_invalid_certs) configuration dependent. Whether that is the right thing to do I don't know, but it might allow a user of the crate to notice specifying the wrong features at compile time.

(I've also bumped the MSVR in the workflow so I can see whether this builds successfully. The CI is still angry, but that looks unrelated.)

jcfj added 2 commits May 29, 2023 13:13
dkregistry hasn't changed, but since some of the dependencies have updated and there is no lockfile, CI would fail without this
@vrutkovs
Copy link
Contributor

cc @PratikMahajan

@jcfj
Copy link
Author

jcfj commented Jun 2, 2023

Hmm, I thought about this a bit more. Maybe it'd be better to just allow passing a reqwest client instance like so. Reqwest can deal with all the cfg stuff, and it'd add some flexibility.

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