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

Remove workspace includes from policy set reads #1080

Merged
merged 2 commits into from
Oct 2, 2023

Conversation

jbonhag
Copy link
Contributor

@jbonhag jbonhag commented Sep 27, 2023

Description

This PR speeds up policy set refreshes by removing the include=workspaces query param from those calls. It takes extra time to serialize the workspace record on the backend and it doesn't seem like any workspace data besides the id is necessary to manage tfe_workspace_policy_set resources in the provider. This is especially noticeable for a policy set attached to a large number of workspaces.

Note that we do still need need the workspace records here, so we can find a workspace associated with the policy set by name:

options := &tfe.PolicySetListOptions{Include: []tfe.PolicySetIncludeOpt{tfe.PolicySetWorkspaces}}

Remember to:

Testing plan

  1. Create a policy set and attach it to a large number of workspaces
  2. Run a simple TF config to create additional workspaces and attach the existing policy set to them
  3. The TF apply should succeed and be faster than provider v0.48.0.

Example: creating some new workspaces and attaching them to an existing policy set:

resource "tfe_workspace" "this" {
  count = 5
  name = "my-workspace-${count.index}"
}

data "tfe_policy_set" "this" {
  name         = "my-policy-set"
}

resource "tfe_workspace_policy_set" "this" {
  count = 5 
  policy_set_id = data.tfe_policy_set.this.id
  workspace_id  = tfe_workspace.this[count.index].id
}

This config can take a very long time to run, depending on how many workspaces this policy set is already attached to. On my local environment with my-policy-set already attached to 1000 workspaces, it took about 1m15s to apply the above config due to the include=workspaces query parameter when refreshing a policy set. Without the include=workspaces query parameter it takes about 10s.

Output from acceptance tests

Please run applicable acceptance tests locally and include the output here. See testing.md to learn how to run acceptance tests.

If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.

$ ❯ ENABLE_BETA=1 TFE_ADMIN_PROVISION_LICENSES_TOKEN=xxx TFE_HOSTNAME=xxx TFE_TOKEN=xxx TESTARGS="-run TestAccTFEWorkspacePolicySet" make testacc
TF_ACC=1 TF_LOG_SDK_PROTO=OFF go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEWorkspacePolicySet -timeout 15m
=== RUN   TestAccTFEWorkspacePolicySetExclusion_basic
--- PASS: TestAccTFEWorkspacePolicySetExclusion_basic (7.01s)
=== RUN   TestAccTFEWorkspacePolicySetExclusion_incorrectImportSyntax
--- PASS: TestAccTFEWorkspacePolicySetExclusion_incorrectImportSyntax (5.80s)
=== RUN   TestAccTFEWorkspacePolicySet_basic
--- PASS: TestAccTFEWorkspacePolicySet_basic (7.29s)
=== RUN   TestAccTFEWorkspacePolicySet_incorrectImportSyntax
--- PASS: TestAccTFEWorkspacePolicySet_incorrectImportSyntax (5.63s)
PASS
ok  	github.com/hashicorp/terraform-provider-tfe/internal/provider	26.028s
?   	github.com/hashicorp/terraform-provider-tfe/version	[no test files]

@jbonhag jbonhag requested a review from a team as a code owner September 27, 2023 20:42
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Nice!

@brandonc brandonc merged commit 0925339 into main Oct 2, 2023
9 checks passed
@brandonc brandonc deleted the jbonhag/remove-workspace-includes branch October 2, 2023 18:18
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.

2 participants