-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Set bigquery connection import id correctly. #4159
Set bigquery connection import id correctly. #4159
Conversation
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=154849" |
@@ -21,7 +21,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides | |||
cloudSql.credential.password: !ruby/object:Overrides::Terraform::PropertyOverride | |||
sensitive: true | |||
id_format: "{{name}}" | |||
import_format: ["{{name}}"] | |||
import_format: ["projects/{{project}}/locations/{{location}}/connections/{{connection_id}}", "{{project}}/{{location}}/{{connection_id}}", "{{location}}/{{connection_id}}"] |
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.
This looks about right based on other import_format
s.
Should id_format
also be updated? It looks like the id_format is sometimes the long form and sometimes {{name}}
so I'm not clear on whether that's intentional or legacy.
Is there a way to add a test to ensure that this works properly?
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.
The id_format
is set after apply, so it's safe to use name
, which is an output field. That's usually the difference - sometimes name
is an output and sometimes it's an input. When it's an output, it should be the ID, when it's an input, ID format should be the long form. Not to say we haven't made occasional mistakes along the way, but that should be the difference, at least.
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.
A test... well, there should be, right? But the thing is, the existing test should have caught this. I'm going to spend a little extra time trying to figure out why it didn't.
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.
ah. we don't actually do import state verify in the tests! that's why! Let me fix that.
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.
Okay! Finally done. That was a tough one but I got it once I got started, haha.
dc29dcc
to
bbf0cdd
Compare
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=159987" |
/gcbrun |
@ndmckinley thanks for adding the tests! It looks like there were some test failures, but they seem unrelated; rerunning gcb to see if they were intermittent. |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=160647" |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceSpannerInstance_basic|TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccBigqueryDataTransferConfig|TestAccComposerEnvironment_withUpdateOnCreate|TestAccContainerNodePool_basic|TestAccContainerNodePool_nodeLocations|TestAccContainerNodePool_maxPodsPerNode|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccContainerNodePool_withNodeConfig|TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withLinuxNodeConfig|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withNodeConfigScopeAlias|TestAccContainerNodePool_withSandboxConfig|TestAccContainerNodePool_regionalAutoscaling|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_withManagement|TestAccContainerNodePool_regionalClusters|TestAccContainerNodePool_resize|TestAccContainerNodePool_autoscaling|TestAccContainerNodePool_shieldedInstanceConfig|TestAccContainerNodePool_012_ConfigModeAttr|TestAccContainerNodePool_EmptyGuestAccelerator|TestAccFilestoreInstance_filestoreInstanceBasicExample|TestAccFilestoreInstance_update|TestAccOSLoginSSHPublicKey_osLoginSshKeyExpiry You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=160651" |
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
Fixes hashicorp/terraform-provider-google#7615.
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)