magic-modules
magic-modules copied to clipboard
feat: datastream sql server connection profile/stream
Adds datastream connection profile and stream configuration for sql server. Fixes https://github.com/hashicorp/terraform-provider-google/issues/17981
Release Note Template for Downstream PRs (will be copied)
datastream: added sql server connection profile/stream configuration
Hello! I am a robot. Tests will require approval from a repository maintainer to run.
@melinath, 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.
Thanks for this contribution! Two initial changes to request:
- Please remove all the changes that were added by an auto-formatting tool; they make it harder to review.
- Please set up a real database for the new example using google_sql_database_instance (and remove skip_test: true)
Sorry about the linter. That is now fixed. Will work on adding the test case.
@melinath I ran into a point where I thought it would be worth clarifying a question before I complete the test case.
- Is it okay to use either an additional provider (for example a mssql provider) or a terraform_data resource for the tests/documentation?
Context:
When creating a google_sql_database_instance
I tried a few things:
- Created a new user and database to use for the stream.
- Use the root user and a new database to create the stream.
- Use the root user and one of the default databases to create the stream.
All of these options present issues:
- When creating a new user and database, the user does not have access to the database by default. This requires a sql command to be run to provide the new user with access to the new database. From what I could tell, providing access to the new database is not possible using the
google_sql_database
orgoogle_sql_user
used to create these resources. If I missed something, please point me in the right direction. - Using the root user and a new database, I have access to the new database. However, the new database does not have CDC enabled which is required to create the stream. Again this requires a sql command be run.
- Using the root and an existing database, I don't have access to all of the databases. For the databases I do have access, CDC is not enabled and requires a sql command be run.
To solve for this, I can use a mssql provider or add a terraform_data
resource that will run local-exec
to execute the necessary sql commands. However, adding a new provider didn't seem like a good option as I couldn't find an official provider and using local-exec
seemed a bit messy since I would need to ensure sqlcmd
is installed as part of the script.
With that said, I have locally tested the modifications to the provider to ensure I can successfully create a sql server connection profile and stream to a database that is correctly configured.
Let me know how you'd like to proceed.
In the meantime, I added the sql server instance, db and user config to the examples for completeness, but left the tests disabled: 59b0f38bee044e8bca1c45b38fcadb164b553821
@melinath I just added an example for the connection profile (includes google_sql_database_instance). Tests are still set to skip.
As a reminder from my previous comment:
To solve for this, I can use a mssql provider or add a terraform_data resource that will run local-exec to execute the necessary sql commands. However, adding a new provider didn't seem like a good option as I couldn't find an official provider and using local-exec seemed a bit messy since I would need to ensure sqlcmd is installed as part of the script.
Let me know if your preference is to still run the tests given the above. IMO the best option is to skip them as the other examples have done.
/gcbrun
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
google
provider: Diff ( 2 files changed, 456 insertions(+), 1 deletion(-))
google-beta
provider: Diff ( 4 files changed, 2986 insertions(+), 801 deletions(-))
terraform-google-conversion
: Diff ( 2 files changed, 857 insertions(+), 120 deletions(-))
Missing test report
Your PR includes resource fields which are not covered by any test.
Resource: google_datastream_connection_profile
(9 total tests)
Please add an acceptance test which includes these fields. The test should include the following:
resource "google_datastream_connection_profile" "primary" {
sql_server_profile {
database = # value needed
hostname = # value needed
password = # value needed
port = # value needed
username = # value needed
}
}
Resource: google_datastream_stream
(5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:
resource "google_datastream_stream" "primary" {
backfill_all {
sql_server_excluded_objects {
schemas {
schema = # value needed
tables {
columns {
column = # value needed
data_type = # value needed
}
table = # value needed
}
}
}
}
source_config {
sql_server_source_config {
exclude_objects {
schemas {
schema = # value needed
tables {
columns {
column = # value needed
data_type = # value needed
}
table = # value needed
}
}
}
include_objects {
schemas {
schema = # value needed
tables {
columns {
column = # value needed
data_type = # value needed
}
table = # value needed
}
}
}
max_concurrent_backfill_tasks = # value needed
max_concurrent_cdc_tasks = # value needed
}
}
}
Tests analytics
Total tests: 11
Passed tests: 4
Skipped tests: 7
Affected tests: 0
Click here to see the affected service packages
- datastream
$\textcolor{green}{\textsf{All tests passed!}}$ View the build log
yeah, if the tests require manual SQL setup then we can't automate them, unfortunately. Would you be able to share debug logs from the local manual test as confirmation? I do also still need to go over the implementation in more detail.
Sure thing. Attached is the config (without the variables set) as well as the output with TF_LOG=DEBUG
. I've redacted a few of the values with (sensitive value)
.
@melinath is this message from the modular-magician something I need to address given the manual sql config on the db I mentioned earlier?
@melinath is this message from the modular-magician something I need to address given the manual sql config on the db I mentioned earlier?
no need to do anything about it since we're doing manual tests. Thanks for checking!
@iperetz-goo would you be interested in taking a glance at this?
marking as changes requested due to needing to revert min_version: beta removal
@melinath I've reverted the min_version: beta
removal and the output only properties. I think that takes care of the outstanding change requests.
@melinath let me know if I can provide more clarity on how the output only parameters were tested
@nbugden I'm not sure exactly what happened, but the error
Value for unconfigurable attribute
will only happen if a field is labeled as output-only in the provider and you're trying to specify it - it's coming from Terraform, not from the API. So most likely either the provider code was not regenerated before running the test, or not all the fields that you were trying to set got the output-only attribute removed. Could you try again?
I found the issue... there were some items that still had output: true
testing again.
@melinath with output: true
removed each parameter is consistently failing with this error:
╷
│ Error: Error waiting for Updating Stream:
│
│ with google_datastream_stream.default,
│ on example.tf line 91, in resource "google_datastream_stream" "default":
│ 91: resource "google_datastream_stream" "default" {
│
╵
Here is the full output of testing each parameter: https://github.com/GoogleCloudPlatform/magic-modules/pull/10553#discussion_r1592867868
@nbugden unfortunately we're not getting the actual error message in the normal output; could you add TF_LOG=DEBUG
to one of those runs and see what error the API is actually returning?
@melinath I'm not seeing anything more helpful in the debug logs... this is what I found:
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: {
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "name": "projects/prj-s-telus-tpm-data-fcfc/locations/us-east1/operations/operation-1715115613831-617e378e9408f-60802f75-36e04b7e",
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "metadata": {
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "@type": "type.googleapis.com/google.cloud.datastream.v1.OperationMetadata",
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "createTime": "2024-05-07T21:00:13.853003653Z",
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "endTime": "2024-05-07T21:00:15.076308037Z",
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "target": "projects/prj-s-telus-tpm-data-fcfc/locations/us-east1/streams/sql-server-stream",
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "verb": "update",
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "requestedCancellation": false,
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "apiVersion": "v1"
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: },
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "done": true,
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "error": {
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "code": 13,
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: "message": "an internal error has occurred"
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: }
2024-05-07T17:00:24.156-0400 [DEBUG] provider.terraform-provider-google-beta: }
Full logs output.log
Error code 13 is not helpful, yeah 😂 Let's assume they are output-only for now since that was working.
@melinath let me know if there is anything else you'd like to address in the PR.
@melinath let me know if there is anything left to change/review
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
google
provider: Diff ( 2 files changed, 442 insertions(+), 1 deletion(-))
google-beta
provider: Diff ( 4 files changed, 2939 insertions(+), 833 deletions(-))
terraform-google-conversion
: Diff ( 2 files changed, 832 insertions(+), 128 deletions(-))
Missing test report
Your PR includes resource fields which are not covered by any test.
Resource: google_datastream_connection_profile
(9 total tests)
Please add an acceptance test which includes these fields. The test should include the following:
resource "google_datastream_connection_profile" "primary" {
sql_server_profile {
database = # value needed
hostname = # value needed
password = # value needed
port = # value needed
username = # value needed
}
}
Resource: google_datastream_stream
(5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:
resource "google_datastream_stream" "primary" {
backfill_all {
sql_server_excluded_objects {
schemas {
schema = # value needed
tables {
columns {
column = # value needed
data_type = # value needed
}
table = # value needed
}
}
}
}
source_config {
sql_server_source_config {
exclude_objects {
schemas {
schema = # value needed
tables {
columns {
column = # value needed
data_type = # value needed
}
table = # value needed
}
}
}
include_objects {
schemas {
schema = # value needed
tables {
columns {
column = # value needed
data_type = # value needed
}
table = # value needed
}
}
}
max_concurrent_backfill_tasks = # value needed
max_concurrent_cdc_tasks = # value needed
}
}
}
Tests analytics
Total tests: 11
Passed tests: 4
Skipped tests: 7
Affected tests: 0
Click here to see the affected service packages
- datastream
@melinath I noticed this PR wasn't included as part of the latest beta provider release. Do you know when it will be released?https://github.com/hashicorp/terraform-provider-google-beta/releases/tag/v5.29.0
It will likely be included in next week's release - it missed the cut for this week's.