-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
team: add organization_access #155
team: add organization_access #155
Conversation
Any chance I can get a review for this? Thanks! |
I went ahead and added |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
tfe/resource_tfe_team.go
Outdated
"visibility": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Computed: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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! |
Co-authored-by: Chris Arcand <chris@chrisarcand.com>
Co-authored-by: Chris Arcand <chris@chrisarcand.com>
Back a bit sooner than expected 🙂! Resolved the docs/style suggestions, thanks for those. Remaining are:
|
And for good measure, all team acceptance tests are still passing:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright let's get this wrapped up! ^_^ Thanks again for your patience, very much appreciated.
- Defaults, Computed values, and
visibility
sorted out below. - 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.
tfe/resource_tfe_team.go
Outdated
"visibility": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Computed: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thank you Chris! Added those defaults and removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you!
Description
Adds support for specifying
organization_access
andvisibility
totfe_team
:https://www.terraform.io/docs/cloud/api/teams.html#request-body
Testing plan
organization_block
orvisibility
to atfe_team
Output from acceptance tests
Module Updates
This depends on hashicorp/go-tfe#113 (v0.7). I updated the version in
go.mod
, rango mod tidy
, and rango mod vendor
to update the vendor directory.