Add Oracle support to DBMS #9114
Conversation
|
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, 1174 insertions(+), 86 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
forward_ssh_connectivity {
private_key = # value needed
}
private_connectivity {
private_connection = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
}
}
|
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 testsTestAccBigQueryDataTable_bigtable|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfilePostgresPrivateConnectivityExample|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileOracleExample|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfilePostgresExample |
|
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 ( 4 files changed, 1245 insertions(+), 82 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
database_service = # value needed
forward_ssh_connectivity {
private_key = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
}
postgresql {
private_service_connect_connectivity {
service_attachment = # value needed
}
}
}
|
|
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, 1243 insertions(+), 82 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
database_service = # value needed
forward_ssh_connectivity {
private_key = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
}
postgresql {
private_service_connect_connectivity {
service_attachment = # value needed
}
}
}
|
|
Requested a review from you since it was a pretty similar situation to the Datastream stuff from before -- and is facing a similar problem with the Oracle DBs. That said, it is a requested (P1 near SLO) feature. |
|
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, 1304 insertions(+), 83 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
forward_ssh_connectivity {
private_key = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
}
postgresql {
private_service_connect_connectivity {
service_attachment = # value needed
}
}
}
|
|
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, 1305 insertions(+), 83 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
forward_ssh_connectivity {
private_key = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
}
postgresql {
private_service_connect_connectivity {
service_attachment = # value needed
}
}
}
|
Tests analyticsTotal tests: Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withAddons|TestAccDatabaseMigrationServiceConnectionProfile_Postgres_PSC|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileOraclePrivateExample|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileOracleForwardSshExample|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfilePostgresExample|TestAccDataSourceGoogleServiceAccountAccessToken_basic |
...tom_flatten/database_migration_service_connection_profile_oracle_forward_ssh_password.go.erb
Show resolved
Hide resolved
...ices/databasemigrationservice/resource_database_migration_service_connection_profile_test.go
Outdated
Show resolved
Hide resolved
|
Rerun these tests in REPLAYING mode to catch issues
|
| - 'oracle.0.password' | ||
| vars: | ||
| profile: 'my-profileid' | ||
| network_name: 'my_network' |
There was a problem hiding this comment.
this should have a test_vars_override that overrides this using acctest.BootstrapSharedTestNetwork
There was a problem hiding this comment.
also currently causing failures due to "name" ("tf_test_my_network1xauverxfi") doesn't match regexp "^(?:[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?)$"
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 939 insertions(+), 6 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
forward_ssh_connectivity {
private_key = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
}
}
|
Tests analyticsTotal tests: 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 testsTestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerCluster_withAddons|TestAccWorkstationsWorkstationConfig_update |
|
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: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 935 insertions(+), 6 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
forward_ssh_connectivity {
private_key = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
}
}
|
Tests analyticsTotal tests: Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerCluster_withAddons|TestAccContainerCluster_withDeletionProtection|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileOracleForwardSshExample|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileOraclePrivateExample |
|
|
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 935 insertions(+), 6 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
forward_ssh_connectivity {
private_key = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
}
}
|
Tests analyticsTotal tests: Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withAddons|TestAccContainerCluster_withDeletionProtection|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withKubeletConfig|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileOracleForwardSshExample|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileOraclePrivateExample |
|
|
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 935 insertions(+), 6 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
forward_ssh_connectivity {
private_key = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
}
}
|
Tests analyticsTotal tests: Action takenFound 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withDeletionProtection|TestAccContainerCluster_withAddons|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withKubeletConfig|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileOracleForwardSshExample|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileOraclePrivateExample|TestAccDataprocJobIamPolicy|TestAccDataSourceGoogleServiceAccountIdToken_impersonation|TestAccDataSourceGoogleServiceAccountJwt |
|
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: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 8 files changed, 1752 insertions(+), 8 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
database_service = # value needed
forward_ssh_connectivity {
hostname = # value needed
password = # value needed
port = # value needed
private_key = # value needed
username = # value needed
}
host = # value needed
password = # value needed
port = # value needed
private_connectivity {
private_connection = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
username = # value needed
}
}
|
Tests analyticsTotal tests: 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 testsTestAccContainerCluster_withAddons|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withKubeletConfig|TestAccDatabaseMigrationServicePrivateConnection_databaseMigrationServicePrivateConnectionExample |
|
Rerun these tests in REPLAYING mode to catch issues
|
There was a problem hiding this comment.
Ideally the private connection resource would've been added separately & previously, since it doesn't have a dependency on the oracle support. I have a few questions and change requests, mostly related to it.
If you would like to move forward with the Oracle support I'd be happy to approve that on its own & handle the private connection in a separate PR.
| @@ -336,15 +346,173 @@ properties: | |||
| Output only. If the source is a Cloud SQL database, this field indicates the network architecture it's associated with. | |||
| values: | |||
| - :NETWORK_ARCHITECTURE_OLD_CSQL_PRODUCER | |||
| - :NETWORK_ARCHITECTURE_NEW_CSQL_PRODUCER - | |||
| !ruby/object:Api::Type::NestedObject | |||
| - :NETWORK_ARCHITECTURE_NEW_CSQL_PRODUCER | |||
There was a problem hiding this comment.
this was fixed as part of a separate postgres PR, right?
There was a problem hiding this comment.
The change was started with the post gres, but it was also definitely a typo that got through the original resource implementation so I don't see the harm in fixing it here rather than there so the change happens faster (especially since it isnt actually related to the functionality of that one)
| # limitations under the License. | ||
|
|
||
| --- !ruby/object:Api::Resource | ||
| name: 'PrivateConnection' |
There was a problem hiding this comment.
the name of this file should be PrivateConnection.yaml (but you could fix that in a separate PR & fix connectionprofile at the same time.)
| network_name: 'acctest.BootstrapSharedTestNetwork(t, "dbms-privateconnection")' | ||
| custom_code: !ruby/object:Provider::Terraform::CustomCode | ||
| constants: templates/terraform/constants/dbms_private_connection.go.erb | ||
| post_create: templates/terraform/post_create/private_connection.go.erb |
There was a problem hiding this comment.
this seems to be reusing post_create and post_import functions created for datastream resources - just want to double-check that we're sure this is necessary? If it is necessary, we should create new custom code files that are namespaced to dbms.
| @@ -0,0 +1,34 @@ | |||
| <% unless compiler == "terraformgoogleconversion-codegen" -%> | |||
There was a problem hiding this comment.
It looks like this is copied from the datastream private connection - are we sure this code is all required?
| description: Labels. | ||
| - !ruby/object:Api::Type::String | ||
| name: 'displayName' | ||
| required: true |
There was a problem hiding this comment.
I'm guessing these fields might have been copied from datastream's PrivateConnection.yaml - just wanted to double-check that this field is actually required?
| description: | | ||
| State of the PrivateConnection. | ||
| output: true | ||
| values: |
There was a problem hiding this comment.
this is unused for output-only fields. You can use a String-type field with no values instead.
| A list of messages that carry the error details. | ||
| - !ruby/object:Api::Type::NestedObject | ||
| name: 'vpcPeeringConfig' | ||
| required: true |
There was a problem hiding this comment.
docs aren't clear - double-checking that this is definitely required?
There was a problem hiding this comment.
I did not totally verify for all the shared elements because the functionality seemed so similar -- will verify for the separate PR with the resource
|
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, 759 insertions(+), 6 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_database_migration_service_connection_profile" "primary" {
oracle {
database_service = # value needed
forward_ssh_connectivity {
hostname = # value needed
password = # value needed
port = # value needed
private_key = # value needed
username = # value needed
}
host = # value needed
password = # value needed
port = # value needed
private_connectivity {
private_connection = # value needed
}
ssl {
ca_certificate = # value needed
client_certificate = # value needed
client_key = # value needed
}
username = # value needed
}
}
|
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withKubeletConfig|TestAccContainerCluster_withAddons |
|
|
Fixes hashicorp/terraform-provider-google#15647
Adds support for Oracle profiles in DBMS, additionally adds missing static/private connectivity support to postgres profiles (that was also included with the Oracle profiles).
Two quirks happening here:
We can not actually create an oracle DB for testing (so the created tests fail, as it actually tests a connection to the supplied db)
The data stream private connection is actually a more generic resource than just data stream (reaches the same downstream URI with the same structure), so it would work for that oracle test... in theory
Release Note Template for Downstream PRs (will be copied)