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

[TF-9914] Add auto destroy attribute to workspaces #786

Merged
merged 13 commits into from
Jan 31, 2024

Conversation

notchairmk
Copy link
Member

@notchairmk notchairmk commented Oct 9, 2023

Description

Updates Workspaces to include the AutoDestroyAt attribute. This attribute is settable on create and update.

Adds a Time receiver to the existing type helpers.
Adds an Nullable generic type with custom marshaling to allow updates to attributes which use significant null values, but which also require omitempty behavior.

Testing plan

Updated workspace integration tests

External links

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

PASS TestWorkspacesUpdate/with_valid_options (0.28s)
      --- PASS: TestWorkspacesUpdate/with_valid_options (0.28s)
PASS TestWorkspacesCreate/with_valid_options (0.34s)
      --- PASS: TestWorkspacesCreate/with_valid_options (0.34s)

@notchairmk notchairmk requested a review from a team October 9, 2023 21:04
@notchairmk notchairmk requested a review from a team as a code owner October 9, 2023 21:04
@notchairmk notchairmk force-pushed the notchairmk/add-auto-destroy branch from a7b3d3b to 484fbd7 Compare October 9, 2023 21:41
Copy link
Member

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

I have questions about this one before I can approve it! (And if there's good answers already, I'll probably push to get them into docs for this attribute.)

From a glance at the workspace api docs, it seems this is a field where a string value means auto-destroy is turned on (with the value of the string being the deadline), but a null value means auto-destroy is turned OFF.

I presume this is why the field on the Workspace struct is a *time.Time pointer instead of a value like the other times!

But: how do I turn auto-destroy off for a workspace?

In the CreateOptions / UpdateOptions structs, you've set the field to omitempty for the serializer, so would a nil pointer do nothing? It seems like it might be impossible to turn it off once it's turned on.

I haven't tested this yet! If I'm wrong, please add a new integration test demonstrating turning auto-destroy on and off again.

@notchairmk notchairmk force-pushed the notchairmk/add-auto-destroy branch 2 times, most recently from 71d716d to 770790f Compare October 10, 2023 03:37
@notchairmk
Copy link
Member Author

Removed omitempty from create and update options (probably don't need to remove it from the create options, but I personally don't have strong feelings one way or the other). I'm not sure if the current implementation works against the grain for patterns elsewhere in the repo, so definitely looking for feedback.

@notchairmk notchairmk force-pushed the notchairmk/add-auto-destroy branch from 770790f to 229f357 Compare October 10, 2023 14:12
@notchairmk
Copy link
Member Author

it seems like there are maybe two ways that I can think of to do this (also open to suggestions):

  1. add to existing update options without omitempty. this is the first implementation, but has the side effect of potentially allowing the user to unintentionally unset AutoDestroyAt
  2. create a distinct remove action, allow omitempty on create and update options. this feels like the preferred path.

let me know if there's another route we should take.

to fix linting I'd probably break the Sprintf calls for "organizations/%s/workspaces/%s" into their own func. but will hold off until I hear further

workspace.go Outdated Show resolved Hide resolved
@notchairmk notchairmk marked this pull request as draft December 21, 2023 17:31
@ctrombley ctrombley force-pushed the notchairmk/add-auto-destroy branch 2 times, most recently from b6054e7 to 7804240 Compare January 5, 2024 21:00
@ctrombley ctrombley marked this pull request as ready for review January 5, 2024 21:50
@ctrombley ctrombley self-assigned this Jan 5, 2024
workspace.go Outdated Show resolved Hide resolved
type_helpers.go Outdated Show resolved Hide resolved
type_helpers.go Outdated Show resolved Hide resolved
@ctrombley ctrombley force-pushed the notchairmk/add-auto-destroy branch from 8284226 to bdfd01a Compare January 10, 2024 21:30
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

This pretty much has my approval pending updating the changelog entry. Comments with 💅 are entirely optional.

Really nice work 🔥

CHANGELOG.md Outdated Show resolved Hide resolved
type_helpers.go Outdated Show resolved Hide resolved
type_helpers.go Outdated Show resolved Hide resolved
workspace.go Outdated Show resolved Hide resolved
workspace_integration_test.go Outdated Show resolved Hide resolved
@ctrombley ctrombley force-pushed the notchairmk/add-auto-destroy branch from a95fd40 to 2ce33d0 Compare January 30, 2024 21:18
@ctrombley ctrombley requested a review from sebasslash January 30, 2024 21:26
@ctrombley ctrombley force-pushed the notchairmk/add-auto-destroy branch from 16e37c0 to f6e82ff Compare January 30, 2024 22:59
@ctrombley ctrombley force-pushed the notchairmk/add-auto-destroy branch from 7e461b8 to bf4c87c Compare January 31, 2024 17:38
@ctrombley ctrombley merged commit c90619c into main Jan 31, 2024
10 checks passed
@ctrombley ctrombley deleted the notchairmk/add-auto-destroy branch January 31, 2024 18:05
Copy link

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

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