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

Allow building VaultClientSettings without address #30

Merged
merged 3 commits into from
Apr 28, 2022

Conversation

r-arias
Copy link
Contributor

@r-arias r-arias commented Apr 18, 2022

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
Copy link
Contributor Author

r-arias commented Apr 22, 2022

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 r-arias force-pushed the fix/validate-default-address branch from c74561f to 3655b35 Compare April 22, 2022 21:24
@r-arias
Copy link
Contributor Author

r-arias commented Apr 22, 2022

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 jmgilman merged commit 9a65ed9 into jmgilman:master Apr 28, 2022
@jmgilman
Copy link
Owner

Thanks for fixing this!

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