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.

Sign up for GitHub

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

Jump to bottom

Fix ability to detect and apply agent_pool_id and execution_mode changes #607

Merged
merged 1 commit into from
Sep 8, 2022
Merged

Fix ability to detect and apply agent_pool_id and execution_mode changes #607

merged 1 commit into from
Sep 8, 2022

Conversation

Uk1288
Copy link
Contributor

@Uk1288 Uk1288 commented Aug 22, 2022 •
edited
Loading

Description

Workspaces can be created using the resource "tfe_workspace" with fields including execution_mode and agent_pool_id. After creation, if the config is updated by removing the execution_mode and agent_pool_id fields, the changes are not detected during an apply and plan fails with error: Error: agent_pool_id must be provided when execution_mode is 'agent'.

See issue: #544

With this fix, changes in execution_mode and agent_pool_id fields are now detected and plans/applies are successful.

Remember to:

Testing plan

  1. Create a workspace with config(Prerequisite: create an org and an agent pool, use the respective names in the config below)
resource "tfe_workspace" "my-workspace" {
  name         = "my-test-workspace"
  organization = "my-org"

  description                   = "hello there"
  global_remote_state           = false
  tag_names                     = null
  working_directory             = null
  structured_run_output_enabled = false

  execution_mode = "agent"
  agent_pool_id  = "apool-my-agent-pool-id"
}
  1. follow with an apply command
  2. change config by removing execution_mode and agent_pool_id,
resource "tfe_workspace" "my-workspace" {
  name         = "my-test-workspace"
  organization = "my-org"

  description                   = "hello there"
  global_remote_state           = false
  tag_names                     = null
  working_directory             = null
  structured_run_output_enabled = false
}
  1. follow with another apply command. Before this PR, changes are not detected and the apply fails. With these PR, changes should be detected and applied successfully

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 TestAccTFEWorkspace" make testacc

...

@Uk1288 Uk1288 requested a review from a team as a code owner August 22, 2022 21:45
@Uk1288 Uk1288 force-pushed the uk1288-fix-plan-failure-with-exec-mode-change branch from 553765e to 225b76c Compare August 22, 2022 21:46
annawinkler
annawinkler reviewed Aug 23, 2022
View reviewed changes
CHANGELOG.md Outdated
@@ -1,4 +1,5 @@
## Unreleased
* r/tfe_workspace: Changes in `agent_pool_id` and `execution_mode` attributes is now detected and applied. ([#607](https://github.com/hashicorp/terraform-provider-tfe/pull/607))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* r/tfe_workspace: Changes in `agent_pool_id` and `execution_mode` attributes is now detected and applied. ([#607](https://github.com/hashicorp/terraform-provider-tfe/pull/607))
* r/tfe_workspace: Changes in `agent_pool_id` and `execution_mode` attributes are now detected and applied. ([#607](https://github.com/hashicorp/terraform-provider-tfe/pull/607))

annawinkler
annawinkler reviewed Aug 23, 2022
View reviewed changes
tfe/resource_tfe_workspace.go Outdated
@@ -85,7 +86,7 @@ func resourceTFEWorkspace() *schema.Resource {
"execution_mode": {
Type: schema.TypeString,
Optional: true,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why this was listed as Computed? Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is most likely based on logic that whenever an execution_mode is not specified, it will be computed and returned. With this changes, the default value is always appended whenever the field is not specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, Computed might still needed for the execution_mode to be updated when it changes as a result of operation field change. I am investigating this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like operations attribute has been marked as deprecated. Would it be easier to reason about if we removed the attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the new changes, I have marked it as deprecated to match the API docs.

@Uk1288 Uk1288 force-pushed the uk1288-fix-plan-failure-with-exec-mode-change branch 2 times, most recently from be89d11 to f12f9eb Compare August 26, 2022 18:43
@Uk1288 Uk1288 force-pushed the uk1288-fix-plan-failure-with-exec-mode-change branch from f12f9eb to caaa10a Compare September 7, 2022 16:13
Uk1288
Uk1288 commented Sep 7, 2022
View reviewed changes
tfe/resource_tfe_workspace_test.go
// this prevents an attempt to destroy the agent pool before dissasociating it from the workspace
func testAccTFEWorkspace_executionModeNull(organization string) string {
return fmt.Sprintf(`
resource "tfe_agent_pool" "foobar" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test was failing during the agent mode unset flow because the first config had the resource "tfe_agent_pool" and the second config did not.The diff causes an attempt to delete the agent pool while applying the second config.
~ Approach 1 is to persist the resource "tfe_agent_pool" in both configs so the only diff action is unsetting the workspace execution mode.
~Approach 2 is to create agent pool external to the config and then use data source to grab the agent pool in the config. This would required using the go client interface.

I have gone with approach 1. Let me know your thoughts...

Copy link
Contributor

Choose a reason for hiding this comment

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

Much, much nicer than approach 2 👍

sebasslash
sebasslash approved these changes Sep 7, 2022
View reviewed changes
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.

LGTM 🔥

Smoke tested ✔️, ran acceptance test locally ✔️

@Uk1288 Uk1288 merged commit 16f2a76 into main Sep 8, 2022
@Uk1288 Uk1288 deleted the uk1288-fix-plan-failure-with-exec-mode-change branch September 8, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annawinkler annawinkler annawinkler left review comments

@sebasslash sebasslash sebasslash approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Uk1288 @annawinkler @sebasslash