Introduce tfe_workspace_settings (deprecates execution_mode, agent_pool_id) #1159

brandonc · 2023-12-01T17:19:46Z

Description

Recently, we merged #1137, which added tfe_organization_default_execution mode resource as well as setting_overwrites field on the tfe_workspace resource.

This added quite some complexity to tfe_workspace because of the previous deprecation of operations, the size of the resource in general, and the special handling required for setting_overwrites in the plugin SDK v2. It also brushed up against agent_pool_id, where we were already having challenges with a circular dependency between tfe_workspace and tfe_agent_pool / _allowed_workspaces.

So this change does three things to simplify the resource API:

extracted execution_mode, agent_pool_id, and setting_overwrites from tfe_workspace into a new resource written for the plugin framework, tfe_workspace_settings. This simplified the plan modifiers required by the computed fields, but also created a nice symmetry with organization defaults since these fields in particular are subject to being defined by the organization. More of these kinds of overwrite settings are expected in the future, so I also...
renamed tfe_organization_default_execution_mode to tfe_organization_default_settings.
deprecated agent_pool_id and execution_mode on tfe_workspace

IMPORTANT

Please review commit-wise!

Testing plan

Use the two new resources to default and overwrite workspace execution
...Especially with an agent pool and agent pool allowed workspaces

Output from acceptance tests

$ TESTARGS="-run TestAccTFEWorkspaceSettings" make testacc
=== RUN   TestAccTFEWorkspaceSettings
2023/12/01 10:18:17 [DEBUG] Configuring client for host "tfcdev-5839ba17.ngrok.io"
2023/12/01 10:18:17 [WARN] Unable to read CLI config or credentials file /Users/brandonc/.terraformrc: open /Users/brandonc/.terraformrc: no such file or directory
2023/12/01 10:18:17 [DEBUG] Service discovery for tfcdev-5839ba17.ngrok.io at https://tfcdev-5839ba17.ngrok.io/.well-known/terraform.json
--- PASS: TestAccTFEWorkspaceSettings (21.75s)
=== RUN   TestAccTFEWorkspaceSettingsImport
--- PASS: TestAccTFEWorkspaceSettingsImport (13.23s)
=== RUN   TestAccTFEWorkspaceSettingsImport_ByName
--- PASS: TestAccTFEWorkspaceSettingsImport_ByName (14.17s)
PASS
ok  	github.com/hashicorp/terraform-provider-tfe/internal/provider	49.589s
...

SwiftEngineer

I found some bugs during testing!

That being said, the biggest of the bugs is the one that happens when unsetting the deprecated execution_mode, so maybe it's something you already knew about and decided it is acceptable? The other only happens with outdated versions of TFE (that don't support setting-overwrites).

If you already knew about these issues and deemed them acceptable, please let me know and I'll re-review with that in mind 🙇

internal/provider/resource_tfe_workspace.go

internal/provider/resource_tfe_workspace_settings.go

CHANGELOG.md

This resource is added for two reasons: to break the circular dependency between tfe_workspace agent_pool_id and tfe_agent_pool_allowed_workspaces, as well as create a resource symmetry between tfe_organization_default_execution_mode (will be renamed to tfe_organization_default_settings in a followup commit)

…fault_settings Even though the diff is large, there are no code changes and some minor documentation repairs.

Because organization defaults do not apply to older versions of TFE, the client needs to set this value to the previous default, "remote"

…t-pool-via-resources-with-tfe-provider-0-48

Resolved

sebasslash

LGTM

sebasslash · 2023-12-12T20:56:30Z

website/docs/r/workspace_settings.markdown

+
+# tfe_workspace_settings
+
+Manages or reads execution mode and agent pool settings for a workspace. If [tfe_organization_default_settings](organization_default_settings.html) are used, those settings may be read using a combination of the read-only `overwrites` argument and the setting itself.


💅 We should probably add an example for using overwrites.

Since it's a read-only attribute, all the examples I can think of feel a little contrived. I don't think you'd be interested in this unless you were also reading the execution_mode but needed to know whether or not a default was imposed. I'll leave it out for now but if a clear example emerges I will add it.

Hmm. The description in this paragraph is a little disorienting, mostly just because the whole overwrites scheme is a bit challenging to get a grip on. I might whip up a proposed reword.

nfagerlund · 2023-12-13T21:39:42Z

website/docs/d/workspace.html.markdown

@@ -60,7 +60,7 @@ In addition to all arguments above, the following attributes are exported:
 * `trigger_patterns` - List of [glob patterns](https://developer.hashicorp.com/terraform/cloud-docs/workspaces/settings/vcs#glob-patterns-for-automatic-run-triggering) that describe the files Terraform Cloud monitors for changes. Trigger patterns are always appended to the root directory of the repository.
 * `vcs_repo` - Settings for the workspace's VCS repository.
 * `working_directory` - A relative path that Terraform will execute within.
-* `execution_mode` - Indicates the [execution mode](https://developer.hashicorp.com/terraform/cloud-docs/workspaces/settings#execution-mode) of the workspace. Possible values include `remote`, `local`, or `agent`.
+* `execution_mode` - **Deprecated** Indicates the [execution mode](https://developer.hashicorp.com/terraform/cloud-docs/workspaces/settings#execution-mode) of the workspace. Use the `tfe_workspace_settings` resource instead.


Hmm, this probably isn't called for in the data-source, since that's read-only... right?

I did this because it's not the full picture any longer because execution_mode can be set from the organization default or from the workspace (as an override) tfe_workspace_settings gives access to both pieces of information and contains only optional attributes.

Ah ok, thx for that context — I think I still disagree slightly, but I'll make a case in a docs PR.

nfagerlund · 2023-12-14T01:19:53Z

website/docs/r/organization_default_settings.html.markdown

+  # Ensures this workspace will inherit the org defaults
+  depends_on = [tfe_organization_default_settings.org_default]


(Recording this for posterity, no one needs to respond to this.) I was confused about why this was necessary, because the workspace will just inherit the default anyway if you don't specify an overridden value. But the issue is that the workspace reports the inherited default as the value of its attribute of the same name, so without the ordering, you get needless churn — the workspace first reports it inherited the old default, then on the second run it reports that it inherited the new default as a change to the computed value. So the ordering is important, because it'll give you a stable result on the first apply. 👍🏼

nfagerlund · 2023-12-14T01:23:04Z

website/docs/r/workspace.html.markdown

@@ -9,6 +9,8 @@ description: |-

 Provides a workspace resource.

+~> **NOTE:** Setting the execution mode and agent pool affinity directly on the workspace has been deprecated in favor of using both [tfe_workspace_settings](workspace_settings) and [tfe_organization_default_settings](organization_default_settings). Use caution when unsetting `execution_mode` because the default value `"remote"` is no longer applied when the `execution_mode` is unset.


As a user reading this, it's not clear to me what will happen instead when I unset execution_mode. 🤔 Tho, as someone who's been in here reviewing recently, I think the answer is "it'll revert to the configured org default (whatever that happens to be) rather than a hardcoded default chosen by this provider".

Oh wait, but actually, after reading the code: it won't revert to anything, it'll just leave it what it was previously managed to (until something else changes it). And that's because, we have to do it that way in order to continue supporting the deprecated attributes properly while allowing the new settings resource to take precedence when the deprecated attrs are absent. GOT IT.

brandonc force-pushed the IPL-4893-unable-to-connect-workspace-to-agent-pool-via-resources-with-tfe-provider-0-48 branch 2 times, most recently from 0ca2d9b to 1a6885e Compare December 1, 2023 22:49

brandonc marked this pull request as ready for review December 1, 2023 22:50

brandonc requested a review from a team as a code owner December 1, 2023 22:50

brandonc requested a review from SwiftEngineer December 1, 2023 22:54

brandonc mentioned this pull request Dec 4, 2023

[COMPLIANCE] Add Copyright and License Headers #1162

Closed

SwiftEngineer requested changes Dec 4, 2023

View reviewed changes

internal/provider/resource_tfe_workspace.go Show resolved Hide resolved

internal/provider/resource_tfe_workspace_settings.go Show resolved Hide resolved

internal/provider/resource_tfe_workspace_settings.go Show resolved Hide resolved

sebasslash previously requested changes Dec 5, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

brandonc force-pushed the IPL-4893-unable-to-connect-workspace-to-agent-pool-via-resources-with-tfe-provider-0-48 branch from 1a6885e to 34ab689 Compare December 5, 2023 22:17

brandonc added 8 commits December 6, 2023 13:01

Remove setting_overwrites from tfe_workspace

dd50c7c

Deprecate execution_mode and agent_pool_id on tfe_workspace

730c856

Rename tfe_organization_default_execution_mode -> tfe_organization_de…

71766e5

…fault_settings Even though the diff is large, there are no code changes and some minor documentation repairs.

Update CHANGELOG

04182e5

Document tfe_workspace_settings

8363b46

Ensure support for TFE when unsetting execution_mode

bab158b

Because organization defaults do not apply to older versions of TFE, the client needs to set this value to the previous default, "remote"

Update CHANGELOG.md

8437592

brandonc force-pushed the IPL-4893-unable-to-connect-workspace-to-agent-pool-via-resources-with-tfe-provider-0-48 branch from 83b1f68 to 8437592 Compare December 6, 2023 20:03

SwiftEngineer approved these changes Dec 7, 2023

View reviewed changes

Merge branch 'main' into IPL-4893-unable-to-connect-workspace-to-agen…

138bfe3

…t-pool-via-resources-with-tfe-provider-0-48

SwiftEngineer approved these changes Dec 8, 2023

View reviewed changes

brandonc mentioned this pull request Dec 12, 2023

[COMPLIANCE] Add Copyright and License Headers #1168

Merged

sebasslash approved these changes Dec 12, 2023

View reviewed changes

brandonc merged commit 9aeb9d0 into main Dec 12, 2023
9 checks passed

brandonc deleted the IPL-4893-unable-to-connect-workspace-to-agent-pool-via-resources-with-tfe-provider-0-48 branch December 12, 2023 21:24

nfagerlund reviewed Dec 13, 2023

View reviewed changes

nfagerlund reviewed Dec 14, 2023

View reviewed changes

nfagerlund mentioned this pull request Dec 14, 2023

Docs tweaks for tfe_workspace_settings and friends #1171

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce tfe_workspace_settings (deprecates execution_mode, agent_pool_id) #1159

Introduce tfe_workspace_settings (deprecates execution_mode, agent_pool_id) #1159

brandonc commented Dec 1, 2023 •

edited

Loading

SwiftEngineer left a comment

sebasslash left a comment

sebasslash Dec 12, 2023

brandonc Dec 12, 2023

nfagerlund Dec 14, 2023

nfagerlund Dec 13, 2023

brandonc Dec 13, 2023

nfagerlund Dec 14, 2023

nfagerlund Dec 14, 2023

nfagerlund Dec 14, 2023

nfagerlund Dec 14, 2023


		# tfe_workspace_settings

		Manages or reads execution mode and agent pool settings for a workspace. If [tfe_organization_default_settings](organization_default_settings.html) are used, those settings may be read using a combination of the read-only `overwrites` argument and the setting itself.

		# Ensures this workspace will inherit the org defaults
		depends_on = [tfe_organization_default_settings.org_default]

		@@ -9,6 +9,8 @@ description: \|-

		Provides a workspace resource.

		~> NOTE: Setting the execution mode and agent pool affinity directly on the workspace has been deprecated in favor of using both [tfe_workspace_settings](workspace_settings) and [tfe_organization_default_settings](organization_default_settings). Use caution when unsetting `execution_mode` because the default value `"remote"` is no longer applied when the `execution_mode` is unset.

Introduce tfe_workspace_settings (deprecates execution_mode, agent_pool_id) #1159

Introduce tfe_workspace_settings (deprecates execution_mode, agent_pool_id) #1159

Conversation

brandonc commented Dec 1, 2023 • edited Loading

Description

IMPORTANT

Testing plan

Output from acceptance tests

SwiftEngineer left a comment

Choose a reason for hiding this comment

sebasslash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonc commented Dec 1, 2023 •

edited

Loading