-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Added customizeDiff for a couple of subfields of params field in google_bigquery_data_transfer_config #6784
Added customizeDiff for a couple of subfields of params field in google_bigquery_data_transfer_config #6784
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @melinath, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 93 insertions(+), 1 deletion(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeForwardingRule_update |
assigning to @roaks3 to do a preliminary review |
This PR looks good to me, but two comments:
|
@c2thorn may have context related to this. |
…odules into bigquery-datatransfer-params-customizediff
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 177 insertions(+), 1 deletion(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeForwardingRule_update |
I have added the unit tests for the particular ForceNew behaviour. |
I'm still able to update this field successfully via the provider. I believe this field may have conditional mutability. I've attached my logs at b/251556517 |
…odules into bigquery-datatransfer-params-customizediff
Thanks @c2thorn for the inputs. I've found out that if |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 201 insertions(+), 1 deletion(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease |
…ta_transfer_config
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.
For reference, I did find this line in the docs that appears to confirm the limitation with Cloud Storage (at least for destination_table
):
Unlike individual loads of data from Cloud Storage to BigQuery, for ongoing transfers you need to create the destination table and its schema in advance of setting up the transfer. BigQuery cannot create the table as part of the recurring data transfer process.
I think this is definitely service-specific logic that we prefer not to re-implement in the provider, but I'm unsure of a better way to solve the problem. For that reason, I believe this custom ForceNew logic is our best option, and moving forward we would need to keep an eye out for other data sources that need similar adjustments.
Adding Stephen for follow-up review.
mmv1/templates/terraform/constants/bigquery_data_transfer.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_bigquery_data_transfer_config_test.go
Show resolved
Hide resolved
…odules into bigquery-datatransfer-params-customizediff
…a_transfer_config
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 221 insertions(+), 1 deletion(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccComputeGlobalForwardingRule_globalForwardingRuleInternalExample|TestAccComputeGlobalForwardingRule_externalHttpLbMigBackendCustomHeaderExample|TestAccComputeGlobalForwardingRule_externalTcpProxyLbMigBackendExample|TestAccComputeForwardingRule_forwardingRuleHttpLbExample|TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample|TestAccComputeForwardingRule_update|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample|TestAccComputeGlobalForwardingRule_internalLoadBalancing |
Tests passed during RECORDING mode: All tests passed |
mmv1/third_party/terraform/tests/resource_bigquery_data_transfer_config_test.go
Show resolved
Hide resolved
…odules into bigquery-datatransfer-params-customizediff
…ata_transfer_config
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 240 insertions(+), 1 deletion(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccDataFusionInstance_dataFusionInstanceZoneExample|TestAccDataFusionInstance_dataFusionInstanceEventExample|TestAccDataFusionInstance_dataFusionInstanceFullExample|TestAccLoggingBucketConfigProject_cmekSettings|TestAccVertexAIIndex_updated|TestAccVertexAIIndex_vertexAiIndexStreamingExample|TestAccFirebaserulesRelease_BasicRelease|TestAccVertexAIIndex_vertexAiIndexExample|TestAccDataFusionInstanceIamPolicyGenerated|TestAccDataFusionInstanceIamMemberGenerated|TestAccDataFusionInstanceIamBindingGenerated |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 240 insertions(+), 1 deletion(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccFirebaserulesRelease_BasicRelease|TestAccVertexAIIndex_vertexAiIndexExample|TestAccDataFusionInstanceIamMemberGenerated |
Tests passed during RECORDING mode: All tests passed |
…le_bigquery_data_transfer_config (GoogleCloudPlatform#6784) * Added customize diff for params field in google_bigquery_data_transfer_config * Added test cases for params field for google_bigquery_data_transfer_config * Added unit tests to cover ForceNew behaviour of params field * Added handling for customizeDiff in google_bigquery_data_transfer_config * Added comments for parmasCustomizeDiff function in google_bigquery_data_transfer_config * Added test case for different data_source_id in resource_bigquery_data_transfer_config * Updated test case for different data_source_id in resource_bigquery_data_transfer_config * Updated error message for resource_bigquery_data_transfer_config_test.go * Updated test case for resource_bigquery_data_transfer_config_test.go
…le_bigquery_data_transfer_config (GoogleCloudPlatform#6784) * Added customize diff for params field in google_bigquery_data_transfer_config * Added test cases for params field for google_bigquery_data_transfer_config * Added unit tests to cover ForceNew behaviour of params field * Added handling for customizeDiff in google_bigquery_data_transfer_config * Added comments for parmasCustomizeDiff function in google_bigquery_data_transfer_config * Added test case for different data_source_id in resource_bigquery_data_transfer_config * Updated test case for different data_source_id in resource_bigquery_data_transfer_config * Updated error message for resource_bigquery_data_transfer_config_test.go * Updated test case for resource_bigquery_data_transfer_config_test.go
…le_bigquery_data_transfer_config (GoogleCloudPlatform#6784) * Added customize diff for params field in google_bigquery_data_transfer_config * Added test cases for params field for google_bigquery_data_transfer_config * Added unit tests to cover ForceNew behaviour of params field * Added handling for customizeDiff in google_bigquery_data_transfer_config * Added comments for parmasCustomizeDiff function in google_bigquery_data_transfer_config * Added test case for different data_source_id in resource_bigquery_data_transfer_config * Updated test case for different data_source_id in resource_bigquery_data_transfer_config * Updated error message for resource_bigquery_data_transfer_config_test.go * Updated test case for resource_bigquery_data_transfer_config_test.go
Added customizeDiff for
data_path_template
anddestination_table_name_template
subfields of params field in google_bigquery_data_transfer_configfixes hashicorp/terraform-provider-google#6992
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)