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 ability to attach a workspace to an existing policy set #591

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

Uk1288
Copy link
Contributor

@Uk1288 Uk1288 commented Aug 9, 2022

Description

Currently the lifecycle of the tfe_policy_set requires that all tfe_workspace resources are created first and then assigned in one call to the tfe_policy_set during creation of the policy. For example:

resource "tfe_workspace" "test_ws" {
  name         = "test_ws"
  organization = "my-org"
  tag_names    = ["test", "app"]
}

resource "tfe_policy_set" "test_policy_set" {
  name          = "my-policy-set"
  description   = "A new policy set"
  organization  = "my-org"
  workspace_ids = [tfe_workspace.test_ws.id]
}

Consider the case where tfe_workspace resources are spread across multiple repositories. Where each team or repo wants to be able to create & attach workspaces to existing policy sets. For example,

team-security is in charge of creating policies and own security-repo having:

resource "tfe_policy_set" "test_policy_set" {
  name          = "my-policy-set"
  description   = "A new policy set"
  organization  = "my-org"
  workspace_ids = []
}

team-dev wants to create its workspaces in dev-repo but attach it to existing policies above:

resource "tfe_workspace" "test_ws" {
  name         = "test_ws"
  organization = "my-org"
  tag_names    = ["test", "app"]
}

***no way to attach to existing policy set except by duplicating the policy set creation

User pain point: This leads to each repo re-creating the same policy unnecessarily.

This PR adds tfe_workspace_policy_set which allows workspaces to be attached to existing policy sets.

resource "tfe_workspace" "test_ws" {
  name         = "test_ws"
  organization = "my-org"
  tag_names    = ["test", "app"]
}


resource "tfe_workspace_policy_set" "attachable_policy" {
  policy_set_id = data.tfe_policy_set.test_policy_set.id
  workspace_id = tfe_workspace.test_ws.id
}

Note:
Since workspaces can be added across multiple repos to the same policy set, this means the policy-set state can be altered by various configs(repos). Similar behaviour with tfe_workspace_variable_set

Remember to:

Testing plan

  1. Create a workspace and a policy set in a config
  2. Create a second config, created another workspace, and attempt to attach this new workspace to the existing policy set from previous config
  3. There is no way to attach new workspace to existing policy set,
  4. After PR changes, attachment can be created with:
resource "tfe_workspace" "test_ws" {
  name         = "test_ws"
  organization = "my-org"
  tag_names    = ["test", "app"]
}

data "tfe_policy_set" "test_policy_set" {
  name = "my-policy-set"
  organization = "my-org"
}

resource "tfe_workspace_policy_set" "attachable_policy" {
  policy_set_id = data.tfe_policy_set.test_policy_set.id
  workspace_id = tfe_workspace.test_ws.id
}

External links

Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.

Output from acceptance tests

Please run applicable acceptance tests locally and include the output here. See TESTS.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.

$ TESTARGS="-run TestAccTFEWorkspacePolicySet" make testacc

...

Screen Shot 2022-08-18 at 10 58 30 AM

Screen Shot 2022-08-18 at 10 58 37 AM

@Uk1288 Uk1288 requested a review from a team as a code owner August 9, 2022 11:36
@annawinkler
Copy link
Contributor

Could you clarify the note in the PR description:

Since workspaces can be added across multiple repos to the same policy set, this means the state can be changed outside a particular config and however, the config should re-sync after an apply. Similar behaviour with tfe_workspace_variable_set

I don't think I understand the implication of what is being said here. 🤔

@Uk1288
Copy link
Contributor Author

Uk1288 commented Aug 9, 2022

Could you clarify the note in the PR description:

Since workspaces can be added across multiple repos to the same policy set, this means the state can be changed outside a particular config and however, the config should re-sync after an apply. Similar behaviour with tfe_workspace_variable_set

I don't think I understand the implication of what is being said here. 🤔

I have added a more detailed explanation. Take a look and let me know if it makes more sense.

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.

When I re-apply a config containing a tfe_policy_set and a tfe_workspace_policy_set, the workspace_ids field is updated in-place, even if it does not appear in config

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # tfe_policy_set.test2 will be updated in-place
  ~ resource "tfe_policy_set" "test2" {
        id            = "polset-qBcEd8WAtFcxTKo6"
        name          = "test-policy-set-2"
      ~ workspace_ids = [
          - "ws-T2kxkEtyTGCdsKXX",
        ]
        # (4 unchanged attributes hidden)
    }

I think this can be fixed by adding Computed: true to this field.

You should also add documentation to this argument to say that the recommended way is to use the tfe_workspace_policy_set resource instead and those two cannot be used simultaneously. I think we did the same thing with tfe_variable_set?

}

resource "tfe_policy_set" "test" {
name = "Test Policy Set"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an invalid value for name-- can only include letters, numbers, -, and _

@Uk1288 Uk1288 force-pushed the uk1288-attach-ws-to-existing-policy-set branch 3 times, most recently from 7c224b6 to b634d7f Compare August 22, 2022 18:11
@Uk1288 Uk1288 force-pushed the uk1288-attach-ws-to-existing-policy-set branch from b634d7f to b38c264 Compare August 22, 2022 18:46
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.

make fmtcheck failed on this code, but that is curious because the linter passed. I think we should probably go ahead and add fmtcheck to the github action!

Everything else looks good

@Uk1288
Copy link
Contributor Author

Uk1288 commented Aug 23, 2022

That's interesting, I just did a make fmtcheck on this branch and no errors. What errors are you seeing?

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.

I think the disparity is my local go version

@Uk1288 Uk1288 merged commit 93ae164 into main Aug 23, 2022
@Uk1288 Uk1288 deleted the uk1288-attach-ws-to-existing-policy-set branch August 23, 2022 18:17
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.

3 participants