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

helper/schema: write "attr.#": "0" for empty maps #1824

Merged
merged 1 commit into from
May 7, 2015

Conversation

phinze
Copy link
Contributor

@phinze phinze commented May 6, 2015

This fixes some perpetual diffs I saw in Atlas AccTests where an empty
map (map[string]interface{}{}) was being d.Set for "metadata_full".

Because the MapFieldWriter was not distinguishing between empty and nil,
this trigger the "map delete" logic and no count was written to the
state. This caused subsequent plans to improperly report a diff.

Here we redefine the map delete functionality to explicitly trigger only
on nil, so we catch the .# field for empty maps.

This fixes some perpetual diffs I saw in Atlas AccTests where an empty
map (`map[string]interface{}{}`) was being `d.Set` for "metadata_full".

Because the MapFieldWriter was not distinguishing between empty and nil,
this trigger the "map delete" logic and no count was written to the
state. This caused subsequent plans to improperly report a diff.

Here we redefine the map delete functionality to explicitly trigger only
on `nil`, so we catch the `.#` field for empty maps.
@phinze
Copy link
Contributor Author

phinze commented May 6, 2015

@mitchellh I know we've had a lot of churn in these types of situations, but I believe this change is still correct and good. At least, it makes sense to me. 😀

@mitchellh
Copy link
Contributor

Let's see what the other acceptance tests say... I feel like we did this before and it had issues with computed diffs but we'll see. :)

@radeksimko radeksimko added the core label May 6, 2015
@phinze
Copy link
Contributor Author

phinze commented May 7, 2015

So far I've validated:

  • Fixes heroku test failures with "all_config_vars.#": "" => "<computed>"
  • Fixes atlas test failures with "full_metadata.#": "" => "<computed>"
  • Does not break any google acceptance tests
  • Running all _basic$ AWS acctest now, good so far

@phinze
Copy link
Contributor Author

phinze commented May 7, 2015

Okay AWS is looking good too - @mitchellh how you feelin?

@mitchellh
Copy link
Contributor

Alright let's do it

phinze added a commit that referenced this pull request May 7, 2015
helper/schema: write "attr.#": "0" for empty maps
@phinze phinze merged commit 051ba78 into master May 7, 2015
@phinze phinze deleted the b-write-count-for-empty-maps branch May 7, 2015 15:41
@ghost
Copy link

ghost commented May 2, 2020

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 ghost locked and limited conversation to collaborators May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants