-
Notifications
You must be signed in to change notification settings - Fork 104
Add UserTokensEnabled field for Organizations #1225
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
Changes from all commits
0eb1403
8542576
e7cf4aa
0a6a2ac
eef56e3
44f2192
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -320,6 +320,64 @@ func TestOrganizationsUpdate(t *testing.T) { | |
| } | ||
| }) | ||
|
|
||
| t.Run("with new UserTokensEnabled option", func(t *testing.T) { | ||
| orgTest, orgTestCleanup := createOrganization(t, client) | ||
| t.Cleanup(orgTestCleanup) | ||
|
|
||
| assert.True(t, *orgTest.UserTokensEnabled, "user tokens enabled by default") | ||
|
|
||
| // we need to switch to an owner's team token, otherwise the client (which auths with a user token) | ||
| // wont be able to delete the org after we disable user tokens | ||
| teamList, err := client.Teams.List(ctx, orgTest.Name, &TeamListOptions{ | ||
| Names: []string{"owners"}, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // it should be the only team, we just created the org... | ||
| require.Len(t, teamList.Items, 1) | ||
| ownersTeam := teamList.Items[0] | ||
|
|
||
| ownerToken, ownerTokenCleanup := createTeamToken(t, client, ownersTeam) | ||
| t.Cleanup(ownerTokenCleanup) | ||
|
|
||
| ownerClient := testClient(t) | ||
| ownerClient.token = ownerToken.Token | ||
|
|
||
| // disable user tokens for the organization | ||
| options := OrganizationUpdateOptions{ | ||
| UserTokensEnabled: Bool(false), | ||
| } | ||
|
|
||
| org, err := ownerClient.Organizations.Update(ctx, orgTest.Name, options) | ||
| require.NoError(t, err) | ||
| assert.False(t, *org.UserTokensEnabled, "user tokens disabled") | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider including assertions that validate the change had the intended effect, i.e. try and look up some resources and expect an error here. (& the inverse when the setting is off)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for the suggestion, I have added some more assertions! |
||
| // try reading something with the user token client and verify that it fails, where the team token client | ||
| // succeeds | ||
| _, err = client.Organizations.Read(ctx, orgTest.Name) | ||
| assert.Error(t, err) | ||
| assert.ErrorContains(t, err, "unauthorized") | ||
|
|
||
| org, err = ownerClient.Organizations.Read(ctx, orgTest.Name) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, orgTest.Name, org.Name) | ||
| assert.False(t, *org.UserTokensEnabled, "user tokens disabled") | ||
|
|
||
| // re-enable user tokens | ||
| options = OrganizationUpdateOptions{ | ||
| UserTokensEnabled: Bool(true), | ||
| } | ||
| org, err = ownerClient.Organizations.Update(ctx, orgTest.Name, options) | ||
| require.NoError(t, err) | ||
| assert.True(t, *org.UserTokensEnabled, "user tokens re-enabled") | ||
|
|
||
| // try reading with the user token again and verify that it works | ||
| org, err = client.Organizations.Read(ctx, orgTest.Name) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, orgTest.Name, org.Name) | ||
| assert.True(t, *org.UserTokensEnabled, "user tokens re-enabled") | ||
| }) | ||
|
|
||
| t.Run("with valid options", func(t *testing.T) { | ||
| orgTest, orgTestCleanup := createOrganization(t, client) | ||
|
|
||
|
|
||
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.
Why do we use a bool pointer type?
falseandnilare both logically equivalent, correct?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.
Good question: it is because this setting defaults to
true. Making this a bool pointer allows us to distinguish between "this is unspecified on the organization" (ie, because it is an older version of TFE) and "this is explicitly false" (ie, because this is an up-to-date TFE and the user has disabled the setting)That is important because in the provider when we read an organization from an old TFE version, we dont want the the value to appear as
false. That could lead to the provider always showing drift for the org and/or be misleading since the behaviour of the old org is that user tokens are enabled.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.
A similar example would be like
OrganizationScopedfor agent pools