update isEmptyValue function

[danawillow]

Added IsValid check, which handles nil objects , and updated our zero-checking logic to use the one built into go in 1.13. This fixes an issue where we were sending null to the API for all nested objects instead of omitting them from the request if they were unset.

The logic is slightly different than in our version, and the only way to really find out if it breaks anything is by running tests- I caught one already and I'll catch any others post-submit in our CI run. Conveniently, this affects fields that aren't set, so the only way we wouldn't notice it for a given resource would be if all of its tests set an Optional block (which is rare; usually we have at least one test that only sets Required fields)

@drebes fyi

Release Note Template for Downstream PRs (will be copied)

all: fixed issue where nested objects were getting sent as null values to GCP on create instead of being omitted from requests

[modular-magician]

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 7f2863c.

Pull request statuses

No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1575
depends: GoogleCloudPlatform/terraform-google-conversion#307
depends: hashicorp/terraform-provider-google#5297

[modular-magician]

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, d3e7297.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

[rileykarson]

The (wrong) behaviour is load bearing for some resources, or at least was. I'd had a PR ~ a year ago that modified this behaviour in a different way (removing the check I think), it ended up breaking at least one update test. I think it was in Firewall.

Obviously it shouldn't be load bearing, so it's probably worth running a full test gauntlet on the TPGB downstream to make sure we catch as many breakages as we can.

[danawillow]

Yup, I thought about doing it yesterday but it would have finished after I left anyway, so I was just going to wait for the CI run and then check the output there. Now that it's the AM though I can start one up.

[emilymye]

Maybe needs a release note? Am fine with doing this and fixing breakages as we see them.

[modular-magician]

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 511f563.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

[danawillow]

Can I get another review on this since I made some changes after the test run?

[drebes]

Can this be merged? I'm working on a feature that depends on this...

[modular-magician]

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

Diff report:

Terraform GA: Diff ( 7 files changed, 10 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 8 files changed, 11 insertions(+), 7 deletions(-))
TF Conversion: Diff ( 1 file changed, 4 insertions(+))