-
Notifications
You must be signed in to change notification settings - Fork 156
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 Global Run Task support #1330
Conversation
4e0a344
to
0363540
Compare
3ac88ce
to
4c09986
Compare
Added compliance team for review as a once-over. |
4c09986
to
877322f
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.
The code looks healthy, but let's start with a high level question 👇
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.
Can you help me understand why this schema was not added to organization_run_task?
In the case of tfe_organization_default_settings, it's very common to not manage the organization itself with the provider... and in the case of tfe_workspace_settings, the primary issue was breaking the circular dependency between agent pools and workspaces but it was also a convenient symmetry with organization_settings because it overrides 'inheritability' of those settings.
In the case of organization run tasks, neither problem seems to appear so it seems like it would be much straightforward to put all schema within the same resource. Is there a concept I am overlooking?
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.
Another concern is "enabled" schema would appear to be managing the same thing on both resources
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.
So there a couple of things that forced me down this path;
-
Global Run Tasks are entitlement based.
So it's possible for their functionality to be removed/added outside of the provider.- Imagine the run task resource includes global information in it's state.
- A run task resource is created without global run tasks being available.
- An admin comes along and upgrades their TFC subscription to premium
- Now the next run the resource will be out of sync because its state has changed. (from null to something)
Same applies when a resource wants to manage a global run task, but the subscription was demoted. (from something to null).
- We are still using Plugin Protocol v5 so I don't have have access to nested attributes. I tried so many different combinations of hacks but I could never get it quite right.
I originally did have it all in one resource ala;
org_run_task "thing" {
...
global = {
enabled = true
enforcement-level = "advisory"
}
}
And it worked perfectly, until I started testing changing the subscription, and everything broke.
I desperately wanted to use a nested block syntax, but I just couldn't, given the dynamic nature of the subscription turning on/off the global run task feature.
website/docs/d/organization_run_task_global_settings.html.markdown
Outdated
Show resolved
Hide resolved
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.
Looks good! 👌
resource "tfe_organization_run_task_global_settings" "example" { | ||
task_id = tfe_organization_run_task.example.id | ||
enabled = false | ||
enforcement_level = "advisory" | ||
stages = ["pre_plan", "post_plan"] | ||
} |
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.
Can you provide the whole example where tfe_organization_run_task.example
is included as well?
Does enabled
need to be an argument in both resources?
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.
Added the missing example, and set enabled to true in both to avoid confusion.
Does enabled need to be an argument in both resources?
Yes.
877322f
to
6b75c41
Compare
6b75c41
to
072749e
Compare
137257a
to
9488a04
Compare
9488a04
to
6be2fb8
Compare
@brandonc or @ctrombley Am I ok to merge? |
This commit; * Updates the workspace run task resource and datasource to deprecate the `stage` attribute in favour of the new `stages` attribute. This also includes a state version update * Adds a new organization_run_task_global_settings resource and datasource for managing the global settings of a Run task. This does require the HCP Terraform/TFE instance to support global run tasks. The provider does attempt to error gracefully if a user is trying to use this feature if it's unsupported.
This commit updates the documentation and changelog for the Global Run Tasks feature.
6be2fb8
to
f094806
Compare
Rebased due to changelog |
Description
This PR;
stage
attributein favour of the new
stages
attribute. This also includes a state version updateglobal settings of a Run task. This does require the HCP Terraform/TFE instance to support
global run tasks. The provider does attempt to error gracefully if a user is trying to use
this feature if it's unsupported.
Remember to:
Testing plan
Handled by acceptance tests
External links
Output from acceptance tests
In this PR.