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

Diff mismatch when changing ForceNew and k/v attribute together #1508

Closed
phinze opened this issue Apr 13, 2015 · 3 comments · Fixed by #1515
Closed

Diff mismatch when changing ForceNew and k/v attribute together #1508

phinze opened this issue Apr 13, 2015 · 3 comments · Fixed by #1515
Assignees

Comments

@phinze
Copy link
Contributor

phinze commented Apr 13, 2015

I'm tracking this with google_compute_instance, but I believe it's a core bug.

Given

A resource definition with:

  • A ForceNew attribute
  • A Map attribute

e.g.

&schema.Resource{
  Schema: map[string]*schema.Schema{
    "foo": {
      Type: schema.TypeString,
      Required: true,
      ForceNew: true,
    },
    "bar": {
      Type: schema.TypeMap,
      Optional: true,
      Elem: schema.TypeString,
    },
  },
}

When

I attempt to apply a change that updates both the ForceNew attr and updates the map in such a way that that changes the set of keys.

e.g.

From a successful apply of this config that creates the resource:

resource "some_resource" "some_name" {
  foo = "one"
  bar {
    one = true
  }
}

An attempt to apply this config:

resource "some_resource" "some_name" {
  foo = "two"
  bar {
    two = true
  }
}

I expect

The resource to be replaced.

Instead I see

Ye olde "diffs didn't match error", like so:

some_resource.some_name: diffs didn't match
some_resource.some_name: reason: attribute mismatch: bar.one

Diff one (diff from the plan) shows "one" going from "true" -> "", and "two" going from "" -> "true".
Diff two (diff during apply) only shows "two" in the diff.

@phinze
Copy link
Contributor Author

phinze commented Apr 13, 2015

cc @sparkprime

@phinze phinze self-assigned this Apr 13, 2015
@phinze
Copy link
Contributor Author

phinze commented Apr 13, 2015

Alright, I've gotten a repro case up at the acceptance test level, but it's difficult to pin down this issue at the unit test level.

I've verified the helper/schema diff behavior - it's not a bug in diff calculation.

The problem comes from the fact that during the Plan graph walk, there's still an existing instance there, but the Apply graph walk visits a "destroy" node and then the normal resource node (which is supposed to recreate). The destroy node deletes the state for the instance, and then the resource node performs a diff against an empty state. This why the second map loses previous keys.

Continuing to investigate along these lines. I think we need to either: (a) Modify the way the diff is produced in a RequiresNew scenario such that the two diffs match, or (b) Loosen diff.Same() to assume two RequiresNew diffs are the same without strict attribute checks

Of course there might be a third path I can't see yet, but that's where we are right now.

phinze added a commit that referenced this issue Apr 13, 2015
fixes #1508

In a DESTROY/CREATE scenario, the plan diff will be run against the
state of the old instance, while the apply diff will be run against an
empty state (because the state is cleared when the destroy node does its
thing.)

For complex attributes, this can result in keys that seem to disappear
between the two diffs, when in reality everything is working just fine.

Same() needs to take into account this scenario by analyzing NewRemoved
and treating as "Same" a diff that does indeed have that key removed.
phinze added a commit that referenced this issue Apr 14, 2015
fixes #1508

In a DESTROY/CREATE scenario, the plan diff will be run against the
state of the old instance, while the apply diff will be run against an
empty state (because the state is cleared when the destroy node does its
thing.)

For complex attributes, this can result in keys that seem to disappear
between the two diffs, when in reality everything is working just fine.

Same() needs to take into account this scenario by analyzing NewRemoved
and treating as "Same" a diff that does indeed have that key removed.
@ghost
Copy link

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

Successfully merging a pull request may close this issue.

1 participant