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.

Sign up for GitHub

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

Jump to bottom

Add drift handling #226

Merged
merged 16 commits into from
Mar 9, 2022
Merged

Add drift handling #226

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e3f4b72
add drift handling
samuel-br Feb 16, 2022
79fd80d
add unit test to resource_template
samuel-br Feb 17, 2022
3868f81
fix cloud credentials project assignment
samuel-br Feb 17, 2022
234334f
fix team project assignment
samuel-br Feb 17, 2022
0003acd
delete refrences to project and environment
samuel-br Feb 17, 2022
d1d60de
fix
samuel-br Feb 17, 2022
357054e
fix fmt
samuel-br Feb 17, 2022
85267ae
Merge branch 'main' of https://github.com/env0/terraform-provider-env…
samuel-br Mar 2, 2022
696d15c
add check for new resource
samuel-br Mar 2, 2022
d388c31
Merge branch 'main' of https://github.com/env0/terraform-provider-env…
samuel-br Mar 4, 2022
4649799
Merge branch 'main' of https://github.com/env0/terraform-provider-env…
samuel-br Mar 7, 2022
443bf24
fix team project assignment
samuel-br Mar 8, 2022
b89584f
Merge branch 'main' of https://github.com/env0/terraform-provider-env…
samuel-br Mar 8, 2022
8b20dc4
update
samuel-br Mar 8, 2022
8f87942
Merge branch 'main' of https://github.com/env0/terraform-provider-env…
samuel-br Mar 9, 2022
c2ec119
add logs when drift detected
samuel-br Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions client/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ type Environment struct {
LifespanEndAt string `json:"lifespanEndAt"`
LatestDeploymentLogId string `json:"latestDeploymentLogId"`
LatestDeploymentLog DeploymentLog `json:"latestDeploymentLog"`
IsArchived bool `json:"isArchived"`
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need to use it on the logic somewhere?
just wondering

Copy link
Contributor

Choose a reason for hiding this comment

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

The PR handled also environment resource at start but since it isn't so trivial to re-create an environment on some drift and there's a deeper product decision to make before implementing it, we've decided to ditch it for now and create a separate issue for environment and project resources.
This looks like a leftover tho.

@samuel-br did you create that seperate issue btw?

Copy link
Member

@eranelbaz eranelbaz Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@yaronya #225?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not so sure. I expect to see an issue that includes environment and project resources in its description, but couldn't find it.

If I get that correctly:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, Ill do it later today, but I want to summary this before on channel, cause for now there is three or for types of resources in this contexts of problem

Copy link
Member

@eranelbaz eranelbaz Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@samuel-br maybe just change their title?
Handle drift - general
Handle drift - specific resources
etc

and link them

Copy link
Contributor

Choose a reason for hiding this comment

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

i am confused with all of those issues.
I just asked to make the PR with a good description (copy-paste the issue if needed to the PR).

anyways, if that line of code is related to this PR keep it.
if not - remove it

@samuel-br
cc @eranelbaz

Copy link
Contributor

Choose a reason for hiding this comment

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

I've changed the title for #250 already FYI.
And yes, prefer not having multiple issues with the same name.

Copy link
Contributor

Choose a reason for hiding this comment

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

@samuel-br can you please share on the main issue (i.e #205) what's needed to be done basically and why it differs from the other issues (parsing the error codes from http client layer and etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yaronya @eranelbaz I changed the issues names (@yaronya thanks on #250 ) and add #205 (comment) on the main issue, please take a look on that comment to ensure its clear enough

}

type DeploymentLog struct {
Expand Down
5 changes: 4 additions & 1 deletion env0/resource_cloud_credentials_project_assignment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ func resourceCloudCredentialsProjectAssignmentRead(ctx context.Context, d *schem
}
}
if !found {
return diag.Errorf("could not find cloud credential project assignment.\n project id = %v, cloud credentials id = %v", projectId, credentialId)
if !d.IsNewResource() {
yaronya marked this conversation as resolved.
Show resolved Hide resolved
d.SetId("")
return nil
}
}

d.SetId(getResourceId(credentialId, projectId))
Expand Down
42 changes: 35 additions & 7 deletions env0/resource_cloud_credentials_project_assignment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package env0

import (
"errors"
"github.com/env0/terraform-provider-env0/client"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"regexp"
"testing"

"github.com/env0/terraform-provider-env0/client"
"github.com/golang/mock/gomock"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
)

func TestUnitResourceCloudCredentialsProjectAssignmentResource(t *testing.T) {
Expand All @@ -16,6 +18,11 @@ func TestUnitResourceCloudCredentialsProjectAssignmentResource(t *testing.T) {
CredentialId: "cred-it",
ProjectId: "proj-it",
}

assignmentForDrift := client.CloudCredentialsProjectAssignment{
CredentialId: "cred-it",
ProjectId: "proj-it-update",
}
stepConfig := resourceConfigCreate(resourceType, resourceName, map[string]interface{}{
"credential_id": assignment.CredentialId,
"project_id": assignment.ProjectId,
Expand Down Expand Up @@ -90,20 +97,41 @@ func TestUnitResourceCloudCredentialsProjectAssignmentResource(t *testing.T) {
mock.EXPECT().RemoveCloudCredentialsFromProject(assignment.ProjectId, assignment.CredentialId).Times(1).Return(nil)
})
})
t.Run("Read didnt api didnt return correct stuff", func(t *testing.T) {
t.Run("detect drift", func(t *testing.T) {
yaronya marked this conversation as resolved.
Show resolved Hide resolved
testCase := resource.TestCase{
Steps: []resource.TestStep{
{
Config: stepConfig,
ExpectError: regexp.MustCompile(`(could not find cloud credential project assignment)`),
Config: stepConfig,
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr(accessor, "id", assignment.CredentialId+"|"+assignment.ProjectId),
resource.TestCheckResourceAttr(accessor, "credential_id", assignment.CredentialId),
resource.TestCheckResourceAttr(accessor, "project_id", assignment.ProjectId),
),
},
{
Config: resourceConfigCreate(resourceType, resourceName, map[string]interface{}{
"credential_id": assignmentForDrift.CredentialId,
"project_id": assignmentForDrift.ProjectId,
}),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr(accessor, "id", assignmentForDrift.CredentialId+"|"+assignmentForDrift.ProjectId),
resource.TestCheckResourceAttr(accessor, "credential_id", assignmentForDrift.CredentialId),
resource.TestCheckResourceAttr(accessor, "project_id", assignmentForDrift.ProjectId),
),
},
},
}

runUnitTest(t, testCase, func(mock *client.MockApiClientInterface) {
mock.EXPECT().AssignCloudCredentialsToProject(assignment.ProjectId, assignment.CredentialId).Times(1).Return(assignment, nil)
mock.EXPECT().CloudCredentialIdsInProject(assignment.ProjectId).Times(1).Return([]string{"1", "2"}, nil)
mock.EXPECT().RemoveCloudCredentialsFromProject(assignment.ProjectId, assignment.CredentialId).Times(1).Return(nil)
mock.EXPECT().AssignCloudCredentialsToProject(assignmentForDrift.ProjectId, assignmentForDrift.CredentialId).Times(1).Return(assignmentForDrift, nil)
mock.EXPECT().RemoveCloudCredentialsFromProject(assignmentForDrift.ProjectId, assignmentForDrift.CredentialId).Times(1).Return(nil)
gomock.InOrder(
mock.EXPECT().CloudCredentialIdsInProject(assignment.ProjectId).Times(1).Return([]string{assignment.CredentialId, "1", "2"}, nil),
mock.EXPECT().CloudCredentialIdsInProject(assignment.ProjectId).Times(1).Return([]string{"1", "2"}, nil),
mock.EXPECT().CloudCredentialIdsInProject(assignmentForDrift.ProjectId).Times(1).Return([]string{assignmentForDrift.CredentialId,
"1", "2"}, nil),
)
})
})
}
1 change: 1 addition & 0 deletions env0/resource_environment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ func resourceEnvironmentRead(ctx context.Context, d *schema.ResourceData, meta i
if err != nil {
return diag.Errorf("could not get environment: %v", err)
}

environmentConfigurationVariables, err := apiClient.ConfigurationVariablesByScope(client.ScopeEnvironment, environment.Id)
if err != nil {
return diag.Errorf("could not get environment configuration variables: %v", err)
Expand Down
1 change: 1 addition & 0 deletions env0/resource_environment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,4 +884,5 @@ func TestUnitEnvironmentResource(t *testing.T) {
})

})

}
3 changes: 2 additions & 1 deletion env0/resource_project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@ package env0
import (
"context"
"errors"
"log"

"github.com/env0/terraform-provider-env0/client"
"github.com/google/uuid"
"github.com/hashicorp/go-cty/cty"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"log"
)

func resourceProject() *schema.Resource {
Expand Down
8 changes: 8 additions & 0 deletions env0/resource_team_project_assignment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package env0
import (
"context"
"fmt"

. "github.com/env0/terraform-provider-env0/client"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand Down Expand Up @@ -59,14 +60,21 @@ func resourceTeamProjectAssignmentRead(ctx context.Context, d *schema.ResourceDa
return diag.Errorf("could not get TeamProjectAssignment: %v", err)
}

found := false
for _, assignment := range assignments {
if assignment.Id == id {
d.Set("project_id", assignment.ProjectId)
d.Set("team_id", assignment.TeamId)
d.Set("role", assignment.ProjectRole)
found = true
yaronya marked this conversation as resolved.
Show resolved Hide resolved
break
}
}
if !found {
if !d.IsNewResource() {
d.SetId("")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if !d.IsNewResource() && !found {
		log.Printf("[WARN] Project (%s) not found, removing from state", d.Id())
		d.SetId("")
		return nil
	}

Looks better to me. I took it from S3 bucket provider as example / reference.
Please do the same for others if can.

I am still not sure about this one.
IIUC we should still throw on d.IsNewResource()==true && someErrorOrBadCodeExists.
I don't see any error returned on that case. @yaronya maybe you can revisit my thoughts here?

Copy link
Member

Choose a reason for hiding this comment

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

i don't think we need to throw something, but we do need to return nil here

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, @eranelbaz is correct - we don't need to throw anything. This is what we wanted to avoid in first place.
We should be removing the resource from state.

@samuel-br haven't we previously discussed it and said that we don't really need to check whether IsNewResource because TF doesn't read the resource before create (only before update/delete) so this would always return false?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yaronya i think read is invoked on create too..

Copy link
Member

Choose a reason for hiding this comment

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

read invokes on terraform refresh IIRC

Copy link
Member

Choose a reason for hiding this comment

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

@razbensimon did you mean to fix the if or for the log?

Copy link
Contributor

Choose a reason for hiding this comment

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

the if is good now, just suggested adding logs to the user as I saw in the S3 bucket example.
for your decision @samuel-br

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"drift detected: TerraForm will remove {this resource} from state" is that good wording?

Copy link
Contributor

Choose a reason for hiding this comment

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

"[WARN] Drift Detected: Terraform will remove {this resource} from state"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c2ec119

return nil
}

Expand Down
93 changes: 73 additions & 20 deletions env0/resource_team_project_assignment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package env0

import (
"testing"

"github.com/env0/terraform-provider-env0/client"
"github.com/golang/mock/gomock"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"testing"
)

func TestUnitTeamProjectAssignmentResource(t *testing.T) {
Expand All @@ -17,26 +19,77 @@ func TestUnitTeamProjectAssignmentResource(t *testing.T) {
ProjectRole: client.Admin,
}

testCase := resource.TestCase{
Steps: []resource.TestStep{
{
Config: resourceConfigCreate(resourceType, resourceName, map[string]interface{}{
"team_id": assignment.TeamId,
"project_id": assignment.ProjectId,
"role": assignment.ProjectRole,
}),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr(accessor, "team_id", assignment.TeamId),
resource.TestCheckResourceAttr(accessor, "project_id", assignment.ProjectId),
resource.TestCheckResourceAttr(accessor, "role", string(assignment.ProjectRole)),
),
},
},
updateAsigment := client.TeamProjectAssignment{
Id: "assignmentIdupdate",
TeamId: "teamIdUupdate",
ProjectId: "projectId0",
ProjectRole: client.Admin,
}
t.Run("create", func(t *testing.T) {
testCase := resource.TestCase{
Steps: []resource.TestStep{
{
Config: resourceConfigCreate(resourceType, resourceName, map[string]interface{}{
"team_id": assignment.TeamId,
"project_id": assignment.ProjectId,
"role": assignment.ProjectRole,
}),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr(accessor, "team_id", assignment.TeamId),
resource.TestCheckResourceAttr(accessor, "project_id", assignment.ProjectId),
resource.TestCheckResourceAttr(accessor, "role", string(assignment.ProjectRole)),
),
},
},
}

runUnitTest(t, testCase, func(mock *client.MockApiClientInterface) {
mock.EXPECT().TeamProjectAssignmentCreateOrUpdate(client.TeamProjectAssignmentPayload{TeamId: assignment.TeamId, ProjectId: assignment.ProjectId, ProjectRole: assignment.ProjectRole}).Times(1).Return(assignment, nil)
mock.EXPECT().TeamProjectAssignments(assignment.ProjectId).Times(1).Return([]client.TeamProjectAssignment{assignment}, nil)
mock.EXPECT().TeamProjectAssignmentDelete(assignment.Id).Times(1).Return(nil)
})
})

runUnitTest(t, testCase, func(mock *client.MockApiClientInterface) {
mock.EXPECT().TeamProjectAssignmentCreateOrUpdate(client.TeamProjectAssignmentPayload{TeamId: assignment.TeamId, ProjectId: assignment.ProjectId, ProjectRole: assignment.ProjectRole}).Times(1).Return(assignment, nil)
mock.EXPECT().TeamProjectAssignments(assignment.ProjectId).Times(1).Return([]client.TeamProjectAssignment{assignment}, nil)
mock.EXPECT().TeamProjectAssignmentDelete(assignment.Id).Times(1).Return(nil)
t.Run("detect drift", func(t *testing.T) {
driftTestCase := resource.TestCase{
Steps: []resource.TestStep{
{
Config: resourceConfigCreate(resourceType, resourceName, map[string]interface{}{
"team_id": assignment.TeamId,
"project_id": assignment.ProjectId,
"role": assignment.ProjectRole,
}),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr(accessor, "team_id", assignment.TeamId),
resource.TestCheckResourceAttr(accessor, "project_id", assignment.ProjectId),
resource.TestCheckResourceAttr(accessor, "role", string(assignment.ProjectRole)),
),
},
{
Config: resourceConfigCreate(resourceType, resourceName, map[string]interface{}{
"team_id": updateAsigment.TeamId,
"project_id": assignment.ProjectId,
"role": assignment.ProjectRole,
}),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr(accessor, "team_id", updateAsigment.TeamId),
resource.TestCheckResourceAttr(accessor, "project_id", assignment.ProjectId),
resource.TestCheckResourceAttr(accessor, "role", string(assignment.ProjectRole)),
),
},
},
}

runUnitTest(t, driftTestCase, func(mock *client.MockApiClientInterface) {
mock.EXPECT().TeamProjectAssignmentCreateOrUpdate(client.TeamProjectAssignmentPayload{TeamId: assignment.TeamId, ProjectId: assignment.ProjectId, ProjectRole: assignment.ProjectRole}).Times(1).Return(assignment, nil)
mock.EXPECT().TeamProjectAssignmentCreateOrUpdate(client.TeamProjectAssignmentPayload{TeamId: updateAsigment.TeamId, ProjectId: assignment.ProjectId, ProjectRole: assignment.ProjectRole}).Times(1).Return(updateAsigment, nil)
gomock.InOrder(
mock.EXPECT().TeamProjectAssignments(assignment.ProjectId).Times(1).Return([]client.TeamProjectAssignment{assignment}, nil),
mock.EXPECT().TeamProjectAssignments(assignment.ProjectId).Times(1).Return([]client.TeamProjectAssignment{updateAsigment}, nil),
mock.EXPECT().TeamProjectAssignments(assignment.ProjectId).Times(1).Return([]client.TeamProjectAssignment{updateAsigment}, nil),
)
mock.EXPECT().TeamProjectAssignmentDelete(updateAsigment.Id).Times(1).Return(nil)
})
})

}
28 changes: 27 additions & 1 deletion env0/resource_template_project_assignment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ func TestUnitTemplateProjectAssignmentResource(t *testing.T) {
Id: "tid",
ProjectIds: []string{"pid", "other-id"},
}

driftReturnValues := client.Template{
Id: "tid",
ProjectIds: []string{"other-id"},
}
updateReturnValues := client.Template{
Id: "updatetid",
ProjectIds: []string{"updatepid"},
Expand Down Expand Up @@ -121,4 +124,27 @@ func TestUnitTemplateProjectAssignmentResource(t *testing.T) {
Times(1).Return(client.Template{}, errors.New("error"))
})
})

t.Run("detect drift", func(t *testing.T) {

runUnitTest(t, testCaseforCreate, func(mock *client.MockApiClientInterface) {
mock.EXPECT().AssignTemplateToProject(resourceTemplateAssignment["template_id"].(string), payLoad).
Times(1).Return(returnValues, nil)

mock.EXPECT().AssignTemplateToProject(resourceTemplateAssignmentUpdate["template_id"].(string), updatePayload).
Times(1).Return(updateReturnValues, nil)

mock.EXPECT().RemoveTemplateFromProject(resourceTemplateAssignmentUpdate["template_id"].(string),
resourceTemplateAssignmentUpdate["project_id"].(string)).Times(1).Return(nil)

gomock.InOrder(
mock.EXPECT().Template(resourceTemplateAssignment["template_id"].(string)).Times(1).
Return(returnValues, nil),
mock.EXPECT().Template(resourceTemplateAssignment["template_id"].(string)).Times(1).
Return(driftReturnValues, nil),
mock.EXPECT().Template(resourceTemplateAssignmentUpdate["template_id"].(string)).Times(1).
Return(updateReturnValues, nil),
)
})
})
}
Loading