Update forward_ssh_connectivity.private_key to be mutable #11930
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @roaks3, 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. |
modular-magician
added
the
awaiting-approval
mmv1/third_party/terraform/services/datastream/resource_datastream_connection_profile_test.go
Outdated
Show resolved
Hide resolved
| @@ -72,6 +75,28 @@ func TestAccDatastreamConnectionProfile_update(t *testing.T) { | |||
| // Disable prevent_destroy | |||
| Config: testAccDatastreamConnectionProfile_mySQLUpdate(context, false, random_pass_2), | |||
| }, | |||
| { | |||
| Config: testAccDatastreamConnectionProfile_SSHKey_Update(context, true, random_key_1), | |||
There was a problem hiding this comment.
This is a little strange, because destroy gets run after all steps are complete, and with the same instance name is being used, so I think it will try to update from mysql to postgres. I would create a new test if you need to use a postgres instance.
mmv1/third_party/terraform/services/datastream/resource_datastream_connection_profile_test.go
Outdated
Show resolved
Hide resolved
| hostname = google_sql_database_instance.instance.public_ip_address | ||
| username = google_sql_user.user.name | ||
| port = 22 | ||
| private_key = "%{private_key}" |
There was a problem hiding this comment.
I'm no real familiar with the use case, but just in case it helps, this test only needs to do enough to use the private_key. If the other config here is extraneous, it could be removed to make things more focused, and possibly faster to deploy.
|
@ari-hacks, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
|
@ari-hacks, this PR is waiting for action from you. If no action is taken, this PR will be closed in 14 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
There was a problem hiding this comment.
I can see you've been pretty thorough with your change, but the test still needs some work. I would really suggest creating a new test instead of adding to TestAccDatastreamConnectionProfile_update, but I think it could still technically work.
Here's what likely needs to change:
acctest.SkipIfVcr(t)would need to be removed so the test is run in the PRRandSSHKeyPair()won't work because it will be random on every run (see mmv1/third_party/terraform/acctest/test_utils.go.tmpl), so you would need to wrap it. I would suggest just usingRandString(or hard-coded keys) unless that causes issues for some reason.
|
@ari-hacks, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
|
@ari-hacks, this PR is waiting for action from you. If no action is taken, this PR will be closed in 14 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
Thanks @roaks3! Sorry for the delay on this. To get this working without too much of a lift what if we:
|
|
@roaks3 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
|
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 1 week. Please take a look! Use the label |
|
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 2 weeks. Please take a look! Use the label |
|
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 3 weeks. Please take a look! Use the label |
There was a problem hiding this comment.
I see all the test steps are wrapped in VcrTest. Does that means without SkipIfVcr(t) those test should run?
Yea, remove that line and the test will run in a PR check, which helps us ensure it passes before we merge.
decouple this test from the other ones with your suggestion of creating a new test - I believe this would need a separate file? resource_datastream_connection_profile_frwd_ssh_test.go
I would actually suggest using the existing file, but copying an existing testAcc... function, and then modifying as needed.
RandString() doesn't support the key format for SSH so I can hard code a random private/public key pair
Yep, hardcoding seems reasonable to me here.
|
@ari-hacks, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
This prevents the connection profile from failing permanently (needing to be re-created) when the forward ssh connectivity private key is updated.
Fixes hashicorp/terraform-provider-google#18999
Release Note Template for Downstream PRs (will be copied)