magic-modules icon indicating copy to clipboard operation
magic-modules copied to clipboard

feat: datastream sql server connection profile/stream

Open nbugden opened this issue 2 months ago • 2 comments

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

nbugden avatar Apr 29 '24 18:04 nbugden

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.

github-actions[bot] avatar Apr 29 '24 18:04 github-actions[bot]

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.

nbugden avatar Apr 29 '24 21:04 nbugden

@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:

  1. Created a new user and database to use for the stream.
  2. Use the root user and a new database to create the stream.
  3. Use the root user and one of the default databases to create the stream.

All of these options present issues:

  1. 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 or google_sql_user used to create these resources. If I missed something, please point me in the right direction.
  2. 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.
  3. 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.

nbugden avatar Apr 30 '24 17:04 nbugden

In the meantime, I added the sql server instance, db and user config to the examples for completeness, but left the tests disabled: 59b0f38bee044e8bca1c45b38fcadb164b553821

nbugden avatar Apr 30 '24 17:04 nbugden

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

nbugden avatar May 01 '24 18:05 nbugden

/gcbrun

melinath avatar May 02 '24 20:05 melinath

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
    }
  }
}

modular-magician avatar May 02 '24 22:05 modular-magician

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

modular-magician avatar May 02 '24 22:05 modular-magician

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.

melinath avatar May 02 '24 23:05 melinath

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

nbugden avatar May 03 '24 02:05 nbugden

@melinath is this message from the modular-magician something I need to address given the manual sql config on the db I mentioned earlier?

nbugden avatar May 03 '24 20:05 nbugden

@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!

melinath avatar May 03 '24 22:05 melinath

@iperetz-goo would you be interested in taking a glance at this?

melinath avatar May 03 '24 23:05 melinath

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.

nbugden avatar May 06 '24 17:05 nbugden

@melinath let me know if I can provide more clarity on how the output only parameters were tested

nbugden avatar May 07 '24 12:05 nbugden

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

nbugden avatar May 07 '24 17:05 nbugden

@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 avatar May 07 '24 17:05 nbugden

@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 avatar May 07 '24 18:05 melinath

@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

nbugden avatar May 07 '24 21:05 nbugden

Error code 13 is not helpful, yeah 😂 Let's assume they are output-only for now since that was working.

melinath avatar May 07 '24 21:05 melinath

@melinath let me know if there is anything else you'd like to address in the PR.

nbugden avatar May 08 '24 13:05 nbugden

@melinath let me know if there is anything left to change/review

nbugden avatar May 09 '24 15:05 nbugden

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
    }
  }
}

modular-magician avatar May 09 '24 16:05 modular-magician

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](https://storage.cloud.google.com/ci-vcr-logs/beta/refs/heads/auto-pr-10553/artifacts/fe4bfb1a-27c1-45a8-ae92-6f1a0f90ec64/build-log/replaying_test.log)

modular-magician avatar May 09 '24 16:05 modular-magician

@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

nbugden avatar May 14 '24 14:05 nbugden

It will likely be included in next week's release - it missed the cut for this week's.

melinath avatar May 14 '24 17:05 melinath