team: add organization_access

[bendrucker]

Description

Adds support for specifying organization_access and visibility to tfe_team:

https://www.terraform.io/docs/cloud/api/teams.html#request-body

Testing plan

1. Add an organization_block or visibility to a tfe_team
2. Build the provider and apply

Output from acceptance tests

TESTARGS="-run ^TestAccTFETeam_" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run ^TestAccTFETeam_ -timeout 15m
?   	github.com/terraform-providers/terraform-provider-tfe	[no test files]
=== RUN   TestAccTFETeam_basic
--- PASS: TestAccTFETeam_basic (4.39s)
=== RUN   TestAccTFETeam_full
--- PASS: TestAccTFETeam_full (3.69s)
=== RUN   TestAccTFETeam_import
--- PASS: TestAccTFETeam_import (3.79s)
PASS
ok  	github.com/terraform-providers/terraform-provider-tfe/tfe	12.189s
?   	github.com/terraform-providers/terraform-provider-tfe/version	[no test files]

Module Updates

This depends on hashicorp/go-tfe#113 (v0.7). I updated the version in go.mod, ran go mod tidy, and ran go mod vendor to update the vendor directory.

[bendrucker]

Any chance I can get a review for this? Thanks!

[bendrucker]

I went ahead and added visibility here as well, since otherwise I would have run into conflicts. Tests are passing, docs are updated, let me know if there's anything else I can do here!

[bendrucker]

@chrisarcand Anything I can do to help get this released? It's not urgent, just hoping to close out it. Happy to split up the go-tfe upgrade and the actual changes required here if that's easier.

[chrisarcand]

Hello again! Your timing is actually extraordinary as sorting this out was actually a part of my afternoon already, and I've just now had a chance to respond 😄 Thanks again for your patience here and your work adding visibility to the client.

For posterity, I think a quick explanation of why this took so long to be added and reviewed is in order!

This feature was proposed long ago in #52, but ultimately pushed off a bit while we considered the following: This API response is fairly unique as these attributes are conditionally included depending on your permission status. That is, if you are not in the Owners team of the organization in Terraform Cloud, these attributes aren't present at all, considered sensitive information.

In the case of the provider here, this means that if you are, for some reason, to share configuration/state with another token/user, there will exist a thrashing scenario where organization access attributes are recorded and removed from state over and over again.

However, after much discussion and seeing this feature out in the wild for the last year, it is reasonable to expect a baseline expectation of permission level on a configuration and that state will not be shared amongst users of a different permission level. If the permission level of the user changes and the user is no longer authorized to a resource or a subset of a resource, errors/thrashing will happen - but that is no different from any other use of Terraform today. There's nothing unique about this situation.

This is all to say that I'm happy to get this polished up and out the door as-is! Thanks for your work here.

---

Got some changes to suggest below, but most importantly there appears to be an update issue that I don't readily see what the problem is at the moment: When you set any of these attributes to false after setting them to true initially, they don't appear to update properly in the API.

We'll need to set up a test for this and verify the behavior a bit.

[chrisarcand]

Hmm I'm not sure we want this to be marked as a computed value. Computed is used to represent values that are not user configurable or aren't known at time of an apply or plan operation, which this shouldn't be.

[bendrucker]

I set this based on the following comment from the plugin SDK:

// If Computed is true, then the result of this value is computed
// (unless specified by config) on creation.

I take that to mean that any with a server set default could be "computed," though this probably moot when it's an optional field where the zero value of the type is equal to the server value. If the server were capable of defaulting to true, I think this might need to be set though? I can remove it here.

[bendrucker]

Hmm, was mis-remembering what values visibility could have. I think this needs to be computed since the API will respond with visibility = "secret" regardless of whether the user has set a value and we'll persist that value to state. I would expect Terraform to present a diff from "secret" -> null on subsequent runs if we don't account for this. I think computed takes care of this, but correct me if I'm wrong.

[chrisarcand]

since the API will respond with visibility = "secret" regardless of whether the user has set a value

Nope, the API should respond with whatever value the user set - and valid options are "secret" or "organization". The default is "secret".

• Computed is very much for unknown values at plan-time that are not user-configurable - things like dates and UUIDs.
• Default is for known values to be used when something isn't explicitly set within the configuration.

For our case here, these are all values that have known defaults that we can provide at plan-time; so they can all be defaulted appropriately. (So this visibility section should not be computed, and should have a default of "secret").

When that's all finished, given a configuration like this:

resource "tfe_team" "test-1" {
  name         = "my-team-name"
  organization = var.organization
  organization_access {
    manage_policies = true
    manage_workspaces = false
  }
}

We should see a plan output like this:

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # tfe_team.test-1 will be created
  + resource "tfe_team" "test-1" {
      + id           = (known after apply)
      + name         = "my-team-name"
      + organization = "my-cool-organization"
      + visibility   = "secret"

      + organization_access {
          + manage_policies     = true
          + manage_vcs_settings = false
          + manage_workspaces   = false
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

See the Schema Behaviors documentation for more info on both of these behaviors.

[bendrucker]

Thanks! Glad my timing was good. 😉

The caveats makes sense, appreciate the explanation. I agree that differing authorization will inevitably cause problems and that it's really a general limitation.

I'll get an update test added/passing and then address the other comments. Should be later this week!

[bendrucker]

Back a bit sooner than expected 🙂! Resolved the docs/style suggestions, thanks for those. Remaining are:

1. Whether visibility needs to be computed to prevent its server-assigned default from being raised as a diff (team: add organization_access #155 (comment))
2. I added a test that updates both visibility and organization access, but was unable to reproduce the issue you described where updates aren't propagated. Since testAccCheckTFETeamExists reads directly from API and then we're making assertions on the response, I feel pretty confident the update is happening. But if you tell me what you were seeing I'm happy to look into it.

[bendrucker]

And for good measure, all team acceptance tests are still passing:

$ TESTARGS="-run TestAccTFETeam_" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFETeam_ -timeout 15m
?   	github.com/terraform-providers/terraform-provider-tfe	[no test files]
=== RUN   TestAccTFETeam_basic
--- PASS: TestAccTFETeam_basic (3.94s)
=== RUN   TestAccTFETeam_full
--- PASS: TestAccTFETeam_full (3.60s)
=== RUN   TestAccTFETeam_full_update
--- PASS: TestAccTFETeam_full_update (5.90s)
=== RUN   TestAccTFETeam_import
--- PASS: TestAccTFETeam_import (3.59s)
PASS
ok  	github.com/terraform-providers/terraform-provider-tfe/tfe	17.288s
?   	github.com/terraform-providers/terraform-provider-tfe/version	[no test files]

[chrisarcand]

Alright let's get this wrapped up! ^_^ Thanks again for your patience, very much appreciated.

1. Defaults, Computed values, and visibility sorted out below.
2. Either your changes in 21613fe actually subtly changed the behavior or I just totally screwed up testing last time, because everything works as expected now! Awesome 👍🏻

With 1. addressed we should be good to go and I'll cut a release ASAP.

[chrisarcand]

since the API will respond with visibility = "secret" regardless of whether the user has set a value

Nope, the API should respond with whatever value the user set - and valid options are "secret" or "organization". The default is "secret".

• Computed is very much for unknown values at plan-time that are not user-configurable - things like dates and UUIDs.
• Default is for known values to be used when something isn't explicitly set within the configuration.

For our case here, these are all values that have known defaults that we can provide at plan-time; so they can all be defaulted appropriately. (So this visibility section should not be computed, and should have a default of "secret").

When that's all finished, given a configuration like this:

resource "tfe_team" "test-1" {
  name         = "my-team-name"
  organization = var.organization
  organization_access {
    manage_policies = true
    manage_workspaces = false
  }
}

We should see a plan output like this:

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # tfe_team.test-1 will be created
  + resource "tfe_team" "test-1" {
      + id           = (known after apply)
      + name         = "my-team-name"
      + organization = "my-cool-organization"
      + visibility   = "secret"

      + organization_access {
          + manage_policies     = true
          + manage_vcs_settings = false
          + manage_workspaces   = false
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

See the Schema Behaviors documentation for more info on both of these behaviors.

[bendrucker]

Thank you Chris! Added those defaults and removed Computed.

[chrisarcand]

Looks great! Thank you!