-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add support for Stored Procedure for Apache Spark #9793
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
Conversation
Hello! I am a robot. It looks like you are a: @roaks3, 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 ( 3 files changed, 571 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_bigquery_routine" "primary" {
spark_options {
archive_uris = # value needed
container_image = # value needed
file_uris = # value needed
main_file_uri = # value needed
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBigQueryRoutine_bigQueryRoutineSparkJarExample|TestAccBigQueryRoutine_bigQueryRoutinePysparkExample |
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.
Overall LGTM and it looks like tests passed. Just a few minor comments
mmv1/templates/terraform/examples/big_query_routine_pyspark_mainfile.tf.erb
Show resolved
Hide resolved
@roaks3 Thank you for your feedback. I have made some corrections. I have also commented on a consideration, so I would be happy if you could take a look at that as well. |
/gcbrun |
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, 753 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBigQueryRoutine_bigQueryRoutinePysparkMainfileExample|TestAccBigQueryRoutine_bigQueryRoutinePysparkExample|TestAccBigQueryRoutine_bigQueryRoutineSparkJar_Update|TestAccBigQueryRoutine_bigQueryRoutineSparkJarExample |
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.
LGTM, one remaining comment on the update test to make sure these fields are configured properly.
mmv1/templates/terraform/examples/big_query_routine_pyspark_mainfile.tf.erb
Show resolved
Hide resolved
connection = google_bigquery_connection.test.name | ||
runtime_version = "2.1" | ||
container_image = "gcr.io/my-project-id/my-spark-image:latest" | ||
main_class = "com.google.test.jar.MainClass" |
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.
I would consider changing main_class
and jar_uris
with this test, maybe even connection
, if that is possible. Note that any fields that cannot be updated in-place should have immutable: true
, to make sure that they signal the resource needs to be recreated.
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.
Thank you. I will modify some tests. Also, I will send another PR for the definition_body.
There will be a delay in response due to a business trip, but I will send out a PR within this week.
/gcbrun |
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, 753 insertions(+), 3 deletions(-)) |
1 similar comment
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, 753 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packages
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 testsTestAccBigQueryRoutine_bigQueryRoutineSparkJar_Update |
Rerun these tests in REPLAYING mode to catch issues
|
Tests analyticsTotal tests: Click here to see the affected service packages
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 testsTestAccBigQueryRoutine_bigQueryRoutineSparkJar_Update |
Rerun these tests in REPLAYING mode to catch issues
|
@roaks3 I have updated the test for updates. I believe now we can merge the PR. |
…m#9793) * impl for hashicorp/terraform-provider-google#16953 * modified format in example * modified tab to space * added test for update and test for coverage * mofify all resource in the spark option section * modified connection id to avoid conflict
…m#9793) * impl for hashicorp/terraform-provider-google#16953 * modified format in example * modified tab to space * added test for update and test for coverage * mofify all resource in the spark option section * modified connection id to avoid conflict
…m#9793) * impl for hashicorp/terraform-provider-google#16953 * modified format in example * modified tab to space * added test for update and test for coverage * mofify all resource in the spark option section * modified connection id to avoid conflict
…m#9793) * impl for hashicorp/terraform-provider-google#16953 * modified format in example * modified tab to space * added test for update and test for coverage * mofify all resource in the spark option section * modified connection id to avoid conflict
PR for hashicorp/terraform-provider-google#16953, adding stored procedure for apache spark on google_bigquery_routine resource.
Fixes hashicorp/terraform-provider-google#16953
Release Note Template for Downstream PRs (will be copied)