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

Add auto_apply_run_trigger attribute to workspace resource and data source #1123

Merged
merged 9 commits into from
Nov 7, 2023

Conversation

nfagerlund
Copy link
Member

@nfagerlund nfagerlund commented Oct 31, 2023

Description

This adds provider support for the new auto-apply-run-trigger workspace attribute. This setting has shipped to the general public on TFC prod, but is not in TFE yet.

Remember to:

Testing plan

  1. Set the attribute to true.
  2. Set the attribute to false.
  3. Leave the attribute blank when creating a workspace (defaults to hard-false, instead of allowing the API to decide for itself).

Something like this:

terraform {
  required_providers {
    tfe = {
      source = "hashicorp/tfe"
      version = "0.49.2" # EXCEPT, DO A BUILD AND SET YOUR DEV OVERRIDES CONFIG
    }
  }
}

provider "tfe" {
  hostname = "app.terraform.io"
  organization = "YOUR ORG"
}

data tfe_project "one-offs" {
    name = "One-offs" # YOUR PROJECT OF CHOICE FOR USELESS WORKSPACES
}

resource tfe_workspace "auto-trigger-test" {
    name = "auto-trigger-test"
    project_id = data.tfe_project.one-offs.id
    auto_apply = false
    auto_apply_run_trigger = true
    description = "Nov 2023 - Just testing provider support for this new attr."
}

External links

Output from acceptance tests

I'm internal, so see CI.

@nfagerlund nfagerlund force-pushed the nf/oct23-auto-run-trigger branch 2 times, most recently from 5ebe4bb to 1c45299 Compare November 1, 2023 23:32
@nfagerlund nfagerlund marked this pull request as ready for review November 3, 2023 00:15
@nfagerlund nfagerlund requested a review from a team as a code owner November 3, 2023 00:15
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.

Everything was great. I'm just requesting changes until go-tfe is released and bumped

@@ -204,6 +205,13 @@ func TestAccTFEWorkspaceDataSource_readProjectIDNonDefault(t *testing.T) {
}

func testAccTFEWorkspaceDataSourceConfig(rInt int) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its OK to specify auto_apply_run_trigger even if the API doesn't support it. Many enterprise installations will not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's cool that this feature was enabled in the test environment

"auto_apply_run_trigger": {
Type: schema.TypeBool,
Optional: true,
Default: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the default needed for some behavior you are trying to elicit? I wonder if optional is enough

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! I was following the existing behavior of auto_apply, since it's basically the exact same setting.

Torn between my user brain saying "unmanaged is fine," my dev brain saying "Optional + Computed always causes weirdness on unset, love 2 avoid that when possible," and both brains saying "just be consistent with the other thing it's basically identical to."

@brandonc
Copy link
Collaborator

brandonc commented Nov 3, 2023

In order to test this, I had to update go-tfe to fbcc54a62e4a6265d8395a477148d72e689ba2e9

Comment on lines 2300 to 2307
func testAccCheckTFEWorkspaceAutoApplyRunTrigger(res, value string) resource.TestCheckFunc {
if enterpriseEnabled() {
return func(s *terraform.State) error {
return nil
}
}
return resource.TestCheckResourceAttr(res, "auto_apply_run_trigger", value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fantastic helper. I'm wondering if we could refactor this method as a generic helper for any attribute we should test TFC-only. This could avoid having to duplicate this logic again or worse, force the parent test to be flagged entirely. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

👀 that's an interesting thought. I'll be going back to this branch anyway to update go-tfe, may as well refactor this to a general helper while I'm in there.

Required for AutoApplyRunTrigger support.
Sometimes you want to test a new attribute that hasn't landed in TFE yet, but
you don't want to skip the entire surrounding test when running against TFE.
Well! here's a wrapper around `TestCheckResourceAttr` that always passes on TFE.
The convoluted stuff here is temporary, and can be simplified once the feature
flag is gone; I wanted to skip testing this attribute on TFE *without* creating
additional test cases.
Upgrading go-tfe pulled in this update for free, but it fixes a user-facing bug
in the provider that deserves mention.
Since this is only used for a single resource attribute at this time, the linter
is being uptight about it. It's meant to vary, we're just not using it elsewhere
yet! Please cool your jets!!
Comment on lines +278 to +285
func testCheckResourceAttrUnlessEnterprise(name, key, value string) resource.TestCheckFunc {
if enterpriseEnabled() {
return func(s *terraform.State) error {
return nil
}
}
return resource.TestCheckResourceAttr(name, key, value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

@nfagerlund nfagerlund dismissed brandonc’s stale review November 7, 2023 22:06

Did the required go-tfe update

@nfagerlund nfagerlund merged commit 0050f18 into main Nov 7, 2023
9 checks passed
@nfagerlund nfagerlund deleted the nf/oct23-auto-run-trigger branch November 7, 2023 22:06
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.

3 participants