Adding ServiceConnectionPolicies resource in NetworkConnectivity. #8273

diogoEsteves42 · 2023-07-06T01:16:05Z

Adding two new resources in TPG.
Service Connection Policy requires a Service Class
https://cloud.google.com/vpc/docs/about-service-connection-policies

We can find information about the resources in the explorer file
https://cloud.google.com/network-connectivity/docs/reference/networkconnectivity/rest

This PR solves: hashicorp/terraform-provider-google#15051

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

google_network_connectivity_service_connection_policy

… to fix the test scenario for policy

modular-magician · 2023-07-06T01:16:52Z

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @roaks3, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

Ensuring it's backwards compatible, covers common error cases, etc.
Summarizing the change into a user-facing changelog note.
Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

diogoEsteves42 · 2023-07-06T01:20:41Z

Hi, @roaks3 I have two questions about this PR
I see that the product networkConnectivity was started via DCL, now when I create new resources using MMv1,
it looks like I have to remove the reference from the DCL in the file mmv1/third_party/terraform/fwmodels/provider_model.go.erb, is that approach correct?

Another one, the resource serviceClass will just be published to public preview next year, there's a way to handle
the situation when one resource depends on another that it's not released yet?

It's a draft, I need to fix the test scenario for one resource and validate some points.

diogoEsteves42 · 2023-07-11T13:50:40Z

Hi @roaks3, just opened the PR to review. But I need some help with the serviceClass resource.

…to evaluate

roaks3 · 2023-07-13T14:47:14Z

Hi @diogoEsteves42 , this repo (used for GA and beta providers) requires that APIs be public. If serviceClass is not available in a public preview until next year, we would need to wait before making that addition here.

Could you confirm that the API you need is not currently public? Any other information you can provide would also help (like if you have engaged with a TAM).

…t, need to evaluate" This reverts commit 4240e1d.

diogoEsteves42 · 2023-07-13T20:38:31Z

Hi @roaks3 , thanks for the answer. The last information that I have about serviceClass is that it will be available to users without allowlisting on early next year.

Can I kind of mock this resource? Use it with a static value with an env_var? Because the serviceConnectionPolicy it's ready, but I don't know how to handle the dependency as it's a required field. I'll update the PR by removing the serviceClass but I need to know how to make the test pass.

appreciate any help

...hird_party/terraform/tests/resource_network_connectivity_service_connection_policies_test.go

diogoEsteves42 · 2023-07-14T19:53:35Z

Hi @roaks3 I've just made a simple update in the code, but I haven't removed the service class yet.

One question:
If the resource is published in the discovery file for v1, is this not enough to create a product here?

If it's not possible I'll update the PR and then we can follow the review.
Appreciate the help

roaks3 · 2023-07-14T20:54:08Z

Hey @diogoEsteves42 , since the API is currently marked as Pre-GA, and uses an allowlist, it would not be something we can add to either of the public providers. However, I can follow up with the team internally to verify that those assumptions are correct.

diogoEsteves42 · 2023-07-19T16:47:38Z

Hi @roaks3 another person is facing the same situation with some feature or resource not being public and he mentions something about releasing it in another way (inside google).

I will remove the serviceClass and update the tests, I'll @ you when it's ready.

diogoEsteves42 · 2023-07-19T20:51:18Z

@roaks3 I've updated the PR, ready to review.

roaks3

I took a first pass and had a few comments on the yaml. Will wait to see what comes back from the tests before continuing review. Thanks for the contribution!

mmv1/products/networkconnectivity/ServiceConnectionPolicies.yaml

modular-magician · 2023-07-20T19:47:05Z

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.

Terraform GA: Diff ( 13 files changed, 1321 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 13 files changed, 1321 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 5 files changed, 168 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 4 files changed, 135 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_network_connectivity_service_connection_policy (4 total tests)
Untested fields: etag

Please add acceptance tests which include these fields.

modular-magician · 2023-07-20T21:04:34Z

Tests analytics

Total tests: 2874
Passed tests 2571
Skipped tests: 299
Affected tests: 4

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccNetworkConnectivityServiceConnectionPolicy_networkConnectivityPolicyBasicExample|TestAccNetworkConnectivityServiceConnectionPolicy_update|TestAccComputeFirewallPolicyRule_multipleRules|TestAccContainerAwsNodePool_BetaBasicHandWritten

Get to know how VCR tests work

modular-magician · 2023-08-01T02:21:28Z

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.

Terraform GA: Diff ( 14 files changed, 1287 insertions(+), 23 deletions(-))
Terraform Beta: Diff ( 13 files changed, 1287 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 5 files changed, 168 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 4 files changed, 124 insertions(+))

modular-magician · 2023-08-01T04:07:02Z

Tests analytics

Total tests: 2912
Passed tests 2607
Skipped tests: 302
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccNetworkConnectivityServiceConnectionPolicy_networkConnectivityPolicyBasicExample|TestAccNetworkConnectivityServiceConnectionPolicy_update

Get to know how VCR tests work

modular-magician · 2023-08-01T04:10:35Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccNetworkConnectivityServiceConnectionPolicy_networkConnectivityPolicyBasicExample[Debug log]
TestAccNetworkConnectivityServiceConnectionPolicy_update[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

roaks3

Looks good! I would just like to make sure the network field is in a good place so that we don't need to make changes later.

roaks3 · 2023-08-01T14:12:49Z

mmv1/products/networkconnectivity/ServiceConnectionPolicies.yaml

+    description: |
+      Free-text description of the resource.
+  - !ruby/object:Api::Type::String
+    name: 'network'


Hmm, that's strange. Could you provide any more info on that? I think we should be able to configure this property in a way that works.

Alternatively, you could submit it with the immutable: true and then I can see the failure in the logs in VCR.

roaks3 · 2023-08-01T14:13:26Z

...hird_party/terraform/tests/resource_network_connectivity_service_connection_policies_test.go

+    network       = google_compute_network.producer_net.id
+  }
+
+  resource "google_compute_subnetwork" "producer_subnet1" {


Nit: You should be able to remove this subnet because it isn't being used in this config. I don't think it will be faster, but a little cleaner to read.

got it, removed.

roaks3 · 2023-08-01T14:13:33Z

...hird_party/terraform/tests/resource_network_connectivity_service_connection_policies_test.go

+  auto_create_subnetworks = false
+}
+
+resource "google_compute_subnetwork" "producer_subnet" {


Nit: Similarly, you should be able to remove this subnet because it isn't being used in this config

great, removed.

…etwork as immutable

diogoEsteves42 · 2023-08-01T15:35:15Z

@roaks3 let's see the test fails.
It just fails locally.

another point it's that we don't have the update method for the CLI
https://cloud.google.com/sdk/gcloud/reference/network-connectivity/service-connection-policies/create

modular-magician · 2023-08-01T15:59:27Z

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.

Terraform GA: Diff ( 14 files changed, 1264 insertions(+), 23 deletions(-))
Terraform Beta: Diff ( 13 files changed, 1264 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 5 files changed, 168 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 4 files changed, 124 insertions(+))

modular-magician · 2023-08-01T17:27:21Z

Tests analytics

Total tests: 2912
Passed tests 2608
Skipped tests: 302
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccNetworkConnectivityServiceConnectionPolicy_update|TestAccContainerAwsNodePool_BetaBasicHandWritten

Get to know how VCR tests work

modular-magician · 2023-08-01T17:29:49Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccNetworkConnectivityServiceConnectionPolicy_update[Error message] [Debug log]
TestAccContainerAwsNodePool_BetaBasicHandWritten[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

roaks3 · 2023-08-01T19:38:07Z

Looking at the error, it does appear that network is required on the PATCH call, but is not being sent when it is immutable. The best way to handle this would be to add an encoder similar to encoders/apigee_organization.go.erb, which would explicitly set the network on the request obj before being sent in the PATCH. You can configure the encoder with a custom_code block in the yaml, and a update_encoder entry under that.

I think this will fix the issue with updates, while allowing network to remain immutable. Let me know how it goes.

…h method

diogoEsteves42 · 2023-08-02T01:10:59Z

Owww, that works, really glad that you helped me @roaks3
Amazing, I believe that now everything is set as expected by the API and will be fine on Terraform.

Thanks again, without your help I would spend much more time on this.

modular-magician · 2023-08-02T13:18:09Z

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.

Terraform GA: Diff ( 14 files changed, 1274 insertions(+), 23 deletions(-))
Terraform Beta: Diff ( 13 files changed, 1274 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 5 files changed, 168 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 4 files changed, 124 insertions(+))

modular-magician · 2023-08-02T14:37:13Z

Tests analytics

Total tests: 2919
Passed tests 2616
Skipped tests: 302
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccNetworkConnectivityServiceConnectionPolicy_update

Get to know how VCR tests work

modular-magician · 2023-08-02T14:40:58Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccNetworkConnectivityServiceConnectionPolicy_update[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}
View the build log or the debug log for each test

roaks3

LGTM, thanks for working through all the feedback.

I also tweaked the release note to remove the service class resource.

…ogleCloudPlatform#8273) * adding both resources, basic test for serviceClass it's working, need to fix the test scenario for policy * fix the test scenario and the service class resource properties * adding a testing update, just adding the labels for the resource * added the resource service connection map, and it's basic test, need to evaluate * Revert "added the resource service connection map, and it's basic test, need to evaluate" This reverts commit 4240e1d. * removing the service class in one test scenario and using a static value * fix a typo * removing serviceClass and fix the tests * removing ga tag and updatinga etag for fingerprint type * updating the code, to remove the static value from the docs, need to test * adding a file to tpgtools override at product level * adding function to help with env var, but the patch on tests it's failing * adding the variable to be used in the docs * fixing the test, the network field cannot be immutable * removing the env_var service class and adding a static value * updating the value used from tests and documentation * removing unused resources for docs and tests * fix typos, not used variables and run the fmt * adding description to the basic test * fixed yaml variable override * removing unused resources for tests, and making the test fails with network as immutable * adding encoder to help network field be immutable and be sent on patch method

adding both resources, basic test for serviceClass it's working, need…

ffc9883

… to fix the test scenario for policy

modular-magician requested a review from roaks3 July 6, 2023 01:16

modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Jul 6, 2023

diogoEsteves42 added 3 commits July 7, 2023 20:21

fix the test scenario and the service class resource properties

4eb449d

Merge branch 'main' into create-service-automation-connection-policy

5dac289

adding a testing update, just adding the labels for the resource

5230861

diogoEsteves42 marked this pull request as ready for review July 11, 2023 13:49

diogoEsteves42 added 2 commits July 13, 2023 12:58

added the resource service connection map, and it's basic test, need …

4240e1d

…to evaluate

Merge branch 'main' into create-service-automation-connection-policy

6460bfb

Revert "added the resource service connection map, and it's basic tes…

ccd09fe

…t, need to evaluate" This reverts commit 4240e1d.

removing the service class in one test scenario and using a static value

5a413b5

diogoEsteves42 commented Jul 14, 2023

View reviewed changes

...hird_party/terraform/tests/resource_network_connectivity_service_connection_policies_test.go Show resolved Hide resolved

diogoEsteves42 added 2 commits July 17, 2023 17:18

Merge branch 'main' into create-service-automation-connection-policy

8b53efc

fix a typo

110baa6

diogoEsteves42 added 2 commits July 19, 2023 20:49

removing serviceClass and fix the tests

a4edf35

Merge branch 'main' into create-service-automation-connection-policy

be29545

diogoEsteves42 changed the title ~~Adding ServiceConnectionPolicies and ServiceClass resources in NetworkConnectivity.~~ Adding ServiceConnectionPolicies resource in NetworkConnectivity. Jul 19, 2023

modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Jul 20, 2023

roaks3 reviewed Jul 20, 2023

View reviewed changes

mmv1/products/networkconnectivity/ServiceConnectionPolicies.yaml Outdated Show resolved Hide resolved

mmv1/products/networkconnectivity/ServiceConnectionPolicies.yaml Outdated Show resolved Hide resolved

mmv1/products/networkconnectivity/ServiceConnectionPolicies.yaml Show resolved Hide resolved

roaks3 reviewed Aug 1, 2023

View reviewed changes

removing unused resources for tests, and making the test fails with n…

b06a8c7

…etwork as immutable

modular-magician requested a review from roaks3 August 1, 2023 15:31

modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Aug 1, 2023

modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Aug 1, 2023

adding encoder to help network field be immutable and be sent on patc…

a448bf9

…h method

modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Aug 2, 2023

modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Aug 2, 2023

roaks3 approved these changes Aug 2, 2023

View reviewed changes

roaks3 merged commit 60bd90b into GoogleCloudPlatform:main Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ServiceConnectionPolicies resource in NetworkConnectivity. #8273

Adding ServiceConnectionPolicies resource in NetworkConnectivity. #8273

diogoEsteves42 commented Jul 6, 2023 •

edited by roaks3

Loading

modular-magician commented Jul 6, 2023

diogoEsteves42 commented Jul 6, 2023 •

edited

Loading

diogoEsteves42 commented Jul 11, 2023

roaks3 commented Jul 13, 2023

diogoEsteves42 commented Jul 13, 2023 •

edited

Loading

diogoEsteves42 commented Jul 14, 2023

roaks3 commented Jul 14, 2023

diogoEsteves42 commented Jul 19, 2023 •

edited

Loading

diogoEsteves42 commented Jul 19, 2023 •

edited

Loading

roaks3 left a comment

modular-magician commented Jul 20, 2023

modular-magician commented Jul 20, 2023

modular-magician commented Aug 1, 2023

modular-magician commented Aug 1, 2023

modular-magician commented Aug 1, 2023

roaks3 left a comment

roaks3 Aug 1, 2023

roaks3 Aug 1, 2023

diogoEsteves42 Aug 1, 2023

roaks3 Aug 1, 2023

diogoEsteves42 Aug 1, 2023

diogoEsteves42 commented Aug 1, 2023

modular-magician commented Aug 1, 2023

modular-magician commented Aug 1, 2023

modular-magician commented Aug 1, 2023

roaks3 commented Aug 1, 2023

diogoEsteves42 commented Aug 2, 2023

modular-magician commented Aug 2, 2023

modular-magician commented Aug 2, 2023

modular-magician commented Aug 2, 2023

roaks3 left a comment

Adding ServiceConnectionPolicies resource in NetworkConnectivity. #8273

Adding ServiceConnectionPolicies resource in NetworkConnectivity. #8273

Conversation

diogoEsteves42 commented Jul 6, 2023 • edited by roaks3 Loading

modular-magician commented Jul 6, 2023

diogoEsteves42 commented Jul 6, 2023 • edited Loading

diogoEsteves42 commented Jul 11, 2023

roaks3 commented Jul 13, 2023

diogoEsteves42 commented Jul 13, 2023 • edited Loading

diogoEsteves42 commented Jul 14, 2023

roaks3 commented Jul 14, 2023

diogoEsteves42 commented Jul 19, 2023 • edited Loading

diogoEsteves42 commented Jul 19, 2023 • edited Loading

roaks3 left a comment

Choose a reason for hiding this comment

modular-magician commented Jul 20, 2023

Diff report

Missing test report

modular-magician commented Jul 20, 2023

Tests analytics

Action taken

modular-magician commented Aug 1, 2023

Diff report

modular-magician commented Aug 1, 2023

Tests analytics

Action taken

modular-magician commented Aug 1, 2023

Rerun these tests in REPLAYING mode to catch issues

roaks3 left a comment

Choose a reason for hiding this comment

roaks3 Aug 1, 2023

Choose a reason for hiding this comment

roaks3 Aug 1, 2023

Choose a reason for hiding this comment

diogoEsteves42 Aug 1, 2023

Choose a reason for hiding this comment

roaks3 Aug 1, 2023

Choose a reason for hiding this comment

diogoEsteves42 Aug 1, 2023

Choose a reason for hiding this comment

diogoEsteves42 commented Aug 1, 2023

modular-magician commented Aug 1, 2023

Diff report

modular-magician commented Aug 1, 2023

Tests analytics

Action taken

modular-magician commented Aug 1, 2023

roaks3 commented Aug 1, 2023

diogoEsteves42 commented Aug 2, 2023

modular-magician commented Aug 2, 2023

Diff report

modular-magician commented Aug 2, 2023

Tests analytics

Action taken

modular-magician commented Aug 2, 2023

Rerun these tests in REPLAYING mode to catch issues

roaks3 left a comment

Choose a reason for hiding this comment

diogoEsteves42 commented Jul 6, 2023 •

edited by roaks3

Loading

diogoEsteves42 commented Jul 6, 2023 •

edited

Loading

diogoEsteves42 commented Jul 13, 2023 •

edited

Loading

diogoEsteves42 commented Jul 19, 2023 •

edited

Loading

diogoEsteves42 commented Jul 19, 2023 •

edited

Loading