-
Notifications
You must be signed in to change notification settings - Fork 157
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
Conversation
5ebe4bb
to
1c45299
Compare
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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."
In order to test this, I had to update go-tfe to |
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) | ||
} |
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
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.
f034953
to
960175a
Compare
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.
960175a
to
de30691
Compare
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!!
func testCheckResourceAttrUnlessEnterprise(name, key, value string) resource.TestCheckFunc { | ||
if enterpriseEnabled() { | ||
return func(s *terraform.State) error { | ||
return nil | ||
} | ||
} | ||
return resource.TestCheckResourceAttr(name, key, value) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
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
Something like this:
External links
Output from acceptance tests
I'm internal, so see CI.