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

fix: VAULT_SKIP_VERIFY inverted behavior #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PierreBeucher
Copy link
Contributor

@PierreBeucher PierreBeucher commented May 16, 2024

VAULT_SKIP_VERIFY=1 was actually ENABLING verify while VAULT_SKIP_VERIFY=0 was SKIPPING verify. We want the opposite.

I was able to cargo build but not cargo test in a nix develop shell, I had failure:

$ cargo test
   Compiling vaultrs v0.7.2 (/home/pbeucher/git/vaultrs)
error[E0277]: the trait bound `Vec<std::string::String>: From<&[std::string::String; 2]>` is not satisfied
   --> tests/identity.rs:224:44
    |
224 |     identity::entity::batch_delete(client, &[entity1.id.to_string(), entity2.id.to_string()])
    |     ------------------------------         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<&[std::string::String; 2]>` is not implemented for `Vec<std::string::String>`
    |     |
    |     required by a bound introduced by this call
    |
    = note: required for `&[std::string::String; 2]` to implement `Into<Vec<std::string::String>>`
note: required by a bound in `batch_delete`
   --> /home/pbeucher/git/vaultrs/src/identity/entity.rs:77:43
    |
77  | pub async fn batch_delete<T: fmt::Debug + Into<Vec<String>>>(
    |                                           ^^^^^^^^^^^^^^^^^ required by this bound in `batch_delete`
help: consider dereferencing here
    |
224 |     identity::entity::batch_delete(client, *&[entity1.id.to_string(), entity2.id.to_string()])
    |                                            +

For more information about this error, try `rustc --explain E0277`.
error: could not compile `vaultrs` (test "identity") due to previous error
warning: build failed, waiting for other jobs to finish...
[Vaultrs Devshell]$ rustc --version
rustc 1.70.0-nightly (8be3c2bda 2023-03-24)

This does not seem related to my local changes, maybe Nix dev shell is broken?

But I hope change is simple enough, if a reviewer or owner can test it on its own :)

VAULT_SKIP_VERIFY=1 was actually ENABLING verify while VAULT_SKIP_VERIFY=0 was SKIPPING verify. We want the opposite.
Copy link
Collaborator

@Haennetz Haennetz left a comment

Choose a reason for hiding this comment

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

Thanks for the MR we should use it like the vault CLI uses it.
Only one suggestion to use assert! instate of assert_eq! but I leave it up to you if you want to apply them

env::remove_var(VAULT_SKIP_VERIFY);
let client = build_client();
assert!(client.settings.verify);
assert_eq!(client.settings.verify, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert_eq!(client.settings.verify, true);
assert!(client.settings.verify);

env::set_var(VAULT_SKIP_VERIFY, value);
let client = build_client();
assert!(!client.settings.verify);
assert_eq!(client.settings.verify, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert_eq!(client.settings.verify, true);
assert!(client.settings.verify);

@@ -63,24 +63,27 @@ fn test_should_verify_tls() {
for value in ["", "1", "t", "T", "true", "True", "TRUE"] {
env::set_var(VAULT_SKIP_VERIFY, value);
let client = build_client();
assert!(client.settings.verify);
// Setting truthy value for SKIP_VERIFY should disable verify
assert_eq!(client.settings.verify, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert_eq!(client.settings.verify, false);
assert!(!client.settings.verify);

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