Addressed Riley's comments

hashicorp · Jul 28, 2017 · 9e95795 · 9e95795
1 parent 4441908
commit 9e95795
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 119 deletions.
diff --git a/google/resource_compute_network_peering.go b/google/resource_compute_network_peering.go
@@ -2,11 +2,12 @@ package google
 
 import (
 	"fmt"
+	"log"
+	"regexp"
+
 	"github.com/hashicorp/terraform/helper/schema"
 	"google.golang.org/api/compute/v1"
 	"google.golang.org/api/googleapi"
-	"log"
-	"regexp"
 )
 
 const peerNetworkLinkRegex = "projects/(" + ProjectRegex + ")/global/networks/((?:[a-z](?:[-a-z0-9]*[a-z0-9])?))$"
@@ -29,14 +30,14 @@ func resourceComputeNetworkPeering() *schema.Resource {
 				Required:         true,
 				ForceNew:         true,
 				ValidateFunc:     validateRegexp(peerNetworkLinkRegex),
-				DiffSuppressFunc: peerNetworkLinkDiffSuppress,
+				DiffSuppressFunc: compareSelfLinkRelativePaths,
 			},
 			"peer_network": &schema.Schema{
 				Type:             schema.TypeString,
 				Required:         true,
 				ForceNew:         true,
 				ValidateFunc:     validateRegexp(peerNetworkLinkRegex),
-				DiffSuppressFunc: peerNetworkLinkDiffSuppress,
+				DiffSuppressFunc: compareSelfLinkRelativePaths,
 			},
 			"auto_create_routes": &schema.Schema{
 				Type:     schema.TypeBool,
@@ -59,15 +60,34 @@ func resourceComputeNetworkPeering() *schema.Resource {
 func resourceComputeNetworkPeeringCreate(d *schema.ResourceData, meta interface{}) error {
 	config := meta.(*Config)
 
-	err := addPeering(config, d)
+	project, err := getProject(d, config)
 	if err != nil {
 		return err
 	}
 
-	peeringName := d.Get("name").(string)
-	networkName := getNameFromNetworkLink(d.Get("network").(string))
+	name := d.Get("name").(string)
+	networkLink := d.Get("network").(string)
+	peerNetworkLink := d.Get("peer_network").(string)
+	autoCreateRoutes := d.Get("auto_create_routes").(bool)
+	networkName := getNameFromNetworkLink(networkLink)
+
+	request := &compute.NetworksAddPeeringRequest{
+		Name:             name,
+		PeerNetwork:      peerNetworkLink,
+		AutoCreateRoutes: autoCreateRoutes,
+	}
+
+	addOp, err := config.clientCompute.Networks.AddPeering(project, networkName, request).Do()
+	if err != nil {
+		return fmt.Errorf("Error adding network peering: %s", err)
+	}
 
-	d.SetId(fmt.Sprintf("%s/%s", networkName, peeringName))
+	err = computeOperationWait(config, addOp, project, "Adding Network Peering")
+	if err != nil {
+		return err
+	}
+
+	d.SetId(fmt.Sprintf("%s/%s", networkName, name))
 
 	return resourceComputeNetworkPeeringRead(d, meta)
 }
@@ -96,8 +116,6 @@ func resourceComputeNetworkPeeringRead(d *schema.ResourceData, meta interface{})
 		return nil
 	}
 
-	// No need to set the `name` and `network` fields. We use both of them to find the peering.
-	// If they change on GCP, we wouldn't have been able to find the peering in the first place.
 	d.Set("peer_network", peering.Network)
 	d.Set("auto_create_routes", peering.AutoCreateRoutes)
 	d.Set("state", peering.State)
@@ -110,55 +128,6 @@ func resourceComputeNetworkPeeringDelete(d *schema.ResourceData, meta interface{
 	config := meta.(*Config)
 
 	// Remove the `network` to `peer_network` peering
-	err := removePeering(config, d)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
-func findPeeringFromNetwork(network *compute.Network, peeringName string) *compute.NetworkPeering {
-	for _, p := range network.Peerings {
-		if p.Name == peeringName {
-			return p
-		}
-	}
-	return nil
-}
-
-func addPeering(config *Config, d *schema.ResourceData) error {
-	project, err := getProject(d, config)
-	if err != nil {
-		return err
-	}
-
-	name := d.Get("name").(string)
-	networkLink := d.Get("network").(string)
-	peerNetworkLink := d.Get("peer_network").(string)
-	autoCreateRoutes := d.Get("auto_create_routes").(bool)
-	networkName := getNameFromNetworkLink(networkLink)
-
-	request := &compute.NetworksAddPeeringRequest{
-		Name:             name,
-		PeerNetwork:      peerNetworkLink,
-		AutoCreateRoutes: autoCreateRoutes,
-	}
-
-	addOp, err := config.clientCompute.Networks.AddPeering(project, networkName, request).Do()
-	if err != nil {
-		return fmt.Errorf("Error adding network peering: %s", err)
-	}
-
-	err = computeOperationWait(config, addOp, project, "Adding Network Peering")
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
-func removePeering(config *Config, d *schema.ResourceData) error {
 	project, err := getProject(d, config)
 	if err != nil {
 		return err
@@ -196,34 +165,22 @@ func removePeering(config *Config, d *schema.ResourceData) error {
 	return nil
 }
 
+func findPeeringFromNetwork(network *compute.Network, peeringName string) *compute.NetworkPeering {
+	for _, p := range network.Peerings {
+		if p.Name == peeringName {
+			return p
+		}
+	}
+	return nil
+}
+
 func getNameFromNetworkLink(network string) string {
 	r := regexp.MustCompile(peerNetworkLinkRegex)
 
 	m := r.FindStringSubmatch(network)
 	return m[2]
 }
 
-func peerNetworkLinkDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
-	r := regexp.MustCompile(peerNetworkLinkRegex)
-
-	m := r.FindStringSubmatch(old)
-	if len(m) != 3 {
-		return false
-	}
-	oldProject, oldPeeringNetworkName := m[1], m[2]
-
-	m = r.FindStringSubmatch(new)
-	if len(m) != 3 {
-		return false
-	}
-	newProject, newPeeringNetworkName := m[1], m[2]
-
-	if oldProject == newProject && oldPeeringNetworkName == newPeeringNetworkName {
-		return true
-	}
-	return false
-}
-
 func getNetworkPeeringLockName(networkName, peerNetworkName string) string {
 	return fmt.Sprintf("network_peering/%s/%s", networkName, peerNetworkName)
 }
diff --git a/google/resource_compute_network_peering_test.go b/google/resource_compute_network_peering_test.go
@@ -97,25 +97,25 @@ func testAccCheckComputeNetworkPeeringAutoCreateRoutes(v bool, peering *compute.
 
 var testAccComputeNetworkPeering_basic = fmt.Sprintf(`
 resource "google_compute_network" "network1" {
-  name = "network-test-1-%s"
-  auto_create_subnetworks = false
+	name = "network-test-1-%s"
+	auto_create_subnetworks = false
 }
 
 resource "google_compute_network" "network2" {
-  name = "network-test-2-%s"
-  auto_create_subnetworks = false
+	name = "network-test-2-%s"
+	auto_create_subnetworks = false
 }
 
 resource "google_compute_network_peering" "foo" {
-  name = "peering-test-1-%s"
-  network = "${google_compute_network.network1.self_link}"
-  peer_network = "${google_compute_network.network2.self_link}"
+	name = "peering-test-1-%s"
+	network = "${google_compute_network.network1.self_link}"
+	peer_network = "${google_compute_network.network2.self_link}"
 }
 
 resource "google_compute_network_peering" "bar" {
-  name = "peering-test-2-%s"
-  auto_create_routes = true
-  network = "${google_compute_network.network2.self_link}"
-  peer_network = "${google_compute_network.network1.self_link}"
+	name = "peering-test-2-%s"
+	auto_create_routes = true
+	network = "${google_compute_network.network2.self_link}"
+	peer_network = "${google_compute_network.network1.self_link}"
 }
 `, acctest.RandString(10), acctest.RandString(10), acctest.RandString(10), acctest.RandString(10))
diff --git a/website/docs/r/compute_network_peering.html.markdown b/website/docs/r/compute_network_peering.html.markdown
@@ -8,23 +8,19 @@ description: |-
 
 # google\_compute\_network\_peering
 
-Manages a network peering within GCE.
+Manages a network peering within GCE. For more information see
+[the official documentation](https://cloud.google.com/compute/docs/vpc/vpc-peering)
+and
+[API](https://cloud.google.com/compute/docs/reference/latest/networks).
 
-## Example Usage
+~> **Note:** Both network must create a peering with each other for the peering to be functional.
 
-```hcl
-resource "google_compute_network" "default" {
-  name                    = "foobar"
-  auto_create_subnetworks = "false"
-}
+~> **Note:** Subnets IP ranges across peered VPC networks cannot overlap.
 
-resource "google_compute_network" "other" {
-  name                    = "other"
-  auto_create_subnetworks = "false"
-}
+## Example Usage
 
-// Both network must create a peering with each other for the peering
-// to be functional.
+```hcl
+// 
 resource "google_compute_network_peering" "peering1" {
   name = "peering1"
   network = "${google_compute_network.default.self_link}"
@@ -37,19 +33,14 @@ resource "google_compute_network_peering" "peering2" {
   peer_network = "${google_compute_network.default.self_link}"
 }
 
-// Subnets IP ranges across peered VPC networks cannot overlap.
-resource "google_compute_subnetwork" "network1-subnet1" {
-  name = "network1-sub1"
-  ip_cidr_range = "10.128.0.0/20"  
-  network = "${google_compute_network.network1.self_link}"
-  region = "us-east1"
+resource "google_compute_network" "default" {
+  name                    = "foobar"
+  auto_create_subnetworks = "false"
 }
 
-resource "google_compute_subnetwork" "network2-subnet1" {
-  name = "network1-sub2"
-  ip_cidr_range = "10.132.0.0/20"  
-  network = "${google_compute_network.network2.self_link}"
-  region = "us-central1"
+resource "google_compute_network" "other" {
+  name                    = "other"
+  auto_create_subnetworks = "false"
 }
 ```
 
@@ -63,14 +54,14 @@ The following arguments are supported:
 
 * `peer_network` - (Required) Resource link of the peer network.
 
-* `auto_create_routes` - (Optional) If set to true, the routes between the two networks will
-  be created and managed automatically. Defaults to true.
+* `auto_create_routes` - (Optional) If set to `true`, the routes between the two networks will
+  be created and managed automatically. Defaults to `true`.
 
 ## Attributes Reference
 
 In addition to the arguments listed above, the following computed attributes are
 exported:
 
-* `state` - (Computed) State for the peering.
+* `state` - State for the peering.
 
-* `state_details` - (Computed) Details about the current state of the peering.
+* `state_details` - Details about the current state of the peering.