Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add vault_kv2_delete module #304
Add vault_kv2_delete module #304
Changes from all commits
91dd47c
23ebf50
dec95f6
73bd8f3
f2e5231
dece66c
e330ef2
330d380
936b466
6db80c8
6c0c2d6
3e523bf
cc76a15
9cafc72
1b3aeb1
ccba8d3
8c5d801
c29ce68
df98b67
0e908ed
35eb69f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, the module does not account for check mode, and so with this set to
True
, check mode would delete secrets 😱One issue currently (same reason status is always
changed
) is that without checking the current status of the secret, check mode has nothing to do but returnchanged
.I'm mulling over the idea of using metadata reads to get current versions status/secret existence to be able to solve both of those, but their use should be optional (a client may have permission to delete but not to hit those other endpoints).
I'll look at this again tomorrow and think about whether that makes sense at all, and if so whether it should be in this PR or a follow-up.
The first thing we should do is add an integration test and unit test (example) for check mode before making any additional changes, and those tests should fail right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I think for now the empty result with no Vault connection is fine.
The unit tests are great! Please also add an integration test for check mode; it should be easy, just checking that the result
is changed
and that the deletion did not happen.Even though the unit test covers this a little more thoroughly, it's always good to have it covered by integration too just to have the code actually invoked by ansible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to check for some more exceptions, depending on what
hvac
might raise/handle (best to check the source there to get an idea what to handle here).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like hvac does not do any error handling for this. I also peeked at the Vault source, and it doesn't do anything special in the delete itself, just passing through any errors it gets such as any errors updating the metadata. If theres anything else you can think of, I'd be happy to add it.