Feat: refactor team role assignment #823

TomerHeber · 2024-04-15T14:43:22Z

Solution

Part of #817

All team role assignment resources are now using the same API code.

Created a common API code.
Added unit tests.
Updated resources (3 in total) to use the new API code.
Updated acceptance tests.

TomerHeber · 2024-04-15T14:43:45Z

client/builtin_roles.go

@@ -0,0 +1,12 @@
+package client


moved this to a common location.

TomerHeber · 2024-04-15T14:44:12Z

client/team_role_assignment.go

@@ -0,0 +1,82 @@
+package client


all relevant resources are now using this API code.
Deleted the rest of the files (above).

TomerHeber · 2024-04-15T14:44:24Z

client/team_role_assignment_test.go

@@ -0,0 +1,133 @@
+package client_test


unit test for the new code.

TomerHeber · 2024-04-15T14:44:48Z

env0/resource_team_environment_assignment.go

@@ -86,31 +88,15 @@ func resourceTeamEnvironmentAssignmentRead(ctx context.Context, d *schema.Resour
 	return nil
 }

-func resourceTeamEnvironmentAssignmentUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {


create and update are the same.

razbensimon

Amazing job

client/api_client.go

razbensimon · 2024-04-15T20:37:18Z

client/team_role_assignment.go

+	EnvironmentId  string `json:"environmentId,omitempty"`
+	OrganizationId string `json:"organizationId,omitempty"`
+	ProjectId      string `json:"projectId,omitempty"`


how can you add validation that 1 of them will be sent/exist?

The terraform resource schema's forces correctness for the end-user.
If I add a validation (which I could) it would be used to detect developer bugs, and not end-user errors

I don't mind adding validations. However, it will not add much value (just a little bit).
Let me know - happy to add if you think it's required.

The terraform resource schema's forces correctness for the end-user.

Good enough for me

razbensimon · 2024-04-15T20:39:26Z

client/team_role_assignment_test.go

+	Describe("CreateOrUpdate", func() {
+		Describe("Success", func() {


please add separate tests for each scope level (org, project, env).
see if needed in the whole file.

I added seperate test cases for the Get and Delete.
However, CreateOrUpdate behaves the same irrespective of the type. Therefore, it's a little redundant.
For completeness sake, I'll add it.

razbensimon · 2024-04-15T20:44:13Z

env0/resource_team_environment_assignment.go

+func resourceTeamEnvironmentAssignmentCreateOrUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
+	apiClient := meta.(client.ApiClientInterface)


A general comment, I was triggered by this line.
User Environment Assignment has the same structure and API behind the scenes.
using userId instead of teamId and maybe the path is different (in 3 of the APIs).

We probably should commonize / reuse / sync the structure of your changes to that scope too.
is there anything to sync there?

Yes - we could make an extra step.
Probably need to repeat this whole exercise for users.

right now user assignments are using different APIs (project, environment, organization). Therefore, if they're merged to one API (?), we can do that extra step afterwards.

repeat this whole exercise for users.

currently, only the env level has the new api.
so not urgent, anyways Org and Project will have the old apis for now.

razbensimon · 2024-04-15T20:51:39Z

client/builtin_roles.go

+const (
+	Admin    string = "Admin"
+	Deployer string = "Deployer"
+	Planner  string = "Planner"
+	Viewer   string = "Viewer"
+)
+
+func IsBuiltinProjectRole(role string) bool {
+	return role == Admin || role == Deployer || role == Planner || role == Viewer
+}


those are suitable for both Environment and Project default roles.
for OrganizationRoles we have User, Admin.

why there is no IsBuiltintRole for the other scopes?
are we missing tests?

This is because of the schema used by resourceTeamProjectAssignment:

Schema: map[string]*schema.Schema{ "team_id": { Type: schema.TypeString, Description: "id of the team", Required: true, ForceNew: true, }, "project_id": { Type: schema.TypeString, Description: "id of the project", Required: true, ForceNew: true, }, "role": { Type: schema.TypeString, Description: "the assigned built-in role (Admin, Planner, Viewer, Deployer)", Optional: true, ValidateDiagFunc: ValidateRole, ExactlyOneOf: []string{"custom_role_id", "role"}, }, "custom_role_id": { Type: schema.TypeString, Description: "id of the assigned custom role", Optional: true, ExactlyOneOf: []string{"custom_role_id", "role"}, ValidateDiagFunc: ValidateNotEmptyString, }, }, }

It's the only one using role and custom_role_id - instead of just role_id.
I thought of deprecating these two fields and adding a role_id instead... but haven't done so yet.

What we can do, is add a proper validation for the other resources (organization and environment). (They're documented, but there's no schema level enforcement).

Yeah we need a schema validation for Org and Environment too.

role (or role_id) should be an uuid or one of the builtins (list depends on scope) [then we can deprecate]

each can be in different PR.

choosing role for both - probably will make less breaking change

will add validation.
I'll deprecate the existing two fields.

This will be done in a follow-up PR.

sorry I'm a bit confused, in the platform we don't use role_id at all, is it ok the behaviour here will be different?

WDYM by not using the id at all?
we do for being able to assign a custom role (uuid) or default role (constant string)

razbensimon

Waiting for the test

TomerHeber · 2024-04-15T22:42:59Z

Waiting for the test

Thank you. Will add the additional tests and merge.

sagydr · 2024-04-16T07:06:24Z

client/builtin_roles.go

+const (
+	Admin    string = "Admin"
+	Deployer string = "Deployer"
+	Planner  string = "Planner"
+	Viewer   string = "Viewer"


these are common for both organization/project/environment ?

not at this stage.
Not all are allowed for each of the types.

There will be a follow-up PR that will leverage these.

TomerHeber · 2024-04-16T14:53:32Z

Waiting for the test

@razbensimon

Tests added.
I will merge for now and add additional improvements (as discussed) after I complete work on other more urgent isssues.

Feat: refactor team role assignment

3ebdfc4

TomerHeber self-assigned this Apr 15, 2024

github-actions bot added feature provider api-client labels Apr 15, 2024

TomerHeber commented Apr 15, 2024

View reviewed changes

client/builtin_roles.go

@@ -0,0 +1,12 @@

package client

Copy link

Collaborator Author

TomerHeber Apr 15, 2024

Choose a reason for hiding this comment

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

moved this to a common location.

TomerHeber commented Apr 15, 2024

View reviewed changes

client/team_role_assignment_test.go

@@ -0,0 +1,133 @@

package client_test

Copy link

Collaborator Author

TomerHeber Apr 15, 2024

Choose a reason for hiding this comment

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

unit test for the new code.

TomerHeber commented Apr 15, 2024

View reviewed changes

TomerHeber requested a review from yaronya April 15, 2024 14:45

TomerHeber added the pending final review label Apr 15, 2024

razbensimon reviewed Apr 15, 2024

View reviewed changes

razbensimon added reviewed - awaiting author and removed pending final review labels Apr 15, 2024

razbensimon requested a review from sagydr April 15, 2024 21:02

razbensimon approved these changes Apr 15, 2024

View reviewed changes

github-actions bot added the ready to merge PR approved - can be merged once the PR owner is ready label Apr 15, 2024

sagydr reviewed Apr 16, 2024

View reviewed changes

add more tests

677855e

TomerHeber added pending final review and removed reviewed - awaiting author pending final review labels Apr 16, 2024

TomerHeber merged commit 282d73c into main Apr 16, 2024
5 checks passed

TomerHeber deleted the feat-refactor-team-role-assignment-#817 branch April 16, 2024 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: refactor team role assignment #823

Feat: refactor team role assignment #823

TomerHeber commented Apr 15, 2024 •

edited

Loading

TomerHeber Apr 15, 2024

TomerHeber Apr 15, 2024

TomerHeber Apr 15, 2024

TomerHeber Apr 15, 2024

razbensimon left a comment

razbensimon Apr 15, 2024

TomerHeber Apr 15, 2024

razbensimon Apr 15, 2024

razbensimon Apr 15, 2024

TomerHeber Apr 15, 2024

razbensimon Apr 15, 2024

TomerHeber Apr 15, 2024

razbensimon Apr 15, 2024

razbensimon Apr 15, 2024 •

edited

Loading

TomerHeber Apr 15, 2024

razbensimon Apr 15, 2024 •

edited

Loading

razbensimon Apr 15, 2024

TomerHeber Apr 15, 2024

sagydr Apr 16, 2024

razbensimon Apr 16, 2024 •

edited

Loading

razbensimon left a comment

TomerHeber commented Apr 15, 2024

sagydr Apr 16, 2024

TomerHeber Apr 16, 2024

TomerHeber commented Apr 16, 2024

		Describe("CreateOrUpdate", func() {
		Describe("Success", func() {

		func resourceTeamEnvironmentAssignmentCreateOrUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
		apiClient := meta.(client.ApiClientInterface)

Feat: refactor team role assignment #823

Feat: refactor team role assignment #823

Conversation

TomerHeber commented Apr 15, 2024 • edited Loading

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

razbensimon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

razbensimon Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

razbensimon Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

razbensimon Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

razbensimon left a comment

Choose a reason for hiding this comment

TomerHeber commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomerHeber commented Apr 16, 2024

TomerHeber commented Apr 15, 2024 •

edited

Loading

razbensimon Apr 15, 2024 •

edited

Loading

razbensimon Apr 15, 2024 •

edited

Loading

razbensimon Apr 16, 2024 •

edited

Loading