Resource coderd_template: key-value pairs should use a map

[michvllni]

As the tf_vars are unique ( it is not possible to define a value for a variable more than once ) it would make sense to change the type of the tf_vars property in the template resource to a map.

How it is now:

resource "coderd_template" "template" {
  name        = "example"
  description = "example"

  versions = [
    {
      name      = "example"
      message   = "example"
      directory = "./template"
      active    = true
      tf_vars = [
        {
          name  = "default_project"
          value = "myproject"
        },
        {
          name  = "default_image_variant"
          value = "bookworm"
        },
        {
          name  = "repository"
          value = "github.com/my/repo"
        }
      ]
    }
  ]
}

How it could be:

resource "coderd_template" "template" {
  name        = "example"
  description = "example"

  versions = [
    {
      name      = "example"
      message   = "example"
      directory = "./template"
      active    = true
      tf_vars = {
          "default_project" = "myproject"
          "default_image_variant" = "bookworm"
          "repository" = "github.com/my/repo"
      }
    }
  ]
}

By doing this we could ensure that no variable is defined twice.

It would also greatly improve the plans readability, heres an example:

  # coderd_template.templates["terraform"] will be updated in-place
  ~ resource "coderd_template" "templates" {
      ~ display_name                      = "terraform" -> (known after apply)
        id                                = "442609fa-9d44-40bd-81ac-593bcbe4aec7"
      ~ max_port_share_level              = "public" -> (known after apply)
        name                              = "terraform"
      ~ organization_id                   = "6aa09569-a619-446e-8e7c-8ea848458631" -> (known after apply)
      ~ versions                          = [
          ~ {
              ~ id             = "512d14e5-2173-4091-be70-5b41209df4ac" -> (known after apply)
              ~ message        = "Merged PR 15261: fix: separate tempalte files to create separate messages" -> "Merged PR 15262: fix: terraform 1.9.8"
              ~ name           = "17f1e43475d9d47871b9aa240026e7cf729d0909" -> "54d813a3f801811851aedf69af079ea7370fafc8"
              ~ tf_vars        = [
                  - {
                      - name  = "default_image_variant" -> null
                      - value = "1.9.7" -> null
                    },
                  - {
                      - name  = "image_variants" -> null
                      - value = jsonencode(
                            [
                              - "1.5.7",
                              - "1.7.4",
                              - "1.9.2",
                              - "1.9.5",
                              - "1.9.6",
                              - "1.9.7",
                            ]
                        ) -> null
                    },
                  + {
                      + name  = "default_image_variant"
                      + value = "1.9.8"
                    },
                  + {
                      + name  = "image_variants"
                      + value = jsonencode(
                            [
                              + "1.5.7",
                              + "1.7.4",
                              + "1.9.2",
                              + "1.9.5",
                              + "1.9.6",
                              + "1.9.7",
                              + "1.9.8",
                            ]
                        )
                    },
                    # (8 unchanged elements hidden)
                ]
                # (3 unchanged attributes hidden)
            },
        ]
        # (14 unchanged attributes hidden)
    }

this could be as simple as

  # coderd_template.templates["terraform"] will be updated in-place
  ~ resource "coderd_template" "templates" {
      ~ display_name                      = "terraform" -> (known after apply)
        id                                = "442609fa-9d44-40bd-81ac-593bcbe4aec7"
      ~ max_port_share_level              = "public" -> (known after apply)
        name                              = "terraform"
      ~ organization_id                   = "6aa09569-a619-446e-8e7c-8ea848458631" -> (known after apply)
      ~ versions                          = [
          ~ {
              ~ id             = "512d14e5-2173-4091-be70-5b41209df4ac" -> (known after apply)
              ~ message        = "Merged PR 15261: fix: separate tempalte files to create separate messages" -> "Merged PR 15262: fix: terraform 1.9.8"
              ~ name           = "17f1e43475d9d47871b9aa240026e7cf729d0909" -> "54d813a3f801811851aedf69af079ea7370fafc8"
              ~ tf_vars        = {
                  ~ "default_image_variant" = "1.9.7" -> "1.9.8"
                  ~ "image_variants"        = "[\"1.5.7\",\"1.7.4\",\"1.9.2\",\"1.9.5\",\"1.9.6\",\"1.9.7\"]" -> "[\"1.5.7\",\"1.7.4\",\"1.9.2\",\"1.9.5\",\"1.9.6\",\"1.9.7\",\"1.9.8\"]"
                    # (8 unchanged elements hidden)
              }
                # (3 unchanged attributes hidden)
            },
        ]
        # (14 unchanged attributes hidden)
    }

I understand this is a breaking change, but as we are very early in the product life cycle I thought it might be an option to change this now before lots of people use the provider

Edit: Typo

[ethanndickson]

Ooh, good suggestion! This is the sort of breaking change we might be happy to make for a 1.0 release - or even earlier, perhaps? WDYT @matifali ?

[matifali]

@ethanndickson, I think breaking while we are on 0.0 is okay.x, we can move to 0.1.x and communicate this change in our changelog. This will improve the UX.

---

Also, @michvllni, besides catching duplication, what else do you think we can achieve by converting this list to a map?

[michvllni]

The two main reasons that came to my head are the ones I wrote earlier: Catching duplication and an easier to read plan. Also it is easier to write.
Also, as you can probably see in my plan I currently use a workaround to pass in not only strings but also objects, but I think this would be difficult to implement and isn't in the scope of this issue

[ethanndickson]

With an automated state migration, this ends up being relatively painless for end-users, however:

I think breaking while we are on 0.0 is okay.x, we can move to 0.1.x and communicate this change in our changelog. This will improve the UX.

I'm not sure if we should make a breaking change like this before 1.0.

It's worth mentioning that Terraform resources internally use an integer for their schema versioning. Bumping the template resource schema from 0 to 1 would misalign it with the major version of the provider, and I don't think that's what hashicorp intended for us to do.

I also think our users who have already deployed this at relative scale might get a little peeved if we make a breaking change for a minor release version, especially since they do have to modify the config.

Can't forget the possibility that the state migration goes wrong and they're required to re-import the resource, - I think that would certainly feel like a surprising task for a non-major release.

If we discover anything else in the template resource that requires a breaking change before we finalise a 1.0 release, we'll also be able to include it in the same migration.

[michvllni]

I agree, best put it in a major release. This is what is done in azurerm and other providers as well