Skip to content
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

Add SSOTeamID to Team structs #364

Merged
merged 1 commit into from Mar 24, 2022

Conversation

xlgmokha
Copy link
Contributor

@xlgmokha xlgmokha commented Mar 18, 2022

Description

The teams API is being updated to include a new attribute called
sso-team-id to allow SSO enabled organizations to use unique identifiers from
external systems to control team membership.

Output from tests (HashiCorp employees only)

モ ENABLE_BETA=1 TFE_ADDRESS=https://... TF_ACC=1 go test -v ./... -tags=integration -run TestTeams
=== RUN   TestTeamsList
=== RUN   TestTeamsList/without_list_options
    team_integration_test.go:37: paging not supported yet in API
=== RUN   TestTeamsList/with_list_options
    team_integration_test.go:43: paging not supported yet in API
=== RUN   TestTeamsList/without_a_valid_organization
--- PASS: TestTeamsList (2.35s)
    --- SKIP: TestTeamsList/without_list_options (0.30s)
    --- SKIP: TestTeamsList/with_list_options (0.00s)
    --- PASS: TestTeamsList/without_a_valid_organization (0.00s)
=== RUN   TestTeamsCreate
=== RUN   TestTeamsCreate/with_valid_options
=== RUN   TestTeamsCreate/with_sso-team-id
=== RUN   TestTeamsCreate/when_options_is_missing_name
=== RUN   TestTeamsCreate/when_options_has_an_invalid_organization
--- PASS: TestTeamsCreate (1.47s)
    --- PASS: TestTeamsCreate/with_valid_options (0.40s)
    --- PASS: TestTeamsCreate/with_sso-team-id (0.21s)
    --- PASS: TestTeamsCreate/when_options_is_missing_name (0.00s)
    --- PASS: TestTeamsCreate/when_options_has_an_invalid_organization (0.00s)
=== RUN   TestTeamsRead
=== RUN   TestTeamsRead/when_the_team_exists
=== RUN   TestTeamsRead/when_the_team_exists/visibility_is_returned
=== RUN   TestTeamsRead/when_the_team_exists/permissions_are_properly_decoded
=== RUN   TestTeamsRead/when_the_team_exists/organization_access_is_properly_decoded
=== RUN   TestTeamsRead/when_the_team_exists/SSO_team_id_is_returned
=== RUN   TestTeamsRead/when_the_team_does_not_exist
=== RUN   TestTeamsRead/without_a_valid_team_ID
--- PASS: TestTeamsRead (2.03s)
    --- PASS: TestTeamsRead/when_the_team_exists (0.19s)
        --- PASS: TestTeamsRead/when_the_team_exists/visibility_is_returned (0.00s)
        --- PASS: TestTeamsRead/when_the_team_exists/permissions_are_properly_decoded (0.00s)
        --- PASS: TestTeamsRead/when_the_team_exists/organization_access_is_properly_decoded (0.00s)
        --- PASS: TestTeamsRead/when_the_team_exists/SSO_team_id_is_returned (0.00s)
    --- PASS: TestTeamsRead/when_the_team_does_not_exist (0.16s)
    --- PASS: TestTeamsRead/without_a_valid_team_ID (0.00s)
=== RUN   TestTeamsUpdate
=== RUN   TestTeamsUpdate/with_valid_options
=== RUN   TestTeamsUpdate/when_the_team_does_not_exist
=== RUN   TestTeamsUpdate/without_a_valid_team_ID
--- PASS: TestTeamsUpdate (1.84s)
    --- PASS: TestTeamsUpdate/with_valid_options (0.39s)
    --- PASS: TestTeamsUpdate/when_the_team_does_not_exist (0.16s)
    --- PASS: TestTeamsUpdate/without_a_valid_team_ID (0.00s)
=== RUN   TestTeamsDelete
=== RUN   TestTeamsDelete/with_valid_options
=== RUN   TestTeamsDelete/without_valid_team_ID
--- PASS: TestTeamsDelete (1.39s)
    --- PASS: TestTeamsDelete/with_valid_options (0.35s)
    --- PASS: TestTeamsDelete/without_valid_team_ID (0.00s)
PASS
ok      github.com/hashicorp/go-tfe     9.080s
?       github.com/hashicorp/go-tfe/examples/organizations      [no test files]
?       github.com/hashicorp/go-tfe/examples/workspaces [no test files]
?       github.com/hashicorp/go-tfe/mocks       [no test files]

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 18, 2022

CLA assistant check
All committers have signed the CLA.

@xlgmokha xlgmokha self-assigned this Mar 18, 2022
@@ -49,6 +49,7 @@ type Team struct {
Visibility string `jsonapi:"attr,visibility"`
Permissions *TeamPermissions `jsonapi:"attr,permissions"`
UserCount int `jsonapi:"attr,users-count"`
SSOTeamID *string `jsonapi:"attr,sso-team-id"`
Copy link
Contributor Author

@xlgmokha xlgmokha Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose *string instead of string because SSOTeamID can be nil.

Copy link
Contributor

@sebasslash sebasslash Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good choice!

team_integration_test.go Show resolved Hide resolved
team_integration_test.go Show resolved Hide resolved
@xlgmokha xlgmokha marked this pull request as ready for review Mar 21, 2022
@xlgmokha xlgmokha changed the title Add SSOTeamID to Team struct Add SSOTeamID to Team structs Mar 21, 2022
Copy link
Contributor

@sebasslash sebasslash left a comment

Changes look good! Some minor things mentioned below...

Another thing to note is to squash the test: commits for a cleaner commit history!

@@ -49,6 +49,7 @@ type Team struct {
Visibility string `jsonapi:"attr,visibility"`
Permissions *TeamPermissions `jsonapi:"attr,permissions"`
UserCount int `jsonapi:"attr,users-count"`
SSOTeamID *string `jsonapi:"attr,sso-team-id"`
Copy link
Contributor

@sebasslash sebasslash Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good choice!

team.go Outdated
@@ -117,6 +121,9 @@ type TeamUpdateOptions struct {
// Optional: New name for the team
Name *string `jsonapi:"attr,name,omitempty"`

// Optional: Unique Identifier to control team membership via SAML
SSOTeamID *string `jsonapi:"attr,sso-team-id"`
Copy link
Contributor

@sebasslash sebasslash Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the field is optional add omitempty in the struct tag

@xlgmokha
Copy link
Contributor Author

xlgmokha commented Mar 22, 2022

I squashed the commits and added omitempty where I think it's appropriate.

モ ENABLE_BETA=1 TFE_ADDRESS=https://... TF_ACC=1 go test -v ./... -tags=integration -run TestTeams
=== RUN   TestTeamsList
=== RUN   TestTeamsList/without_list_options
    team_integration_test.go:37: paging not supported yet in API
=== RUN   TestTeamsList/with_list_options
    team_integration_test.go:43: paging not supported yet in API
=== RUN   TestTeamsList/without_a_valid_organization
--- PASS: TestTeamsList (2.13s)
    --- SKIP: TestTeamsList/without_list_options (0.25s)
    --- SKIP: TestTeamsList/with_list_options (0.00s)
    --- PASS: TestTeamsList/without_a_valid_organization (0.00s)
=== RUN   TestTeamsCreate
=== RUN   TestTeamsCreate/with_valid_options
=== RUN   TestTeamsCreate/with_sso-team-id
=== RUN   TestTeamsCreate/when_options_is_missing_name
=== RUN   TestTeamsCreate/when_options_has_an_invalid_organization
--- PASS: TestTeamsCreate (1.48s)
    --- PASS: TestTeamsCreate/with_valid_options (0.40s)
    --- PASS: TestTeamsCreate/with_sso-team-id (0.23s)
    --- PASS: TestTeamsCreate/when_options_is_missing_name (0.00s)
    --- PASS: TestTeamsCreate/when_options_has_an_invalid_organization (0.00s)
=== RUN   TestTeamsRead
=== RUN   TestTeamsRead/when_the_team_exists
=== RUN   TestTeamsRead/when_the_team_exists/visibility_is_returned
=== RUN   TestTeamsRead/when_the_team_exists/permissions_are_properly_decoded
=== RUN   TestTeamsRead/when_the_team_exists/organization_access_is_properly_decoded
=== RUN   TestTeamsRead/when_the_team_exists/SSO_team_id_is_returned
=== RUN   TestTeamsRead/when_the_team_does_not_exist
=== RUN   TestTeamsRead/without_a_valid_team_ID
--- PASS: TestTeamsRead (1.96s)
    --- PASS: TestTeamsRead/when_the_team_exists (0.18s)
        --- PASS: TestTeamsRead/when_the_team_exists/visibility_is_returned (0.00s)
        --- PASS: TestTeamsRead/when_the_team_exists/permissions_are_properly_decoded (0.00s)
        --- PASS: TestTeamsRead/when_the_team_exists/organization_access_is_properly_decoded (0.00s)
        --- PASS: TestTeamsRead/when_the_team_exists/SSO_team_id_is_returned (0.00s)
    --- PASS: TestTeamsRead/when_the_team_does_not_exist (0.16s)
    --- PASS: TestTeamsRead/without_a_valid_team_ID (0.00s)
=== RUN   TestTeamsUpdate
=== RUN   TestTeamsUpdate/with_valid_options
=== RUN   TestTeamsUpdate/when_the_team_does_not_exist
=== RUN   TestTeamsUpdate/without_a_valid_team_ID
--- PASS: TestTeamsUpdate (1.72s)
    --- PASS: TestTeamsUpdate/with_valid_options (0.38s)
    --- PASS: TestTeamsUpdate/when_the_team_does_not_exist (0.18s)
    --- PASS: TestTeamsUpdate/without_a_valid_team_ID (0.00s)
=== RUN   TestTeamsDelete
=== RUN   TestTeamsDelete/with_valid_options
=== RUN   TestTeamsDelete/without_valid_team_ID
--- PASS: TestTeamsDelete (1.31s)
    --- PASS: TestTeamsDelete/with_valid_options (0.34s)
    --- PASS: TestTeamsDelete/without_valid_team_ID (0.00s)
PASS
ok      github.com/hashicorp/go-tfe     (cached)
?       github.com/hashicorp/go-tfe/examples/organizations      [no test files]
?       github.com/hashicorp/go-tfe/examples/workspaces [no test files]
?       github.com/hashicorp/go-tfe/mocks       [no test files]

@xlgmokha xlgmokha requested a review from sebasslash Mar 23, 2022
sebasslash
sebasslash previously approved these changes Mar 23, 2022
Copy link
Contributor

@sebasslash sebasslash left a comment

We're not in Kansas anymore 🔥

@@ -321,7 +346,7 @@ func TestTeamCreateOptions_Marshal(t *testing.T) {
bodyBytes, err := req.BodyBytes()
require.NoError(t, err)

expectedBody := `{"data":{"type":"teams","attributes":{"name":"team name","organization-access":{"manage-policies":true},"visibility":"organization"}}}
expectedBody := `{"data":{"type":"teams","attributes":{"name":"team name","organization-access":{"manage-policies":true},"sso-team-id":null,"visibility":"organization"}}}
Copy link
Contributor Author

@xlgmokha xlgmokha Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change might cause failures when run from CI because the associated feature flag is disabled by default. Should I undo this change?

Copy link
Member

@radditude radditude Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can bring this back once we remove the feature flag and officially un-beta-fy this field!

Copy link
Contributor

@sebasslash sebasslash Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the skipIfBeta(t) helper for tests that require a feature sitting behind a feature flag! I think it still eludes me which feature flags are enabled for CI. Disregard, you are using the helper. You could add it to this test, and to be honest I'm not sure why this test exists (since other tests already indirectly validate [un]marshaling)

Copy link
Member

@radditude radditude left a comment

This looks great to me! Though I will defer to a maintainer for final approval 😆

Copy link
Contributor

@sebasslash sebasslash left a comment

Not from a Jedi! 🔥 Good work

@xlgmokha xlgmokha merged commit c1df079 into hashicorp:main Mar 24, 2022
4 checks passed
@xlgmokha xlgmokha deleted the xlgmokha/team-sso-id branch Mar 24, 2022
@github-actions
Copy link

github-actions bot commented Mar 24, 2022

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants