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

added a vault_kv2_delete with destroy option #449

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

Conversation

Kloppi313
Copy link

@Kloppi313 Kloppi313 commented Aug 16, 2024

SUMMARY

The vault_kv2_delete module can only delete data. There is little code change to also destroy. It seems more reasonable to add the option instead of creating a new module

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

vault_kv2_delete

ADDITIONAL INFORMATION

Idea came up here #446.


Help needed:
How do I break the run, when destroy is true but versions is not set?

@Kloppi313 Kloppi313 changed the title added a vault_kv2_delete with destroy optione added a vault_kv2_delete with destroy option Aug 16, 2024
@briantist briantist self-assigned this Aug 16, 2024
@briantist briantist added the enhancement New feature or request label Aug 16, 2024
@briantist
Copy link
Collaborator

note: there are collection-wide CI failures at the moment that I need to deal with that aren't related to your PR:


While I will consider this change rather than a dedicated destroy module, I do think some of the reasons for having a separate module still stand: namely around testing and option combinations (like versions being required now but only in certain circumstances). Maybe they aren't as severe as I thought either though.

I'll point out that coverage for the existing module is 100%:

I will not want that bring that lower, and this PR will need both integration and unit test additions. The process of adding those should begin highlight how much complexity (or not) this change ends up adding, vs. having a dedicated module, so let's see how adding the tests play out.

(and of course: coverage number is not the end-all-be-all: 100% coverage doesn't mean we're testing all the right things, but <100% does mean we are missing some code paths; I will try to point out any test cases I think are important when I have proper time to review again)

@Kloppi313
Copy link
Author

Kloppi313 commented Aug 19, 2024

Just my 2cents why I did it within this module:

  • I am no programmer, I am sysadmin, so the copycat approach is easier for me
  • no code duplication with a new module
  • I think I would be maintainer of this new module, because I am no programmer, that might be ne good choice
  • a new module would also need tests(?)

Please be advised: the case destroy == true && versions not defined is not covered within the if/else branch, as I do not know how to exit correctly for ansible. This should be a finding of tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants