-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
add event_stream block to google_storage_transfer_job schema #8894
add event_stream block to google_storage_transfer_job schema #8894
Conversation
Hello! I am a robot. It looks like you are a: Community Contributor @SarahFrench, 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. |
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 ( 1 file changed, 29 insertions(+)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_storage_transfer_job" "primary" {
event_stream {
event_stream_expiration_time = # value needed
event_stream_start_time = # value needed
name = # value needed
}
}
|
@BBBmau I kicked off the automated tests, though I'm conscious that this is still a draft so won't review until you ask/mark as ready! Also, take a look at the guidance about writing release notes - if you were a community contributor I would edit your release note for you, but I'd like you to have a go getting the release note to match our style guide as practice. |
Tests analyticsTotal tests: Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccStorageTransferJob_omitScheduleEndDate|TestAccStorageTransferJob_basic|TestAccStorageTransferJob_posixSink|TestAccStorageTransferJob_objectConditions|TestAccStorageTransferJob_notificationConfig|TestAccStorageTransferJob_posixSource|TestAccStorageTransferJob_transferOptions |
|
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.
Quick fly-by review while we wait for the tests to run, I'll give more feedback once they finish!
mmv1/third_party/terraform/services/storagetransfer/resource_storage_transfer_job.go
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/storagetransfer/resource_storage_transfer_job.go
Show resolved
Hide resolved
Tests analyticsTotal tests: Action takenFound 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccGKEHub2Scope_gkehubScopeBasicExample|TestAccGKEHub2ScopeIamBindingGenerated|TestAccGKEHub2ScopeIamMemberGenerated|TestAccGKEHub2ScopeIamPolicyGenerated|TestAccStorageTransferJob_basic |
Rerun these tests in REPLAYING mode to catch issues
|
@BBBmau The test failure provided a useful error message:
This happened after the API call to create the resource:
|
Grateful for the clear Error message 🙏 We'll have to update the docs also since it has no reference of not being able to use both. The bottom is what is shown in the docs for both fields. @SarahFrench
|
I also noticed that |
Good catch! Looks like it's marked that way in the docs but not the code (phew) so we're not expanding the scope of the PR too much |
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 ( 3 files changed, 233 insertions(+), 5 deletions(-)) |
Tests analyticsTotal tests: 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 testsTestAccStorageTransferJob_eventStream |
Rerun these tests in REPLAYING mode to catch issues
|
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.
Congrats on getting the acceptance test passing! Now we need to ensure the test will run robustly. See individual comments for details 😁
Also, take a look at the guidance about writing release notes - if you were a community contributor I would edit your release note for you, but I'd like you to have a go getting the release note to match our style guide as practice.
mmv1/third_party/terraform/services/storagetransfer/resource_storage_transfer_job_test.go
Show resolved
Hide resolved
mmv1/third_party/terraform/services/storagetransfer/resource_storage_transfer_job_test.go
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/storagetransfer/resource_storage_transfer_job_test.go
Show resolved
Hide resolved
…torage_transfer_job_test.go Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
mmv1/third_party/terraform/services/storagetransfer/resource_storage_transfer_job_test.go
Outdated
Show resolved
Hide resolved
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 ( 4 files changed, 242 insertions(+), 5 deletions(-)) |
Tests analyticsTotal tests: 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 testsTestAccStorageTransferJob_eventStream |
Rerun these tests in REPLAYING mode to catch issues
|
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 ( 3 files changed, 235 insertions(+), 5 deletions(-)) |
Tests analyticsTotal tests: 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 testsTestAccDataprocClusterIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
Thanks for making that final change - I'm happy to approve and merge now. On that page it says this is how release notes for new fields should be added:
So instead of:
I've changed this PR's release note to:
|
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.
See comment above
This PR adds the
event_stream
field to thegoogle_storage_transfer_job
resource. This can be found in the docs hereIt's also referenced as part of a feature request here: hashicorp/terraform-provider-google#13293
Release Note Template for Downstream PRs (will be copied)