Resource `coderd_template`: key-value pairs should use a map #121

michvllni · 2024-10-29T15:49:56Z

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

The text was updated successfully, but these errors were encountered:

ethanndickson · 2024-10-30T03:42:33Z

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 · 2024-10-30T06:35:25Z

@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 · 2024-10-30T06:53:00Z

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 · 2024-10-31T13:54:02Z

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 · 2024-11-04T07:33:50Z

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

coder-labeler bot added help wanted Extra attention is needed invalid This doesn't seem right labels Oct 29, 2024

ethanndickson removed invalid This doesn't seem right help wanted Extra attention is needed labels Oct 30, 2024

ethanndickson linked a pull request Oct 31, 2024 that will close this issue

DNM: chore!: use map over list for template tf_vars & provisioner_tags #124

Draft

ethanndickson self-assigned this Oct 31, 2024

ethanndickson changed the title ~~resource coderd_template: tf_vars should be a map, not a list~~ Resource coderd_template: key-value pairs should use a map Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource `coderd_template`: key-value pairs should use a map #121

Resource `coderd_template`: key-value pairs should use a map #121

michvllni commented Oct 29, 2024 •

edited

Loading

ethanndickson commented Oct 30, 2024 •

edited

Loading

matifali commented Oct 30, 2024

michvllni commented Oct 30, 2024

ethanndickson commented Oct 31, 2024 •

edited

Loading

michvllni commented Nov 4, 2024

Resource coderd_template: key-value pairs should use a map #121

Resource coderd_template: key-value pairs should use a map #121

Comments

michvllni commented Oct 29, 2024 • edited Loading

ethanndickson commented Oct 30, 2024 • edited Loading

matifali commented Oct 30, 2024

michvllni commented Oct 30, 2024

ethanndickson commented Oct 31, 2024 • edited Loading

michvllni commented Nov 4, 2024

Resource `coderd_template`: key-value pairs should use a map #121

Resource `coderd_template`: key-value pairs should use a map #121

michvllni commented Oct 29, 2024 •

edited

Loading

ethanndickson commented Oct 30, 2024 •

edited

Loading

ethanndickson commented Oct 31, 2024 •

edited

Loading