bigquerydatatransfer: add schedule_options_v2 to google_bigquery_data_transfer_config#17755
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Googlers: For automatic test runs see go/terraform-auto-test-runs. @melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
…_transfer_config Add the scheduleOptionsV2 field, modeling the API's oneof of time_based_schedule, manual_schedule, and event_driven_schedule. The event-driven option lets a transfer run fire on Pub/Sub events. Extend the hand-built update mask so scheduleOptionsV2 changes are sent on update.
c318da9 to
d08c068
Compare
This comment was marked as outdated.
This comment was marked as outdated.
melinath
left a comment
There was a problem hiding this comment.
Thanks for the PR! I haven't thoroughly reviewed this yet but the test failures look like this:
=== RUN TestAccBigqueryDataTransferConfig/service_account
resource_bigquery_data_transfer_config_test.go:485: Step 1/2 error: After applying this test step, the non-refresh plan was not empty.
stdout:
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# google_bigquery_data_transfer_config.query_config will be updated in-place
~ resource "google_bigquery_data_transfer_config" "query_config" {
id = "projects/1067888929963/locations/asia-northeast1/transferConfigs/6a3dd3a7-0000-2f99-bebf-34c7e91f51eb"
name = "projects/1067888929963/locations/asia-northeast1/transferConfigs/6a3dd3a7-0000-2f99-bebf-34c7e91f51eb"
# (12 unchanged attributes hidden)
- schedule_options_v2 {
- time_based_schedule {
- schedule = "every day 00:00" -> null
# (2 unchanged attributes hidden)
}
}
}
Plan: 0 to add, 1 to change, 0 to destroy.
I'd interpret this as: the server probably returns a value if nothing is set in the config. See https://googlecloudplatform.github.io/magic-modules/develop/diffs/ for guidance on resolving this (most likely via default_from_api.)
This is also missing a test for the new pubsub_subscription field.
|
@alien2003, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
…t-driven test Mark scheduleOptionsV2 as default_from_api (Optional+Computed). The API mirrors a legacy schedule/scheduleOptions value into scheduleOptionsV2 on read (and applies a data-source default when nothing is set), so a config that omits the block previously showed a permanent diff. Making it Optional+Computed lets the server value be absorbed instead of diffing. Add a handwritten event_driven_schedule acceptance test covering pubsub_subscription, granting the BigQuery Data Transfer service agent the roles/pubsub.subscriber and roles/serviceusage.serviceUsageConsumer roles that event-driven Cloud Storage transfers require. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regenerated the GA provider and confirmed schedule_options_v2 now generates as Optional + Computed; go vet, go build, Provider().InternalValidate() (TestProvider). I currently don't have a billing-enabled project I can test this on, so I haven't run the acc suite, TestAccBigqueryDataTransferConfig/schedule_options_v2 and .../schedule_options_v2_event_driven still need to be tested |
This comment was marked as outdated.
This comment was marked as outdated.
| return fmt.Sprintf(` | ||
| data "google_project" "project" {} | ||
|
|
||
| resource "google_project_iam_member" "permissions" { |
There was a problem hiding this comment.
We don't want to provision IAM resources in these tests. I see that this isn't unique to this test and you probably copied it from one of the other tests, but we shouldn't be doing it & we should actually remove it from all the tests. The problem is that managing IAM in this test could cause other tests relying on that IAM to fail; there can also be failures caused by propagation delays.
See https://googlecloudplatform.github.io/magic-modules/test/test/#iam-resources (the "Handwritten" tab for information about how to bootstrap IAM permissions; this should probably happen once in the parent test & be taken out of the child test configs.
There was a problem hiding this comment.
removed the shared Data Transfer service-agent bindings, bootstrap them once in the parent TestAccBigqueryDataTransferConfig instead
|
|
||
| params = { | ||
| data_path_template = "${google_storage_bucket.bucket.url}/*.json" | ||
| destination_table_name_template = "my_table" |
There was a problem hiding this comment.
Current test failure is:
Error: Error creating Config: googleapi: Error 400: Cannot find the destination table provided in request: my_table in dataset my_datasetksxzuvyo3b
This is happening when trying to create the google_bigquery_data_transfer_config.
It looks like this test config doesn't have a table defined. You'll also need to have an explicit dependency on it to ensure the creation order is right.
| destination_table_name_template = "my_table" | |
| destination_table_name_template = google_bigquery_table.my_table.table_id |
There was a problem hiding this comment.
added google_bigquery_table.my_table, pointed destination_table_name_template -> google_bigquery_table.my_table.table_id
| # Service agent to be able to pull from the Pub/Sub subscription and to consume | ||
| # the project's services. See | ||
| # https://cloud.google.com/bigquery/docs/event-driven-transfer | ||
| resource "google_project_iam_member" "pubsub_subscriber" { |
There was a problem hiding this comment.
same comment here re: IAM bootstrapping.
There was a problem hiding this comment.
bootrapped once in the parent test, IAM removed from this config
| # The API mirrors a legacy `schedule`/`scheduleOptions` value into | ||
| # scheduleOptionsV2 on read (and applies a data-source default when nothing | ||
| # is set), so without default_from_api a config that omits this block sees a | ||
| # permanent diff. Optional+Computed lets the server value be absorbed. |
There was a problem hiding this comment.
O+C doesn't just save the value; it will also send it on subsequent requests. If both old & new values are sent in an API request, what happens? If that causes problems, this may need a different solution to suppress the diff without setting a value for the fields.
I see that testAccBigqueryDataTransferConfig_scheduledQuery_update does an update test with a scheduled_options block; in theory that should catch if there were any problems but I may want to manually verify just to be safe.
There was a problem hiding this comment.
I switched to ignore_read per the diffs guide. The field wasn't populated on import, so I added it to ImportStateVerifyIgnore in the new tests
| }) | ||
| } | ||
|
|
||
| func testAccBigqueryDataTransferConfig_scheduleOptionsV2_timeBased(t *testing.T) { |
There was a problem hiding this comment.
Please add update steps to the new tests to make sure all updatable fields get exercised: https://googlecloudplatform.github.io/magic-modules/test/test/#add-an-acceptance-test (the Handwritten tab.) TLDR the main thing is to have a second step that has a new config updating the fields, using ConfigPlanChecks to make sure that the resource isn't being replaced.
There was a problem hiding this comment.
added a second step to both tests that updates an in-place field, the schedule for time-based, the Pub/Sub subscription for event-driven to catch any accidental recreation
- Switch schedule_options_v2 from default_from_api to ignore_read. The API echoes the legacy schedule into scheduleOptionsV2 on read, causing a permanent diff. default_from_api (Optional+Computed) would persist that value and re-send it on every update, but scheduleOptionsV2 cannot be sent together with the legacy schedule/scheduleOptions fields, so it would be rejected on a legacy-schedule update. ignore_read suppresses the diff without persisting or re-sending a value. - Fix the event-driven test: create the destination google_bigquery_table (recording failed with "Cannot find the destination table"). - Stop provisioning the shared DTS service-agent IAM in test configs; bootstrap those roles once in the parent test to avoid cross-test races and IAM propagation delays. - Add update steps with ConfigPlanChecks to the time-based and event-driven tests so updatable fields are exercised without recreation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Validated locally: regenerated the GA provider and confirmed |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 59b40e5: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the replaying VCR build log Step 2: Recording Mode
🟢 All tests passed! View the recording VCR build log or the debug logs folder for detailed results. @alien2003, @melinath, @sachinpro VCR tests complete for 59b40e5! |
|
Manual test run to verify scheduleOptionsV2 behavior (you won't have access to this): https://hashicorp.teamcity.com/buildConfiguration/TerraformProviders_GoogleCloud_GOOGLE_BETA_MMUPSTREAMTESTS_GOOGLEBETA_PACKAGE_BIGQUERYDATATRANSFER/688540 |
melinath
left a comment
There was a problem hiding this comment.
looking through the test runs for this resources, I see the following behaviors:
- Setting
scheduleand/orscheduleOptionsin the request results in the API returningscheduleOptionsandscheduleOptionsV2(wherescheduleOptionsV2is the combination of both fields)- if only
scheduleis sent,scheduleOptionswill be returned as an empty object.
- if only
- Setting
scheduleOptionsV2.eventDrivenScheduledoesn't setscheduleorscheduleOptions - BUT setting
scheduleOptionsV2.timeBasedSchedulesets bothscheduleandscheduleOptions
The main point is that scheduleOptionsV2 is returned by the API and therefore shouldn't be marked ignore_read - doing so would be considered a bug (because users will encounter it as such in the future).
To solve the permadiffs, instead use diff suppression that checks what's set in the resource config & suppresses based on that, similar to #17709. For example, if schedule is set in the config, and scheduleOptionsV2.timeBasedSchedule.schedule isn't set in the config, suppress diffs on scheduleOptionsV2.timeBasedSchedule.schedule, and so on.
You'll need to add a test for disable_auto_scheduling=true (which I assume is going to be equivalent to scheduleOptionsV2.manualSchedule); doesn't look like we have one yet.
Something that would help with tests (if it would work): remove the SkipIfVcr calls from all the tests for this resource and instead ensure that the times are relatively static but always in the future - something like: time.Date(time.Now().Year(), 12, 31, 0, 0, 0, 0, time.Now().Location()).AddDate(0, 0, 10).Format(time.RFC3339). That helps avoid needing manual tests like the one above.
|
(Alternatively, you could remove support for |
…sed/manual Address review feedback: scheduleOptionsV2 IS returned by the API, so ignore_read was incorrect (it permanently hid real drift). Instead: - Remove ignore_read and the redundant time_based_schedule / manual_schedule sub-schedules, which duplicate the existing schedule / schedule_options fields. Only event_driven_schedule (the genuinely new capability) is exposed. - Add a decoder that drops the scheduleOptionsV2 the API mirrors back from legacy schedule / schedule_options (timeBasedSchedule / manualSchedule) when it carries no eventDrivenSchedule, avoiding a permadiff while still reading back a real event_driven_schedule normally. - Stop ignoring schedule_options_v2 on import in the event-driven test (it is now readable). - Add a disable_auto_scheduling=true test. - Drop SkipIfVcr across the resource's tests and use static-but-future timestamps so they record/replay deterministically. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@melinath, thank you for the review!
|
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit e51dc1c: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the replaying VCR build log Step 2: Recording Mode
🟢 All tests passed! View the recording VCR build log or the debug logs folder for detailed results. @alien2003, @melinath, @sachinpro VCR tests complete for e51dc1c! |

Fixes hashicorp/terraform-provider-google#24274
google_bigquery_data_transfer_confighad no way to configure event-driven (Pub/Sub) schedules, and more generally no mapping for the API'sscheduleOptionsV2.This adds a
schedule_options_v2block that mirrors the API oneof:time_based_schedule(schedule,start_time,end_time)manual_schedule(empty block; runs only viaStartManualTransferRuns)event_driven_schedule(pubsub_subscription), the option asked for in the issue, supported for the Cloud Storage data sourceImplementation notes:
exactly_one_ofgroup, matching the API.scheduleOptionsV2replaces the legacyschedule/schedule_options, so they aren't meant to be combined. I left that to API-side validation rather than a schemaConflictsWith, sinceConflictsWithacross nested blocks is unreliable in the SDK.pre_update, so I added ascheduleOptionsV2entry there; without it, updates to the block would be dropped.event_driven_scheduleunderschedule_options. Per the v1 discovery document the field belongs toscheduleOptionsV2, which is what this PR uses.Testing:
make provider VERSION=ga PRODUCT=bigquerydatatransfer, thengo build,go vet, andgo testfor the package andProvider().InternalValidate()all pass.schedule_options_v2withtime_based_schedule) modeled on the existing scheduled-query test. I don't have a billing-enabled project so I have not run the acc suite; please run it via CI. The event-driven path needs a configured Cloud Storage source plus Pub/Sub IAM, so its example isexclude_test, consistent with the other examples on this resource.schedule_options_v2and the legacyschedule/schedule_optionsfields (whether the server echoes a value into the legacy fields when V2 is used).Disclosure: drafted with help from Claude Code; the diff and the API mapping (against the v1 discovery document) were reviewed manually.