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

Adding support for the resource_count attribute on tfe_workspace #682

Closed

Conversation

rhughes1
Copy link

@rhughes1 rhughes1 commented Nov 9, 2022

Description

This PR exposes the resource_count attribute on the tfe_workspace resource. This is to make it easier for users writing a Sentinel policy to prevent workspaces from being deleted prior to deleting all of the resources that belong to the given workspace. This attribute is already exposed as part of the data source call.

Below is a screenshot of the documentation update on the tfe_workspace resource:

image

Testing plan

  1. Create a new workspace using the configuration below
resource "tfe_organization" "test-organization" {
  name  = "my-org-name"
  email = "[email protected]"
}

resource "tfe_workspace" "test" {
  name         = "my-workspace-name"
  organization = tfe_organization.test-organization.name
  tag_names    = ["test", "app"]
}

output "workspace_resource_count" {
  value = tfe_workspace.test.resource_count
}

This should yield a result like this:

$ terraform refresh
...
Outputs:

workspace_resource_count = 0
  1. Inside the new workspace (ie my-workspace-name), use the following configuration (or similar) to add resources under the control of the workspace:
resource "tfe_team" "test" {
  name         = "my-team-name"
  organization = "my-org-name"
}
  1. Back on the original workspace, re-run a terraform refresh so we can now see that the resource_count has changed from 0 to 1:
$ terraform refresh
...
Outputs:

workspace_resource_count = 1

External links

Output from acceptance tests

N/A at this point in time.

@rhughes1 rhughes1 requested a review from a team as a code owner November 9, 2022 03:35
@hashicorp-cla
Copy link

hashicorp-cla commented Nov 9, 2022

CLA assistant check
All committers have signed the CLA.

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.

@rhughes1 Hello! And thanks for the contribution. I completely get the sentiment and know how easy it is to accidentally delete a workspace that has resources under management and I don't have a problem with adding resource_count to tfe_workspace.

I also want to point out that a policy may not be required if you use Terraform Cloud because we are actually rolling out a platform-wide safeguard with this problem in mind, known as safe delete / force delete that is making its way into the provider as a new attribute that you must add to workspace before it can be deleted when resources are still under management.

Hopefully this could also solve your problem without additional policy code. I'll give this a smoke test but it looks pretty good.

@brandonc
Copy link
Collaborator

brandonc commented Nov 14, 2022

@rhughes1 After more investigation, there seems to be a lot fields that are present in data.tfe_workspace that are not present in the tfe_workspace resource. I wonder if a solution like this could solve your problem while we evaluate if/how the tfe maintainers should should address the larger disparity?

resource "tfe_workspace" "test" {
  name         = "my-workspace-name"
  organization = tfe_organization.test-organization.name
  tag_names    = ["test", "app"]
}

data "tfe_workspace" "test" {
  name         = "my-workspace-name"
  organization = tfe_organization.test-organization.name
  depends_on = [tfe_workspace.test]
}

output "workspace_resource_count" {
  value = data.tfe_workspace.test.resource_count
}

@rhughes1
Copy link
Author

rhughes1 commented Nov 14, 2022

@brandonc So that solution you mentioned would indeed work. However, from a Terraform best practice approach I would think it's a little bit odd having a data source and a resource within the same root module for the same resource (Not to mention using a depends_on block).

I think there still might be value in exposing the resource_count for the Terraform workspace.

@brandonc
Copy link
Collaborator

I would say it's not ideal to have a data source that references a resource and for what it's worth I just realized you could omit the depends_on argument by specifying the name dependency directly:

resource "tfe_workspace" "test" {
  name         = "my-workspace-name"
  organization = tfe_organization.test-organization.name
  tag_names    = ["test", "app"]
}

data "tfe_workspace" "test" {
  name         = tfe_workspace.test.name
  organization = tfe_organization.test-organization.name
}

output "workspace_resource_count" {
  value = data.tfe_workspace.test.resource_count
}

I think we will unify the tfe_workspace resource/data export-only arguments but might be willing to move forward in adding just resource_count for now. Unfortunately, I can't merge this PR because the tests are failing because the resource_count is not always 0. I will take another look at it this week or next unless you get to it first. Thanks!

@annawinkler
Copy link
Contributor

👋 Hi @rhughes1 - could you please update your fork with the latest? We'll also be looking at the test failure to see if we can suggest an update to fix the problem. ✨

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

FEATURES:
* r/tfe_organization: Add `allow_force_delete_workspaces` attribute to set whether admins are permitted to delete workspaces with resource under management. ([#661](https://github.com/hashicorp/terraform-provider-tfe/pull/661))
* r/tfe_workspace: Add attribute `resource_count`
Copy link
Contributor

@annawinkler annawinkler Dec 7, 2022

Choose a reason for hiding this comment

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

💅 A small suggestion to update the changelog entry to be consistent with the format of the file and include a link to the PR:

Suggested change
* r/tfe_workspace: Add attribute `resource_count`
* r/tfe_workspace: Add attribute `resource_count` to `tfe_workspace` ([#682](https://github.com/hashicorp/terraform-provider-tfe/pull/682))

@rhughes1
Copy link
Author

rhughes1 commented Dec 8, 2022

👋 Hi @rhughes1 - could you please update your fork with the latest? We'll also be looking at the test failure to see if we can suggest an update to fix the problem. ✨

@annawinkler I updated my fork with the latest changes. Let me know how else I can help move this along :)

@annawinkler
Copy link
Contributor

Merged #712

@annawinkler annawinkler closed this Dec 8, 2022
@annawinkler
Copy link
Contributor

Thanks @rhughes1 ! I updated the code in a separate branch to also confirm the tests were running OK too. Appreciate the contribution! 💖

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.

4 participants