Always set instance template source images on read.

[paddycarver]

Previously, we were only setting source images for the disks in instance
templates if they weren't set in the config. Instead, let's just always
set them. This fixes our test failures about self_links not matching
names. Also, relative links are safer than just image names, as
Terraform wouldn't notice if another project's image of the same name
got used instead of your project's.

Also fix the setting of tags and tag fingerprints to always set, even if
to the empty value, to fix the tests.

[danawillow]

If a user was previously using this resource and didn't have all their disks defined in their config, then they're going to see a diff immediately after upgrading. I'm fine with this, but we'll want a big warning in the changelog so we don't end up with another app engine fiasco.

[paddycarver]

I investigated when this would break things for users, and if it was worth keeping the old logic:

• The old logic would be expensive and error prone to keep, because we'd need to expand the arbitrary string we accept into a self_link before we could convert it to a relative link. That's finicky logic, and getting it wrong breaks a lot more people, I think, than there are people who'd run into this.
• The only people this should be able to break is users who use instance templates, imported them, and didn't put all their disks in the config. Which is broken, that should diff for them.

I think you're right, messaging in the CHANGELOG is probably the best way to address this.

[danawillow]

we'd need to expand the arbitrary string we accept into a self_link

I'm happy with your analysis, but just wanted to point out that ^ is a thing we do all the time.

[paddycarver]

I'm happy with your analysis, but just wanted to point out that ^ is a thing we do all the time.

I couldn't find an example--I looked in self_link_utils.go, but what complicates this is we could have:

• projects/PROJECT/global/images/family/FAMILY
• projects/PROJECT/global/images/NAME
• a self link
• global/images/NAME
• global/images/family/FAMILY

and that felt like a lot of options to try and wrangle with backwards compatibility. If we have a way to do this, though, I'm game!

[danawillow]

Oh yeah with images it's more complicated, I was thinking just in general: https://github.com/terraform-providers/terraform-provider-google/blob/master/google/field_helpers.go

[paddycarver]

Oh, fair.

Honestly, I think the added complexity probably isn't worth preserving the broken behaviour, and advertising it in the changelog as a bug fix with a warning is probably the right way to go. I think you're in agreement, but do you mind just confirming/correcting?

[danawillow]

Confirmed!

[morgante]

@danawillow Perhaps I'm confused, but I'm not sure how I'm supposed to work around this. I'm seeing a permadiff with this config:

resource "google_compute_instance_template" "default" {
  count       = "${var.module_enabled ? 1 : 0}"
  project     = "${var.project}"
  name_prefix = "default-"

  machine_type = "${var.machine_type}"

  region = "${var.region}"

  tags = ["${concat(list("allow-ssh"), var.target_tags)}"]

  labels = "${var.instance_labels}"

  network_interface {
    network            = "${var.subnetwork == "" ? var.network : ""}"
    subnetwork         = "${var.subnetwork}"
    access_config      = ["${var.access_config}"]
    address            = "${var.network_ip}"
    subnetwork_project = "${var.subnetwork_project == "" ? var.project : var.subnetwork_project}"
  }

  can_ip_forward = "${var.can_ip_forward}"

  disk {
    auto_delete  = "${var.disk_auto_delete}"
    boot         = true
    source_image = "debian-cloud/debian-9"
    type         = "PERSISTENT"
    disk_type    = "${var.disk_type}"
    disk_size_gb = "${var.disk_size_gb}"
  }

  service_account {
    email  = "${var.service_account_email}"
    scopes = ["${var.service_account_scopes}"]
  }

  metadata = "${merge(
    map("startup-script", "${var.startup_script}", "tf_depends_id", "${var.depends_id}"),
    var.metadata
  )}"

  lifecycle {
    create_before_destroy = true
  }
}

Do we need to always specify the fully-qualified source image going forward? If so that seems decidedly suboptimal since projects/debian-cloud/global/images/family/debian-9 is much harder to create and discover than debian-cloud/debian-9.

[paddycarver]

Do we need to always specify the fully-qualified source image going forward? If so that seems decidedly suboptimal since projects/debian-cloud/global/images/family/debian-9 is much harder to create and discover than debian-cloud/debian-9.

That should not be the case. I agree this is suboptimal.

I'm seeing a permadiff with this config:

I think that's a bug, then. Let me investigate this, so we don't roll out the release without a fix for this.

Update: this is a bug, fix incoming.

[paddycarver]

I've opened #1995 to resolve this, but that is blocked by hashicorp/terraform#18795, unfortunately. I'll make sure we get a solution in before release, though. Thanks for catching this and reporting it before the release!

[ghost]

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!