Skip to content
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 status to bigquery_job #4455

Merged

Conversation

kanterov
Copy link
Contributor

@kanterov kanterov commented Feb 2, 2021

Fixes hashicorp/terraform-provider-google#8357

If this PR is for Terraform, I acknowledge that I have:

  • Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

bigquery: added `status` field to `google_bigquery_job`

@google-cla
Copy link

google-cla bot commented Feb 2, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Feb 2, 2021
@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@slevenick, please review this PR or find an appropriate assignee.

@@ -978,6 +978,40 @@ objects:
description: |
The geographic location of the job. The default value is US.
default_value: 'US'
- !ruby/object:Api::Type::NestedObject
name: 'status'
output: true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is needed. Because of that terraform generator doesn't set maxItems: 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is needed because the user cannot set the value of the status object. output: true is not necessarily needed on the sub-fields of this object, as if the top-level is output only then all sub fields will be as well

@kanterov
Copy link
Contributor Author

kanterov commented Feb 2, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Feb 2, 2021
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 138 insertions(+))
Terraform Beta: Diff ( 2 files changed, 138 insertions(+))

@modular-magician
Copy link
Collaborator

Oops! It looks like you're using an unknown release-note type in your changelog entries:

  • REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/gcbrun

Rerunning jobs as release note is fixed now

@@ -978,6 +978,40 @@ objects:
description: |
The geographic location of the job. The default value is US.
default_value: 'US'
- !ruby/object:Api::Type::NestedObject
name: 'status'
output: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is needed because the user cannot set the value of the status object. output: true is not necessarily needed on the sub-fields of this object, as if the top-level is output only then all sub fields will be as well

@slevenick
Copy link
Contributor

/gcbrun

Apparently review comments don't trigger this

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=170550"

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 138 insertions(+))
Terraform Beta: Diff ( 2 files changed, 138 insertions(+))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccBigQueryJob_bigqueryJobQueryTableReferenceExample|TestAccBigQueryJob_bigqueryJobQueryExample|TestAccBigQueryJob_bigqueryJobLoadExample|TestAccBigQueryJob_withLocation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityCmekExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=170552"

@kanterov kanterov marked this pull request as ready for review February 2, 2021 22:08
@kanterov kanterov marked this pull request as draft February 2, 2021 22:23
@slevenick slevenick self-requested a review February 2, 2021 22:24
@kanterov
Copy link
Contributor Author

kanterov commented Feb 2, 2021

I fixed my setup so I can run tests locally and now I can see what's wrong. Fixing it now and going to update the code soon.

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this causes a panic during the bigquery tests:

panic: status.0.error_result: '' expected type 'string', got unconvertible type 'map[string]interface {}'

Looks like the errorResult is this structure: https://cloud.google.com/bigquery/docs/reference/rest/v2/ErrorProto

rather than just a string

@kanterov kanterov force-pushed the add-status-to-bigquery-job branch from 05ca323 to 91ef325 Compare February 2, 2021 22:28
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 197 insertions(+))
Terraform Beta: Diff ( 2 files changed, 197 insertions(+))

@kanterov
Copy link
Contributor Author

kanterov commented Feb 2, 2021

/gcbrun

@kanterov
Copy link
Contributor Author

kanterov commented Feb 2, 2021

@slevenick sorry, totally confused it. Now tests pass locally, is it possible to re-trigger GCB?

@slevenick
Copy link
Contributor

/gcbrun

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=170555"

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 197 insertions(+))
Terraform Beta: Diff ( 2 files changed, 197 insertions(+))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccBigQueryJob_bigqueryJobLoadExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityCmekExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=170640"

@kanterov kanterov marked this pull request as ready for review February 3, 2021 08:41
@kanterov
Copy link
Contributor Author

kanterov commented Feb 3, 2021

I can't reproduce the failure of TestAccBigQueryJob_bigqueryJobLoadExample locally, is there any way to see the logs?

TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample and TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityCmekExample seem to be failing in the main branch as well.

@slevenick
Copy link
Contributor

I can't reproduce the failure of TestAccBigQueryJob_bigqueryJobLoadExample locally, is there any way to see the logs?

TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample and TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityCmekExample seem to be failing in the main branch as well.

Yeah, the other two tests are unrelated to your changes, ignore them.

I'm still seeing this failure for the bigquery job:

TestAccBigQueryJob_bigqueryJobLoadExample: provider_test.go:270: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
(map[string]string) (len=1) {
(string) (len=14) "status.0.state": (string) (len=4) "DONE"
}
(map[string]string) (len=1) {
(string) (len=14) "status.0.state": (string) (len=7) "RUNNING"
}

it looks like the job status is changing between creating the job & the import step running. This may be flaky due to the time it takes the job to finish. We probably want to ignore the status when running the import step due to this, as it should never be set by the user.

You can add status.0.state to the ImportStateVerifyIgnore for the bigquery job tests and it should fix this

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 206 insertions(+), 9 deletions(-))
Terraform Beta: Diff ( 5 files changed, 207 insertions(+), 10 deletions(-))

@kanterov
Copy link
Contributor Author

kanterov commented Feb 3, 2021

@slevenick thanks for the clarifications, I've updated tests, including auto-generated ones. Should we /gcbrun?

@slevenick
Copy link
Contributor

/gcbrun

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=170713"

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 206 insertions(+), 9 deletions(-))
Terraform Beta: Diff ( 5 files changed, 207 insertions(+), 10 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityCmekExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=170729"

@slevenick slevenick self-requested a review February 3, 2021 23:40
Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks

@kanterov
Copy link
Contributor Author

kanterov commented Feb 4, 2021

@slevenick thanks a lot for your support, I learned a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "state" and "error_result" field to google_bigquery_job
3 participants