test:add a scenario test for unset fields in bigquerydataset

[xiaoweim]

This PR adds a scenario test to log the behavior of the BigQueryDataset terraform controller when updating the resource with fields unset

• Terraform finds the existing values of the unset field and maintain the value.

This test will later be used to verify the behavior of direct controller being backward compatible after #3125 is merged.

• Run make ready-pr to ensure this PR is ready for review.
• Perform necessary E2E testing for changed resources.

[xiaoweim]

/assign @jingyih

[xiaoweim]

/assign @acpana

[jingyih]

/lgtm

[xiaoweim]

/assign @maqiuyujoyce

[maqiuyujoyce]

This PR adds a scenario test to log the behavior of the BigQueryDataset terraform controller when updating the resource with fields unset

• Terraform finds the existing values of the unset field and maintain the value.

Unsetting a field in a CR and reconcile it via the TF-based/DCL-based controller is to unmanage the field. @xiaoweim when you mentioned Terraform finds the existing values of the unset field and maintain the value. did you mean the TF lib or the TF-based controller maintains the value? If it is the TF lib (e.g. resource_bigquery_dataset.go having some special logic) then do we know why?

This test will later be used to verify the behavior of direct controller being backward compatible after #3125 is merged.

If it is the TF-based controller maintains the values of the unset fields, then I think those become externally-managed fields. @cheftako @yuwenma is it the correct understanding that we still support backwards compatibility of externally-managed fields feature when we migrate a resource to direct?

[jingyih]

My understanding is that, the purpose of this test is to document the current behavior of the TF-based implementation, ensuring we keep the same behavior in the direct controller.

[maqiuyujoyce]

@jingyih yes, I understand. In that case, does it mean we don't plan to / don't need to support the actual unsetting of those fields (i.e. fields not set in the underlying GCP resource)?

[jingyih]

@jingyih yes, I understand. In that case, does it mean we don't plan to / don't need to support the actual unsetting of those fields (i.e. fields not set in the underlying GCP resource)?

Maybe I am missing some context, but my initial understanding was that this resource is widely used, so we aim to preserve its existing behavior while migrating its implementation to a direct controller. Is there a specific bug or issue with this resource that requires properly unsetting the field?

[maqiuyujoyce]

Is there a specific bug or issue with this resource that requires properly unsetting the field?

I don't recall any myself. Double checking just in case!

[yuwenma]

Maybe I am missing some context, but my initial understanding was that this resource is widely used, so we aim to preserve its existing behavior while migrating its implementation to a direct controller. Is there a specific bug or issue with this resource that requires properly unsetting the field?

@jingyih that's correct! Thanks for the explanation.

@xiaoweim Could you upload both direct and tf-based log so we can do the comparison?

[jingyih]

Maybe I am missing some context, but my initial understanding was that this resource is widely used, so we aim to preserve its existing behavior while migrating its implementation to a direct controller. Is there a specific bug or issue with this resource that requires properly unsetting the field?

@jingyih that's correct! Thanks for the explanation.

@xiaoweim Could you upload both direct and tf-based log so we can do the comparison?

Log for direct controller implementation is pending the go client migration PR #3125.

[xiaoweim]

Maybe I am missing some context, but my initial understanding was that this resource is widely used, so we aim to preserve its existing behavior while migrating its implementation to a direct controller. Is there a specific bug or issue with this resource that requires properly unsetting the field?

@jingyih that's correct! Thanks for the explanation.
@xiaoweim Could you upload both direct and tf-based log so we can do the comparison?

Log for direct controller implementation is pending the go client migration PR #3125.

Yes @yuwenma , Jingyi is right. The direct controller log will be reflected later after PR #3125 is merged.

[maqiuyujoyce]

/approve
/hold

Hold in case @yuwenma has further comments but overall lgtm.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maqiuyujoyce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [maqiuyujoyce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[yuwenma]

Maybe I am missing some context, but my initial understanding was that this resource is widely used, so we aim to preserve its existing behavior while migrating its implementation to a direct controller. Is there a specific bug or issue with this resource that requires properly unsetting the field?

@jingyih that's correct! Thanks for the explanation.
@xiaoweim Could you upload both direct and tf-based log so we can do the comparison?

Log for direct controller implementation is pending the go client migration PR #3125.

Yes @yuwenma , Jingyi is right. The direct controller log will be reflected later after PR #3125 is merged.

That's fine. Can we have #3125 goes in first and then update this PR to include both TF-based and direct log?

[yuwenma]

/hold cancel