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
hcp_vault_cluster resource changes for adding vault plugins #575
hcp_vault_cluster resource changes for adding vault plugins #575
Changes from 4 commits
ed94c4b
8416d13
629843b
ced527e
325a75d
0ca8f12
5e4dcb8
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.
adding comment to explain why vault plugin type is validated on both create & update & plugin name is only validated on update
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.
I also have a relative question in another comment on
how to do plugin name validation on create
:)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.
can definitely update comments to make this more clear
answered question in other thread : )
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.
Noticed we're only asserting on the
vault_plugin
field on the second update. Could we add more assertions for the other updates and this one?Like on update 1 that the first plugin is added, update 2 the second plugin is added (I think rn it's only asserting on the
venafi SECRET
one and not theoracle DATABASE
), and update 3 thatresource.TestCheckNoResourceAttr
on thevault_plugin
that was removed (similar to how we do it for metrics/audit logs when they are removed) + the one that wasn't removed is still set.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.
so that test case isn't included yet because there is only 1 customer managed plugin to work with. I did some hacky things to test 2 of them. Once more plugins are added, we can update the tests.
hafsa had the same confusion, sorry if this wasn't clear in pr description
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.
Ohh gotcha. Suggestion: Could we maybe have update 3 use a config with no plugins at all and then assert
resource.TestCheckNoResourceAttr
so then acceptance tests run by anyone will test removal without needing the extra setup you did?