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

Make dataflow on_delete a virtual field #3567

Merged
merged 5 commits into from
May 27, 2020

Conversation

c2thorn
Copy link
Member

@c2thorn c2thorn commented May 26, 2020

Fixes hashicorp/terraform-provider-google#6301
updating on_delete would either cause the batch job to be destroyed or the stream to be updated-by-replacement. To prevent this, we need to check if on_delete is our only change by update time.

Release Note Template for Downstream PRs (will be copied)

`dataflow`: fixed an issue where `google_dataflow_job` would try to update `max_workers`
`dataflow`: fixed an issue where updating `on_delete` in `google_dataflow_job` would cause the job to be replaced

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 124 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 2 files changed, 124 insertions(+), 1 deletion(-))

@c2thorn c2thorn requested a review from danawillow May 26, 2020 15:54
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 126 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 2 files changed, 126 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 127 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 2 files changed, 127 insertions(+), 1 deletion(-))

if field == "on_delete" {
continue
}
// Each key within a map must be checked for a change
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true for sure? We have other resources with maps that we just do a HasChange on the whole thing for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some digging, turns out that this is just needed for the labels field due to the resourceDataflowJobLabelDiffSuppress. When we decide to suppress a key, the parent field still reports that it has a change. Terraform plan will show no changes, but d.HasChange("labels") = true. I ran into this disparity between the labels parent/keys and incorrectly assumed that it could happen to other maps.

I can't find ways to clear the change during the diff suppress, so I believe the iteration is still needed for labels. I modified the condition and comments to be specific for the labels field.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 128 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 2 files changed, 128 insertions(+), 4 deletions(-))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent google_dataflow_job on_delete from triggering updates
4 participants