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

feat(workspace): add auto destroy attribute to resource and data source #1354

Merged
merged 7 commits into from May 22, 2024

Conversation

notchairmk
Copy link
Member

@notchairmk notchairmk commented May 9, 2024

Description

This change adds auto_destroy_at to Workspace resource and data source schemas. The Workspaces API accepts three possible types of values for auto_destroy_at:

  • a set value
    • e.g. a payload with the shape { "data": { "attributes": { "name": "some-workspace", "auto-destroy-at": "2006-01-02T15:04:05Z" }}}
  • an ignored value
    • e.g. a payload with the shape { "data": { "attributes": { "name": "some-workspace" }}}
  • and an unset value
    • e.g. a payload with the shape { "data": { "attributes": { "name": "some-workspace", "auto-destroy-at": null }}}

Remember to:

Testing plan

(these follow the integration tests added as part of these changes)

  1. create a workspace with auto_destroy_at unset
    resource "tfe_workspace" "foobar" {
      name            = "workspace-test-0"
      organization    = "hashicorp"
    }
    
  2. confirm no diffs for future plans
  3. update the workspace with a set auto_destroy_at
    resource "tfe_workspace" "foobar" {
      name            = "workspace-test-0"
      organization    = "hashicorp"
      auto_destroy_at = "2024-05-31T14:04:00Z"
    }
    
  4. confirm that auto_destroy_at is set in state and that no further diff occurs with future plans
  5. repeat above steps but creating workspace with auto_destroy_at then removing in the update

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 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.

Tested against local TFE, but these auto_destroy_at changes are out in production TFC.

➜  ~/h/terraform-provider-tfe git:(notchairmk/workspace-auto-destroy) TFE_HOSTNAME=<host> TFE_TOKEN=<token> TESTARGS="-run TestAccTFEWorkspace.*AutoDestroyAt" make testacc
TF_ACC=1 TF_LOG_SDK_PROTO=OFF go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEWorkspace.*AutoDestroyAt -timeout 15m
?   	github.com/hashicorp/terraform-provider-tfe	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-tfe/internal/client	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-tfe/internal/logging	(cached) [no tests to run]
?   	github.com/hashicorp/terraform-provider-tfe/internal/provider/validators	[no test files]
?   	github.com/hashicorp/terraform-provider-tfe/version	[no test files]
=== RUN   TestAccTFEWorkspaceDataSource_readAutoDestroyAt
--- PASS: TestAccTFEWorkspaceDataSource_readAutoDestroyAt (10.10s)
=== RUN   TestAccTFEWorkspace_createWithAutoDestroyAt
--- PASS: TestAccTFEWorkspace_createWithAutoDestroyAt (5.61s)
=== RUN   TestAccTFEWorkspace_updateWithAutoDestroyAt
--- PASS: TestAccTFEWorkspace_updateWithAutoDestroyAt (11.59s)
PASS
ok  	github.com/hashicorp/terraform-provider-tfe/internal/provider	27.816s

@notchairmk notchairmk self-assigned this May 9, 2024
@notchairmk notchairmk requested a review from a team as a code owner May 9, 2024 19:07
@notchairmk notchairmk requested a review from a team May 9, 2024 19:07
@notchairmk notchairmk force-pushed the notchairmk/workspace-auto-destroy branch 3 times, most recently from bee7df9 to 17f66fa Compare May 9, 2024 23:51
@notchairmk notchairmk force-pushed the notchairmk/workspace-auto-destroy branch from c87fc41 to 615ea2e Compare May 10, 2024 15:27
Copy link

@emailnitram emailnitram left a comment

Choose a reason for hiding this comment

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

Is it worth documenting that once the destroy plan is run the auto_destroy_at attribute will be deleted there will be a plan diff (drift)?

Copy link

@emailnitram emailnitram left a comment

Choose a reason for hiding this comment

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

tested locally and working as expected

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.

Just a few thought-provoking questions. I'm still trying to wrap my head around the IaC use case for this

@@ -68,6 +68,10 @@ The following arguments are supported:
* `assessments_enabled` - (Optional) Whether to regularly run health assessments such as drift detection on the workspace. Defaults to `false`.
* `auto_apply` - (Optional) Whether to automatically apply changes when a Terraform plan is successful. Defaults to `false`.
* `auto_apply_run_trigger` - (Optional) Whether to automatically apply changes for runs that were created by run triggers from another workspace. Defaults to `false`.
* `auto_destroy_at` - (Optional) A future date/time string at which point all resources in a workspace will be scheduled for deletion. Must be a string in RFC3339 format (e.g. "2100-01-01T00:00:00Z").

~> **NOTE:** `auto_destroy_at` is not intended for workspaces containing production resources. Additionally, the field is partially managed by HCP Terraform so usage with any long-lived workspaces may cause a perpetual diff.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the platform is going to change this value, I think it should be marked as computed in the schema to avoid 'perpetual diff' though I think it may still happen if the config is different. When does HCP Terraform change this value? After the auto destroy comes to pass?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Marking it as computed would at least allow users to manage the workspace with the tfe provider but the auto-destroy-at configuration could be managed in the UI without causing drift in the next plan.

Copy link
Member Author

Choose a reason for hiding this comment

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

"perpetual diff" is sloppy phrasing on my part, because the HCP Terraform API will respect the value set by the provider. but there will be contention because both pieces are effectively managing the workspace. I agree with your other comment that this doesn't make as much sense in an IaC context, however we have discussed this with enterprise teams and they have expressed a desire to control this specifically through the provider.

yeah, HCP Terraform will unset auto_destroy_at after a destroy run occurs.

I did initially have this as a computed attribute but allowing the user to set "meaningful null" values added significantly more complexity (specifically where the user has set the value but then removes auto_destroy_at from the TF config). what's in this PR ended up feeling like the provider SDK's preferred path for allowing for meaningful null values.

Copy link
Member Author

Choose a reason for hiding this comment

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

we will need to make this computed, but the drift we'd be trying to work around is actually intended behavior that will be more explicit once we factor in logic around the duration attribute (update forthcoming here hashicorp/go-tfe#902). changes for the duration attribute will be compatible with the behavior in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it the intention that a tfe_workspace without a auto_destroy_at config should unset any drifted autodestroy setting?

Usually this kind of drift correction is good but in the case of auto_destroy, it seems like terraform wouldn't be the best source of that config.

Methodology:

  1. create workspace using basic config with no auto destroy
  2. set up auto destroy in the UI
  3. terraform apply

Outcome:
terraform plans to unset the auto destroy setting

Screenshot 2024-05-21 at 2 14 26 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it the intention that a tfe_workspace without a auto_destroy_at config should unset any drifted autodestroy setting?

By default, yes. Talked this through with @jondavidjohn and the way we're leaning (Jon correct me if I'm wrong) is how it's currently implemented because it allows the most flexibility for users.

@brandonc your argument makes sense: there are cases where the user may want to ignore changes to this specific field. If that's the case we can recommend that they use an ignore_changes here. My argument is that as a user, there are cases where I would explicitly want to be alerted if somebody set auto destroy on my workspace – in this case I always want to show the diff.

If we specifically ignore the diff within the provider there's no way to override that behavior as a user, but there is more flexibility if we default to showing a diff.

@@ -68,6 +68,10 @@ The following arguments are supported:
* `assessments_enabled` - (Optional) Whether to regularly run health assessments such as drift detection on the workspace. Defaults to `false`.
* `auto_apply` - (Optional) Whether to automatically apply changes when a Terraform plan is successful. Defaults to `false`.
* `auto_apply_run_trigger` - (Optional) Whether to automatically apply changes for runs that were created by run triggers from another workspace. Defaults to `false`.
* `auto_destroy_at` - (Optional) A future date/time string at which point all resources in a workspace will be scheduled for deletion. Must be a string in RFC3339 format (e.g. "2100-01-01T00:00:00Z").
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm trying to imagine how a static date could be useful in the context of config. Likely this data would come from a variable, but aside from that it will be inevitable that they will define a relative time that will end up changing with every plan.

timeadd(timestamp(), "1h")

@notchairmk notchairmk merged commit 5219dbf into main May 22, 2024
6 checks passed
@notchairmk notchairmk deleted the notchairmk/workspace-auto-destroy branch May 22, 2024 18:30
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.

None yet

3 participants