-
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
Ensure that changes are applied for monorepos #81
Conversation
The monorepo fields were not being checked for changes, so if you ran an apply where only file triggers or trigger prefixes changed, it would not persist your data.
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.
LGTM! 🏅
@@ -303,7 +303,8 @@ func resourceTFEWorkspaceUpdate(d *schema.ResourceData, meta interface{}) error | |||
} | |||
|
|||
if d.HasChange("name") || d.HasChange("auto_apply") || d.HasChange("queue_all_runs") || | |||
d.HasChange("terraform_version") || d.HasChange("working_directory") || d.HasChange("vcs_repo") { | |||
d.HasChange("terraform_version") || d.HasChange("working_directory") || d.HasChange("vcs_repo") || | |||
d.HasChange("file_triggers_enabled") || 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.
I wonder if it makes more sense to write this check as if !d.HasChange(“ssh_key_id”)
?
If this method gets called it means something changed, and the only special case seems to be the SSH key stuff.
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 that would prevent updates to any other fields at the same time as SSH key changes, which seems incorrect.
Edit: I do agree that this condition is a footgun, but I can't think of any clear way of avoiding it, other than removing the conditional and always making the PATCH
request.
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.
Ah yes... You are correct on that one 👍🏻I'll silence myself 😉
The monorepo fields were not being checked for changes, so if you ran an apply where only file triggers or trigger prefixes changed, it would not persist your data.
Includes a test which only updates "file triggers enabled"; this test fails without the code change.