-
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
Feature/add trigger patterns #502
Conversation
tfe/data_source_workspace_test.go
Outdated
file_triggers_enabled = true | ||
trigger_patterns = ["/modules/**/*", "/**/network/*"] | ||
working_directory = "terraform/test" | ||
} |
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.
Although this technically works as a data source test, it would be better to just only the data source. The way to do this is in the test itself TestAccTFEWorkspaceDataSource_with_trigger_patterns
, to use the go-tfe client to create a workspace with the trigger patterns, then pass that workspace.name and org.name here in this testAccTFEWorkspaceDataSourceConfigWithTriggerPatterns
config to be used as strings in the data source fields (name and organization). Then continue with verifying that the data is the same.
This way you also don't need a depends_on
.
tfe/resource_tfe_workspace.go
Outdated
for _, tp := range tps.([]interface{}) { | ||
if t, ok := tp.(string); ok { | ||
options.TriggerPrefixes = append(options.TriggerPrefixes, t) | ||
if d.HasChange("trigger_prefixes") { |
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 HasChange
is needed in create function?
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.
Yeah, good point, we'd need https://github.com/hashicorp/go-tfe/pull/410/files merged for that
} | ||
} else { | ||
options.TriggerPatterns = []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.
So now we've handled all the potential data conflicts over on the API side?
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.
These tests should prove that since they're hitting the real thing :-)
Yeah on backend we allow for the presence of both trigger-patterns
and trigger-prefixes
at the same time, but both cannot be populated.
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.
just one test comment, otherwise LGTM
@@ -52,6 +52,7 @@ In addition to all arguments above, the following attributes are exported: | |||
* `tag_names` - The names of tags added to this workspace. | |||
* `terraform_version` - The version (or version constraint) of Terraform used for this workspace. | |||
* `trigger_prefixes` - List of repository-root-relative paths which describe all locations to be tracked for changes. | |||
* `trigger_patterns` - List of repository-root-relative GLOB patterns which describe all locations to be tracked for changes. |
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.
We should probably link to a page explaining GLOB patterns being used here.
We also need the GLOB page for the UI changes currently in work, so once we decide what to link I'll add it here as well.
# Conflicts: # go.mod # go.sum # website/docs/r/workspace.html.markdown
@@ -12,6 +12,8 @@ Provides a workspace resource. | |||
|
|||
~> **NOTE:** Using `global_remote_state` or `remote_state_consumer_ids` requires using the provider with Terraform Cloud or an instance of Terraform Enterprise at least as recent as v202104-1. | |||
|
|||
~> **NOTE:** Using `trigger-patterns` requires using the provider with Terraform Cloud. |
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 wonder if we could put this under the actual trigger patterns bullet? I also wonder if it needs to be a note, or if we could incorporate it into the bullet text. The reason why is that I'm thinking it'd be easy for someone to kind of skip right to the available attributes and miss this note. I think it'd be more helpful for users to see the information about its requirements in context:
"trigger_prefixes
- (Optional) List of prefixes that describe the paths Terraform Cloud monitors for changes, in addition to the working directory. Trigger prefixes are always appended to the root directory of the repository. Terraform Cloud or Terraform Enterprise will start a run when files are changed in any directory path matching the provided set of prefixes. Mutually exclusive with trigger-patterns
. Only available for Terraform Cloud."
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 point -> 8a22a37
* `trigger_prefixes` - List of trigger prefixes that describe the paths Terraform Cloud monitors for changes, in addition to the working directory. Trigger prefixes are always appended to the root directory of the repository. | ||
Terraform Cloud or Terraform Enterprise will start a run when files are changed in any directory path matching the provided set of prefixes. | ||
* `trigger_patterns` - List of glob patterns that describe the files Terraform Cloud monitors for changes. Trigger patterns are always appended to the root directory of the repository. |
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.
Do we also need to say here that the prefixes are only available for Terraform Cloud?
Also - and feel free to push back here because I don't want to go against what we typically do in these docs, but do we ever provide links to the documentation where folks can find out more about these? Like. wonder if the patterns section could benefit from a link to that new glob patterns section you're adding onto the actual VCS settings page?
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'll link it once the Atlas documentation is published. This will not be merged for few days at least. Thanks for the suggestion.
Does the changelog need to be updated also? |
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.
Thank you for submitting this PR! I think we want to update the Changelog too.
@annawinkler sure, thanks for the reminder. 7232415 I think I'll have to update it with a link to glob patterns once the Atlas documentation is published |
# Conflicts: # website/docs/r/workspace.html.markdown
Note: this feature is already in GA. It's in TFC UI and API docs |
@omarismail This PR is a little hard for folks to test without testing instructions. Wondering if you could contribute a few bullet points? Things that I've collected: |
Testing Plan
|
For folks who test, make sure you follow the steps to build the provider manually, then update the dev overrides in your
|
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! Thank you for the testing steps. Let's make sure to squash the commits.
This reverts commit 33de91d.
Description
We're introducing a new field to the workspace model. The field is called
trigger-patterns
and it is meant to be another way of defining when to trigger a run based on file changes. It is mutually exclusive withtrigger-prefixes
which is reflect in the provider, as well as on the backend.Remember to:
Update the Change Log
Update the Documentation
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.