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

fix(project_user): Recreate project_user if ignore_missing_user #177

Merged
merged 1 commit into from
Nov 19, 2024
Merged

fix(project_user): Recreate project_user if ignore_missing_user #177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 15 additions & 20 deletions pkg/project/resource/resource_project_user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,33 +191,28 @@ func (r *ProjectUserResource) Read(ctx context.Context, req resource.ReadRequest
return
}

updateStateValues := true
if response.StatusCode() == http.StatusNotFound {
if state.IgnoreMissingUser.ValueBool() {
updateStateValues = false
} else {
resp.State.RemoveResource(ctx)
return
}
// on read always ensure the resource is not part of the state if user or project_user are missing
// this will ensure its detected as deleted and re-created on plan/apply
resp.State.RemoveResource(ctx)
return
} else if response.IsError() {
utilfw.UnableToRefreshResourceError(resp, projectError.String())
return
}

if updateStateValues {
state.ID = types.StringValue(fmt.Sprintf("%s:%s", projectKey, user.Name))
state.Name = types.StringValue(user.Name)
state.ProjectKey = types.StringValue(projectKey)
roles, ds := types.SetValueFrom(ctx, types.StringType, user.Roles)
if ds.HasError() {
resp.Diagnostics.Append(ds...)
return
}
state.Roles = roles
state.ID = types.StringValue(fmt.Sprintf("%s:%s", projectKey, user.Name))
state.Name = types.StringValue(user.Name)
state.ProjectKey = types.StringValue(projectKey)
roles, ds := types.SetValueFrom(ctx, types.StringType, user.Roles)
if ds.HasError() {
resp.Diagnostics.Append(ds...)
return
}
state.Roles = roles

if state.IgnoreMissingUser.IsNull() {
state.IgnoreMissingUser = types.BoolValue(false)
}
if state.IgnoreMissingUser.IsNull() {
state.IgnoreMissingUser = types.BoolValue(false)
}

// Save updated data into Terraform state
Expand Down
102 changes: 102 additions & 0 deletions pkg/project/resource/resource_project_user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/go-resty/resty/v2"
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
"github.com/hashicorp/terraform-plugin-testing/plancheck"
acctest "github.com/jfrog/terraform-provider-project/pkg/project/acctest"
project "github.com/jfrog/terraform-provider-project/pkg/project/resource"
"github.com/jfrog/terraform-provider-shared/testutil"
Expand Down Expand Up @@ -335,6 +336,7 @@ func TestAccProjectMember_missing_user_ignored(t *testing.T) {
"username": username,
"email": email,
"roles": `["Developer","Project Admin"]`,
"create_user": false,
}

template := `
Expand All @@ -354,24 +356,124 @@ func TestAccProjectMember_missing_user_ignored(t *testing.T) {
use_project_user_resource = true
}

{{ if .create_user }}

resource "artifactory_managed_user" "{{ .username }}" {
name = "{{ .username }}"
email = "{{ .email }}"
password = "Password1!"
admin = false
}

{{ end }}

resource "project_user" "{{ .username }}" {
project_key = project.{{ .project_name }}.key
name = "{{ .username }}"
roles = {{ .roles }}
ignore_missing_user = true
{{ if .create_user }}
depends_on = [artifactory_managed_user.{{ .username }}]
{{ end }}
}
`

config := util.ExecuteTemplate("TestAccProjectUser", template, params)

updateParams := map[string]interface{}{
"project_name": params["project_name"],
"project_key": params["project_key"],
"username": params["username"],
"email": params["email"],
"roles": params["roles"],
"create_user": true,
}

configUpdated := util.ExecuteTemplate("TestAccProjectUser", template, updateParams)

resource.Test(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(t) },
CheckDestroy: acctest.VerifyDeleted(resourceName, func(id string, request *resty.Request) (*resty.Response, error) {
return verifyProjectUser(username, projectKey, request)
}),
ExternalProviders: map[string]resource.ExternalProvider{
"artifactory": {
Source: "jfrog/artifactory",
},
},
ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories,
Steps: []resource.TestStep{
// attempt create, will not work
{
Config: config,
ConfigPlanChecks: resource.ConfigPlanChecks{
PostApplyPostRefresh: []plancheck.PlanCheck{
// expect create of project user
plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate),
},
},
ExpectNonEmptyPlan: true,
// expect user to be added to state
Check: resource.ComposeTestCheckFunc(
Copy link
Member

Choose a reason for hiding this comment

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

This is the error from the acceptance tests:

=== RUN   TestAccProjectMember_missing_user_ignored
    resource_project_user_test.go:394: Step 1/4 error: After applying this test step, the refresh plan was not empty.
        stdout


        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          + create

        Terraform will perform the following actions:

          # project_user.not_existingsolpd will be created
          + resource "project_user" "not_existingsolpd" {
              + id                  = (known after apply)
              + ignore_missing_user = true
              + name                = "not_existingsolpd"
              + project_key         = "czmmvqeaxp"
              + roles               = [
                  + "Developer",
                  + "Project Admin",
                ]
            }

        Plan: 1 to add, 0 to change, 0 to destroy.
--- FAIL: TestAccProjectMember_missing_user_ignored (2.94s)
FAIL
FAIL	github.com/jfrog/terraform-provider-project/pkg/project/resource	4.160s
FAIL

I think you'll need:

  1. Add ExpectNonEmtpyPlan: true
  2. Remove Check: ... since there'd be no state being saved by the resource.

Copy link
Contributor Author

@Danielku15 Danielku15 Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Add ExpectNonEmtpyPlan: true

Yes, this looks promising according to: https://github.com/hashicorp/terraform-plugin-testing/blob/4a889266a95c9a38734a7c9f6e079b641a8fae53/helper/resource/testing_new_config.go#L358-L376

I updated the PR with annotations on the first 3 steps where the plan should not be empty.

  1. Remove Check: ... since there'd be no state being saved by the resource.

TF will still write the project_user to state in this variant of the fix. My initial attempt to not write a state entry failed. TF will report an error if a TF provider does not create a state entry for a resource. So its either "fail" or "create state entry".

The key change in this PR is that on a read we remove the item from the state if it is missing (causing a create to be detected on plan). Hence the assertions that the state has values as I've seen it on manual tests.

Copy link
Member

Choose a reason for hiding this comment

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

This is the test Steps that pass for me:

Steps: []resource.TestStep{
	// attempt create, will not work
	{
		Config: config,
		ConfigPlanChecks: resource.ConfigPlanChecks{
			PostApplyPostRefresh: []plancheck.PlanCheck{
				// expect create of project user
				plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate),
			},
		},
		ExpectNonEmptyPlan: true,
		// expect user to be added to state
		Check: resource.ComposeTestCheckFunc(
			resource.TestCheckResourceAttr(resourceName, "project_key", fmt.Sprintf("%s", params["project_key"])),
			resource.TestCheckResourceAttr(resourceName, "name", username),
			resource.TestCheckResourceAttr(resourceName, "ignore_missing_user", "true"),
			resource.TestCheckResourceAttr(resourceName, "roles.#", "2"),
			resource.TestCheckResourceAttr(resourceName, "roles.0", "Developer"),
			resource.TestCheckResourceAttr(resourceName, "roles.1", "Project Admin"),
		)},
	// re-attempt create, will still not work
	{
		Config: config,
		ConfigPlanChecks: resource.ConfigPlanChecks{
			PostApplyPostRefresh: []plancheck.PlanCheck{
				// again expect create of project user (refresh will mark project user as missing)
				plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate),
			},
		},
		ExpectNonEmptyPlan: true,
		// expect user to be in the state
		Check: resource.ComposeTestCheckFunc(
			resource.TestCheckResourceAttr(resourceName, "project_key", fmt.Sprintf("%s", params["project_key"])),
			resource.TestCheckResourceAttr(resourceName, "name", username),
			resource.TestCheckResourceAttr(resourceName, "ignore_missing_user", "true"),
			resource.TestCheckResourceAttr(resourceName, "roles.#", "2"),
			resource.TestCheckResourceAttr(resourceName, "roles.0", "Developer"),
			resource.TestCheckResourceAttr(resourceName, "roles.1", "Project Admin"),
		)},
	// re-attempt create with user being added, will work
	{
		Config: configUpdated,
		ConfigPlanChecks: resource.ConfigPlanChecks{
			PreApply: []plancheck.PlanCheck{
				// again expect create of project user (refresh will mark project user as missing)
				plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate),
			},
			PostApplyPostRefresh: []plancheck.PlanCheck{
				// again expect create of project user (refresh will mark project user as missing)
				plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop),
			},
		},
		// expect user to be in the state
		Check: resource.ComposeTestCheckFunc(
			resource.TestCheckResourceAttr(resourceName, "project_key", fmt.Sprintf("%s", params["project_key"])),
			resource.TestCheckResourceAttr(resourceName, "name", username),
			resource.TestCheckResourceAttr(resourceName, "ignore_missing_user", "true"),
			resource.TestCheckResourceAttr(resourceName, "roles.#", "2"),
			resource.TestCheckResourceAttr(resourceName, "roles.0", "Developer"),
			resource.TestCheckResourceAttr(resourceName, "roles.1", "Project Admin"),
		)},
	// now user is there, no action should be performed
	{
		Config: configUpdated,
		ConfigPlanChecks: resource.ConfigPlanChecks{
			PostApplyPostRefresh: []plancheck.PlanCheck{
				// again expect create of project user (refresh will mark project user as missing)
				plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop),
			},
		},
		// expect user to be in the state
		Check: resource.ComposeTestCheckFunc(
			resource.TestCheckResourceAttr(resourceName, "project_key", fmt.Sprintf("%s", params["project_key"])),
			resource.TestCheckResourceAttr(resourceName, "name", username),
			resource.TestCheckResourceAttr(resourceName, "ignore_missing_user", "true"),
			resource.TestCheckResourceAttr(resourceName, "roles.#", "2"),
			resource.TestCheckResourceAttr(resourceName, "roles.0", "Developer"),
			resource.TestCheckResourceAttr(resourceName, "roles.1", "Project Admin"),
		)},
	},
})

Copy link
Member

Choose a reason for hiding this comment

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

I also need to add this to TestCase for step 3 to work:

ExternalProviders: map[string]resource.ExternalProvider{
	"artifactory": {
		Source: "jfrog/artifactory",
	},
},

Copy link
Contributor Author

@Danielku15 Danielku15 Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@alexhung Highly appreciate your help here due to my lack of a test environment. I updated the PR with your proposed changes. The changes make sense and still fit my expectations on how things should work. I'm still fairly new to developing TF providers, a lot of unknowns in the testing framework 😁

resource.TestCheckResourceAttr(resourceName, "project_key", fmt.Sprintf("%s", params["project_key"])),
resource.TestCheckResourceAttr(resourceName, "name", username),
resource.TestCheckResourceAttr(resourceName, "ignore_missing_user", "true"),
resource.TestCheckResourceAttr(resourceName, "roles.#", "2"),
resource.TestCheckResourceAttr(resourceName, "roles.0", "Developer"),
resource.TestCheckResourceAttr(resourceName, "roles.1", "Project Admin"),
)},
// re-attempt create, will still not work
{
Config: config,
ConfigPlanChecks: resource.ConfigPlanChecks{
PostApplyPostRefresh: []plancheck.PlanCheck{
// again expect create of project user (refresh will mark project user as missing)
plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate),
},
},
ExpectNonEmptyPlan: true,
// expect user to be in the state
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "project_key", fmt.Sprintf("%s", params["project_key"])),
resource.TestCheckResourceAttr(resourceName, "name", username),
resource.TestCheckResourceAttr(resourceName, "ignore_missing_user", "true"),
resource.TestCheckResourceAttr(resourceName, "roles.#", "2"),
resource.TestCheckResourceAttr(resourceName, "roles.0", "Developer"),
resource.TestCheckResourceAttr(resourceName, "roles.1", "Project Admin"),
)},
// re-attempt create with user being added, will work
{
Config: configUpdated,
ConfigPlanChecks: resource.ConfigPlanChecks{
PreApply: []plancheck.PlanCheck{
// again expect create of project user (refresh will mark project user as missing)
plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate),
},
PostApplyPostRefresh: []plancheck.PlanCheck{
// again expect create of project user (refresh will mark project user as missing)
plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop),
},
},
// expect user to be in the state
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "project_key", fmt.Sprintf("%s", params["project_key"])),
resource.TestCheckResourceAttr(resourceName, "name", username),
resource.TestCheckResourceAttr(resourceName, "ignore_missing_user", "true"),
resource.TestCheckResourceAttr(resourceName, "roles.#", "2"),
resource.TestCheckResourceAttr(resourceName, "roles.0", "Developer"),
resource.TestCheckResourceAttr(resourceName, "roles.1", "Project Admin"),
)},

// now user is there, no action should be performed
{
Config: configUpdated,
ConfigPlanChecks: resource.ConfigPlanChecks{
PostApplyPostRefresh: []plancheck.PlanCheck{
// again expect create of project user (refresh will mark project user as missing)
plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop),
},
},
// expect user to be in the state
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "project_key", fmt.Sprintf("%s", params["project_key"])),
resource.TestCheckResourceAttr(resourceName, "name", username),
Expand Down
Loading