consul: Fix several problems w/ consul_keys update #4787
Conversation
| @@ -277,7 +277,7 @@ func (w *MapFieldWriter) setSet( | |||
| // not the `value` directly is because this forces all types | |||
| // to become []interface{} (generic) instead of []string, which | |||
| // most hash functions are expecting. | |||
| s := &Set{F: schema.Set} | |||
There was a problem hiding this comment.
Oh my... this must've been lurking here since I did the refactoring that added schema.ZeroValue months ago. Sorry about that!
Curious to see if this will fix weird quirky behavior with other resources that use the default hash implementation. I know a few times I've seen "diffs don't match" errors that I then failed to reproduce, and it seems plausible that this would be the cause for at least some of them...
There was a problem hiding this comment.
No worries! I think this may be the first TypeSet using the default implementation that gets written, since the behavior I saw was a nil reference panic, and we definitely would have noticed that elsewhere.
|
This LGTM! Presumably changing the set hash function is going to cause Terraform to see all of the set items as changed, which will cause some extra diff noise but otherwise not do anything bad I think. Not setting the |
phinze
force-pushed
the
phinze/fix-consul-keys-update
branch
from
bb474a8
to
ec6b3e1
Compare
|
Thanks for the review, @apparentlymart! Did another pass on the code and ended up deciding to set back
Good point. The proper thing to do here would be a state migration. I'll look into that.
Yeah, I think you're right about it being a breaking change. The prior behavior was extra diffs when using default, though, so it's basically a |
Implementation notes: * The hash implementation was not considering key value, causing "diffs did not match" errors when a value was updated. Switching to default HashResource implementation fixes this * Using HashResource as a default exposed a bug in helper/schema that needed to be fixed so the Set function is picked up properly during Read * Stop writing back values into the `key` attribute; it triggers extra diffs when `default` is used. Computed values all just go into `var`. * Includes a state migration to prevent unnecessary diffs based on "key" attribute hashcodes changing. In the tests: * Switch from leaning on the public demo Consul instance to requiring a CONSUL_HTTP_ADDR variable be set pointing to a `consul agent -dev` instance to be used only for testing. * Add a test that exposes the updating issues and covers the fixes Fixes #774 Fixes #1866 Fixes #3023
phinze
force-pushed
the
phinze/fix-consul-keys-update
branch
from
ec6b3e1
to
069425a
Compare
|
Okay I followed up with a state migration so the only BC-notable change is that "value" will no longer be updated. @apparentlymart PTAL when you have the chance |
|
LGTM! |
consul: Fix several problems w/ consul_keys update
|
Belated LGTM 😀 |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Implementation notes:
did not match" errors when a value was updated. Switching to default
HashResource implementation fixes this
needed to be fixed so the Set function is picked up properly during
Read
keyattribute; it triggers extradiffs when
defaultis used. Computed values all just go intovar."key" attribute hashcodes changing.
In the tests:
CONSUL_HTTP_ADDR variable be set pointing to a
consul agent -devinstance to be used only for testing.
Fixes #774
Fixes #1866
Fixes #3023