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

Ensure that changes are applied for monorepos #81

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

alisdair
Copy link
Contributor

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.

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.
@ghost ghost added the size/S label Jun 26, 2019
Copy link
Member

@ryanuber ryanuber left a 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") {
Copy link
Contributor

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.

Copy link
Contributor Author

@alisdair alisdair Jun 26, 2019

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.

Copy link
Contributor

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 😉

@alisdair alisdair merged commit b93e66d into master Jun 26, 2019
@alisdair alisdair deleted the alisdair/fix-monorepo-filtering-fields branch June 26, 2019 19:10
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.

5 participants