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

instance: fix round-trip serialization #159

Closed
wants to merge 2 commits into from

Conversation

josephholsten
Copy link
Contributor

@josephholsten josephholsten commented Jun 20, 2017

builds on #151, attempting to fix #113

@josephholsten josephholsten changed the title instance: update acc test [WIP] instance: update acc test Jun 20, 2017
@josephholsten josephholsten force-pushed the acc-test-instance branch 6 times, most recently from b86fcac to 054d51f Compare June 27, 2017 19:24
@josephholsten josephholsten changed the title [WIP] instance: update acc test instance: fix round-trip serialization Jun 27, 2017
include notes for deprecated attributes and fixes for acceptance
tests
Copy link
Contributor

@codycushing codycushing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting up an instance with a plan then running apply again without any change causes the instance to be destroyed and recreated--when it diffs it thinks the subnet is different for some reason even though it's exactly the same.

@josephholsten
Copy link
Contributor Author

@codycushing what config are you using? I'm not seeing that with the config in the acc test:

resource "baremetal_core_instance" "t" {
  availability_domain = "${data.baremetal_identity_availability_domains.ADs.availability_domains.0.name}"
  compartment_id      = "${var.compartment_id}"
  image               = "` + FastestImageID + `"
  shape               = "` + SmallestShapeName + `"
  subnet_id           = "${baremetal_core_subnet.s.id}"
}

@codycushing
Copy link
Contributor

It's with a plan using the nested vnic object:

resource "baremetal_core_instance" "instance1" {
  availability_domain = "${lookup(data.baremetal_identity_availability_domains.ADs.availability_domains[0],"name")}" 
  compartment_id = "${var.compartment_ocid}"
  display_name = "be-instance1"
  image = "${lookup(data.baremetal_core_images.image-list.images[0], "id")}"
  shape = "VM.Standard1.1"

  create_vnic_details {
    subnet_id = "${var.subnet}"
    display_name = "vnic1"
    assign_public_ip = true
    hostname_label = "be-instance1"
  },
}

@josephholsten
Copy link
Contributor Author

@codycushing I see what's happening now!

essentially, after the instance is created, the impl is wanting to set resource.baremetal_core_instance.instance1.subnet_id from the response. I was hoping https://github.com/oracle/terraform-provider-baremetal/pull/159/files#diff-930932b2c63a550ddecdf09c110553bdR242 would keep that from breaking things, but I'd actually need to hack the state (and violate layering) in order to trick terraform into thinking that a null resource.baremetal_core_instance.instance1.subnet_id doesn't need to change.

Talking with @grubernaut, it looks like it's not really feasible to support the old resource.baremetal_core_instance.instance1.subnet_id attribute and the new resource.baremetal_core_instance.instance1.create_vnic_details.0.subnet_id in the same impl, so we need a backwards incompatible change.

While we're at it, I'd really like to rename the create_vnic_details attribute to vnic, which makes more sense when reading attributes, and will be more clear when (if?) instances support more than one vnic.

@josephholsten
Copy link
Contributor Author

I'm going to create a variant of this PR that is backward incompatible, but has state migration so no data (or resources) are broken

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform refresh is not getting remote state ?
2 participants