Make consul_keys behavior less surprising #5210
Merged
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.
The
consul_keys
resource has a number of rough edges that make its behavior rather surprising. @phinze addressed a lot of its bugginess in a recent change, but its general design and implementation approach remained rather unusual.In particular:
value
attribute in thekey
blocks is not updated on refresh. This meant that Terraform does not notice "drift" in the configuration and show it in the plan, but theUpdate
implementation secretly updates those changed keys anyway, leading to surprise and confusion.delete
attribute inkey
blocks applies only when the entire resource is removed, and not when individualkey
blocks are removed. This means the resource is able to add and update keys but not to remove keys.The main driver for this change is to make
delete
work for individual key blocks, so removing a key block and applying has the expected effect of removing just that key from the store.The implementation of this depends on using set difference to detect added and removed key blocks, which in turn required restoring the behavior of refreshing
value
so that Terraform would actually identify and fix differences in the underlying store. An unintended but positive side-effect of this implementation choice was fixing the first point above, so that Terraform will now report correctly in the plan that it intends to update the affected key.In the process of implementing this I altered the implementation style to be what I'd consider a more "conventional" Terraform provider implementation, with separately-implemented
Create
andUpdate
. However, that created a lot of code repetition and so I factored out the noise of interacting with the Consul API into a separate private client that is designed around Terraform's needs.There are several confusing aspects of this resource that this PR does not address and, as far as I can tell, that can't be addressed without a breaking interface change:
key
blocks are implemented as a set rather than as a map (withpath
as the key) means that the resource still produces some rather confusing diffs in cases where the value of a particular key is being updated: rather than appearing as an update of thevalue
field as you'd expect, it instead appears as a removal of an entire set member and the addition of another one with the samepath
.I've intentionally not attempted to address these in this PR, but wanted to call them out.