-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Managed zone resource of Integration Connectors. #9617
Conversation
Hello! I am a robot. It looks like you are a: @rileykarson, 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. |
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 ( 5 files changed, 972 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_integration_connectors_managed_zone" "primary" {
description = # value needed
dns = # value needed
labels = # value needed
name = # value needed
target_project = # value needed
target_vpc = # 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 ( 6 files changed, 972 insertions(+), 7 deletions(-)) |
3114ab7
to
2902538
Compare
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 ( 6 files changed, 1053 insertions(+), 2 deletions(-)) |
1 similar comment
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 ( 6 files changed, 1053 insertions(+), 2 deletions(-)) |
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 testsTestAccIntegrationConnectorsManagedZone_integrationConnectorsManagedZoneExample|TestAccIntegrationConnectorsManagedZone_integrationConnectorsManagedZoneExample_update|TestAccDataSourceGoogleServiceAccountIdToken_impersonation |
Rerun these tests in REPLAYING mode to catch issues
|
labels = { | ||
intent = "example" | ||
} | ||
target_project="connectors-example" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See error message in the VCR results. We'll need a real target project- I think we will want to define a google_project
resource inline and permission the service account against it as appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to create a project in this same terraform config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a list of projects in which this may run, so that I can create IAM policies to allow the role in the project which has this resource. It didn't make sense to create end to end of this setup in terraform as this goes to terraform docs as well, please suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rileykarson please take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't make sense to create end to end of this setup in terraform as this goes to terraform docs as well, please suggest.
That should be fine, we do that level of setup in those samples in other products too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend the full setup (including the target project) inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not just the target project, do we allow creating a new project, if so, which billing account to use, etc,etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From chat:
You draw them from the environment! https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/services/apigee/resource_apigee_environment_type_test.go.erb#L13 is a good reference test
|
||
func testAccIntegrationConnectorsManagedZone_integrationConnectorsManagedZoneExample_full(context map[string]interface{}) string { | ||
return acctest.Nprintf(` | ||
data "google_project" "test_project" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not getting used, we can remove it.
...erraform/services/integrationconnectors/resource_integration_connectors_managed_zone_test.go
Show resolved
Hide resolved
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 ( 6 files changed, 1053 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccIntegrationConnectorsManagedZone_integrationConnectorsManagedZoneExample_update|TestAccIntegrationConnectorsManagedZone_integrationConnectorsManagedZoneExample |
|
/gcbrun |
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 ( 9 files changed, 1401 insertions(+), 6 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_integration_connectors_connection" "primary" {
eventing_config {
proxy_destination_config {
destination {
service_attachment = # value needed
}
}
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
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 testsTestAccIntegrationConnectorsConnection_integrationConnectorsConnectionAdvancedExample|TestAccIntegrationConnectorsManagedZone_integrationConnectorsManagedZoneExample|TestAccIntegrationConnectorsManagedZone_integrationConnectorsManagedZoneExample_update |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest comment is #9617 (comment), commenting to dismiss review request until next update.
* bq table - add geojson support * bq table - add geojson support * CR comments
…latform#10529) Co-authored-by: Sichen Liu <[email protected]>
…0454) Co-authored-by: Khaled Hassan <[email protected]>
Co-authored-by: Luca Prete <[email protected]>
Adding support for Managed zone resource of Integration connectors.
Release Note Template for Downstream PRs (will be copied)
Fixes hashicorp/terraform-provider-google#16762