Manage Variable Sets

[rexredinger]

Description

Add support for variable sets

TODO: Get go-tfe updates in a tagged release for reference.

Testing plan

1. Run specs
2. Try to provision a global varset
3. Try to provision a varset assigned to particular workspaces
4. Add variables
5. Read the variable sets from datasources
6. destroy everything

External links

Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.

• go-tfe documentation
• API documentation
• Related PR

Output from acceptance tests

🍔_alex: terraform-provider-tfe $ TESTARGS="-run TestAccTFEVariable" envchain tfe_provider make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEVariable -timeout 15m
?   	github.com/hashicorp/terraform-provider-tfe	[no test files]
=== RUN   TestAccTFEVariableSetsDataSource_basic
--- PASS: TestAccTFEVariableSetsDataSource_basic (8.39s)
=== RUN   TestAccTFEVariablesDataSource_basic
--- PASS: TestAccTFEVariablesDataSource_basic (7.69s)
=== RUN   TestAccTFEVariableSet_basic
--- PASS: TestAccTFEVariableSet_basic (6.55s)
=== RUN   TestAccTFEVariableSet_update
--- PASS: TestAccTFEVariableSet_update (10.49s)
=== RUN   TestAccTFEVariableSet_import
--- PASS: TestAccTFEVariableSet_import (7.06s)
=== RUN   TestAccTFEVariable_basic
--- PASS: TestAccTFEVariable_basic (6.55s)
=== RUN   TestAccTFEVariable_basic_variable_set
--- PASS: TestAccTFEVariable_basic_variable_set (6.36s)
=== RUN   TestAccTFEVariable_update
--- PASS: TestAccTFEVariable_update (12.05s)
=== RUN   TestAccTFEVariable_update_key_sensitive
--- PASS: TestAccTFEVariable_update_key_sensitive (11.97s)
=== RUN   TestAccTFEVariable_import
--- PASS: TestAccTFEVariable_import (7.53s)
PASS
PASS	github.com/hashicorp/terraform-provider-tfe/tfe	85.186s
?   	github.com/hashicorp/terraform-provider-tfe/version	[no test files]
PASS
...

[sebasslash]

Phew, ok... this PR is a juicy one and I will definitely need to give it a second pass (and have a second pair of eyes). For now, I simply wanted to give back enough to chew on while I thoroughly re-review.

All in all, awesome work and thank you so much for this! 🔥 Feel free to debate any of my points as I may have left some feedback without the proper context.

[sebasslash]

I might be wrong on this, but from what I've read in the API docs workspace and variable IDs are included in the response body for the list endpoint? So you can simply loop through the Workspaces field without the need to make an extra API call:

for _, ws :=  range vs.Workspaces {
  workspaces = append(workspaces, ws.ID)
}

[rexredinger]

My thinking here was to not put the extra load of looking up and serializing all the vars and workspaces until we know which one we care about. There are realistic scenarios where I believe the List call could become quite slow if asked to collect all that extra data. Happy to chat more about it if that seems unfounded.

[sebasslash]

Let's add a test check for the variable_ids attribute as well. However, we'd typically structure tests as follows:

• TestAccTFEVariableSetsDataSource_basic: Test the required attributes only
• TestAccTFEVariableSetsDataSource_full: Test the required and optional attributes. Each attribute has its own test case. For attributes that are aggregate types we test for two things: the length/number-of-keys, and that each element matches the desired ID

[rexredinger]

Since the ids are determined at run time how can I reference them from inside the test func declaration? Looking for prior art and coming up short.

[sebasslash]

Ditto on computed attributes showing up in the configuration (we should only have arguments)

[sebasslash]

I may be wrong here and computed attributes that are also arguments are totally valid.

[rorychatterton]

Referencing my post in this thread: #391 (comment)

By coupling the variable set creation logic, with the assignment logic (workspace_ids), this resource becomes much harder to consume.

This means that you have to create the variable set and assign it to child workspaces, all in the same workspace.

Is it possible to further split tfe_variable_set resource further to decouple the logic?

[sebasslash]

This makes sense to me, I definitely think the workspace_ids attribute being both a computed property and an argument can make it harder to reason about your configuration, especially if that variable set is attached to a workspace outside your configuration. Your config might look like:

resource "tfe_variable_set" "foo" {
  # other attributes here
  workspace_ids = ["ws-ID1"]
}

and your state file:

"workspace_ids": ["ws-ID1", "ws-ID2", "ws-ID3"] 

(which I think it's weird 'cause it creates a disjunction between intent and reality)

Expressing relationships as resources has been done before, the examples I can think of are: tfe_organization_module_sharing and tfe_organization_membership. Doing so for variable sets, we can at least be explicit about what VS->Wkspace relationships are managed by Terraform, while keeping workspace_ids as the source of truth for all relationships (including those not managed by Terraform).

[rorychatterton]

Appreciate this PR is closed. Now that you have explained it, I can understand why that could be difficult to work with.

The Vault Provider deals with this with the use of an explicit = true and external_policies = true flag, as seen in these two resources:
identity_group_policies
identity_group_policies

This tells Terraform to ignore any items that weren't previously managed under state to avoid the disfunction you spoke of earlier. I haven't looked closely enough at the provider code to understand how that works under the hood.

You mentioned you want to decouple the conversation. Please advise where you would like to track that going forth if looking for further input :)

[sebasslash]

I'm going to hit approve on this one 🥷 . While there is an interesting discussion to be had regarding how best to represent object relationships, the team implementing this has notified me there have already been internal discussions regarding this and have decided a change like this is beyond scope.

Truthfully, this is part of a larger underlying issue with provider (and not just this resource) and should be addressed in future releases. I apologize to those wanting the decoupling!