-
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
Added new resource "Router Nat Address" #11390
Added new resource "Router Nat Address" #11390
Conversation
…ependency-of-compute-router-nat
…ependency-of-compute-router-nat
- WiP custom code for RouterNatAddress;
…ependency-of-compute-router-nat
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.
|
Tests analyticsTotal tests: 0 Click here to see the affected service packages
Tests were added that are skipped in VCR:
View the build log |
…ependency-of-compute-router-nat
- Added diff suppress for natIps when using initialNatIps; - Added RouterNatAddress example and handwritten Test DestroyProducer function;
/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.
|
Tests analyticsTotal tests: 983 Click here to see the affected service packages
Action takenFound 18 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
- Added DiffSuppressFunction to drainNatIps in RouterNat for when initialNatIps is used; - Fixed issues with RouterNatAddress id_format;
…ependency-of-compute-router-nat
- Fixed withAddressCount test using create_before_destroy addresses workaround;
…pply; - Fixed RouterNatAddress tests;
…ependency-of-compute-router-nat
…ependency-of-compute-router-nat
@SarahFrench This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_router_nat" "primary" {
drain_nat_ips = # value needed
}
Resource: resource "google_compute_router_nat_address" "primary" {
drain_nat_ips = # value needed
}
|
Tests analyticsTotal tests: 1007 Click here to see the affected service packages
Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
@GoogleCloudPlatform/terraform-team @SarahFrench This PR has been waiting for review for 1 week. Please take a look! Use the label |
👋 Sorry, I've had a few days off on sick leave. Taking a look at the PR now! |
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.
Left a comment here about the need for a new acceptance test to help us decide between diff suppress funcs and using default_from_api
when implementing these changes
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_router_nat" "primary" {
drain_nat_ips = # value needed
}
Resource: resource "google_compute_router_nat_address" "primary" {
drain_nat_ips = # value needed
}
|
Tests analyticsTotal tests: 1011 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
Thanks! The new test passing shows that users can still change those fields when their use case changes, and from looking at the test debug logs I can see the same resource is being updated (versus being recreated). Given this I'm happy to approve |
Adds new resource
google_compute_router_nat_address
, allowing changes to thenat_ips
anddrain_nat_ips
fields separately from thegoogle_compute_router_nat
resource.Also adds a new field
initial_nat_ip
for resourcegoogle_compute_router_nat
as well, to be used for the required address during creation.Part of: hashicorp/terraform-provider-google#6812
Release Note Template for Downstream PRs (will be copied)