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.

Sign up for GitHub

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

Jump to bottom

Set bigquery connection import id correctly. #4159

Merged
merged 2 commits into from
Nov 30, 2020
Merged

Set bigquery connection import id correctly. #4159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7fe1e6
Set bigquery connection import id correctly.
nat-henderson Oct 28, 2020
bbf0cdd
update to add import tests - and revise fetch path so import can succ…
nat-henderson Nov 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion products/bigqueryconnection/api.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ objects:
- !ruby/object:Api::Resource
name: 'Connection'
base_url: projects/{{project}}/locations/{{location}}/connections
self_link: "{{name}}"
self_link: projects/{{project}}/locations/{{location}}/connections/{{connection_id}}
create_url: projects/{{project}}/locations/{{location}}/connections?connectionId={{connection_id}}
update_verb: :PATCH
update_mask: true
Expand Down
4 changes: 2 additions & 2 deletions products/bigqueryconnection/terraform.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ overrides: !ruby/object:Overrides::ResourceOverrides
custom_flatten: 'templates/terraform/custom_flatten/bigquery_connection_flatten.go.erb'
cloudSql.credential.password: !ruby/object:Overrides::Terraform::PropertyOverride
sensitive: true
id_format: "{{name}}"
import_format: ["{{name}}"]
id_format: "projects/{{project}}/locations/{{location}}/connections/{{connection_id}}"
import_format: ["projects/{{project}}/locations/{{location}}/connections/{{connection_id}}", "{{project}}/{{location}}/{{connection_id}}", "{{location}}/{{connection_id}}"]
Copy link
Member

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_formats.

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

examples:
- !ruby/object:Provider::Terraform::Examples
min_version: beta
Expand Down
12 changes: 12 additions & 0 deletions third_party/terraform/tests/resource_bigquery_connection_test.go.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,21 @@ func TestAccBigqueryConnectionConnection_bigqueryConnectionBasic(t *testing.T) {
{
Config: testAccBigqueryConnectionConnection_bigqueryConnectionBasic(context),
},
{
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"cloud_sql.0.credential.0.password", "cloud_sql.0.credential.0.username"},
ResourceName: "google_bigquery_connection.connection",
},
{
Config: testAccBigqueryConnectionConnection_bigqueryConnectionBasicUpdate(context),
},
{
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"cloud_sql.0.credential.0.password", "cloud_sql.0.credential.0.username"},
ResourceName: "google_bigquery_connection.connection",
},
},
})
}
Expand Down