jmgilman / vaultrs

An asynchronous Rust client library for the Hashicorp Vault API
https://docs.rs/vaultrs
MIT License
97 stars 60 forks source link

Allow building VaultClientSettings without address #30

Closed r-arias closed 2 years ago

r-arias commented 2 years ago

Hi,

this is the PR attempting to solve #29

Let me know, if you'd like me to make any changes. The test file I've added is kept fairly simple...

Cheers, R

r-arias commented 2 years ago

I changed the type to Url and made the setter panic on an invalid one, as we discussed.

I realized there is still going to be a breaking change in the API, since software relying on VaultClientBuilder::address being a String will trip over it being a Url now. I don't think that any users will be significantly inconvenienced by it. (In most use cases I can imagine they could go from using &settings.address to using settings.address.as_ref()-- thanks to https://docs.rs/url/2.2.2/url/struct.Url.html#impl-AsRef%3Cstr%3E)

Cheers, R

r-arias commented 2 years ago

Note that one of the test cases is kind of hacky...

For the reasons outlined there, I don't see an easy way to add another test case (that runs, but would be flaky):

#[test]
#[should_panic]
fn build_with_invalid_address_from_env_var() {
    let address_with_unacceptable_scheme = "ftp://example.com";
    env::set_var("VAULT_ADDR", address_with_unacceptable_scheme);

    let _ = VaultClientSettingsBuilder::default().build().unwrap();
}
jmgilman commented 2 years ago

Thanks for fixing this!