From e496b382717b08089eec42ae21730c130b831cc9 Mon Sep 17 00:00:00 2001 From: Vincent Roseberry Date: Wed, 20 Dec 2017 10:41:49 -0800 Subject: [PATCH 1/2] Proper DiffSuppress for disk image field --- google/resource_compute_disk.go | 21 ++++++++++++++- google/resource_compute_disk_test.go | 39 ++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/google/resource_compute_disk.go b/google/resource_compute_disk.go index af4e7718f15..889eb5a72ff 100644 --- a/google/resource_compute_disk.go +++ b/google/resource_compute_disk.go @@ -65,7 +65,7 @@ func resourceComputeDisk() *schema.Resource { Type: schema.TypeString, Optional: true, ForceNew: true, - DiffSuppressFunc: linkDiffSuppress, + DiffSuppressFunc: diskImageDiffSuppress, }, "project": &schema.Schema{ @@ -407,3 +407,22 @@ func resourceComputeDiskDelete(d *schema.ResourceData, meta interface{}) error { d.SetId("") return nil } + +func diskImageDiffSuppress(_, old, new string, _ *schema.ResourceData) bool { + parts := strings.Split(old, "/") + + if parts[len(parts)-1] == new { + return true + } + + // Handle the "latest" image short hand case: + // "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213" => "debian-cloud/debian-8" + newParts := strings.Split(new, "/") + if len(newParts) == 2 { + if strings.Contains(old, newParts[0]) && strings.Contains(old, newParts[1]) { + return true + } + } + + return false +} diff --git a/google/resource_compute_disk_test.go b/google/resource_compute_disk_test.go index 73dcaaaece8..51957b86dae 100644 --- a/google/resource_compute_disk_test.go +++ b/google/resource_compute_disk_test.go @@ -12,6 +12,45 @@ import ( "google.golang.org/api/compute/v1" ) +func TestDiskImageDiffSuppress(t *testing.T) { + cases := map[string]struct { + Old, New string + ExpectDiffSuppress bool + }{ + "new is a matching short hand": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "debian-cloud/debian-8-jessie-v20171213", + ExpectDiffSuppress: true, + }, + "new is a matching latest short hand": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "debian-cloud/debian-8", + ExpectDiffSuppress: true, + }, + "new is a different self_link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-7-jessie-v20171213", + ExpectDiffSuppress: false, + }, + "new is a different short hand": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "debian-cloud/debian-7-jessie-v20171213", + ExpectDiffSuppress: false, + }, + "new is a different latest short hand": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "debian-cloud/debian-7", + ExpectDiffSuppress: false, + }, + } + + for tn, tc := range cases { + if diskImageDiffSuppress("image", tc.Old, tc.New, nil) != tc.ExpectDiffSuppress { + t.Errorf("bad: %s, %q => %q expect DiffSuppress to return %t", tn, tc.Old, tc.New, tc.ExpectDiffSuppress) + } + } +} + func TestAccComputeDisk_basic(t *testing.T) { t.Parallel() From b00dc0f9c43c5e3f24382a84a372d1b7d3b1c5d0 Mon Sep 17 00:00:00 2001 From: Vincent Roseberry Date: Wed, 20 Dec 2017 16:36:27 -0800 Subject: [PATCH 2/2] Add support for more possible image format --- google/image.go | 31 +++---- google/resource_compute_disk.go | 92 ++++++++++++++++++--- google/resource_compute_disk_test.go | 119 +++++++++++++++++++++++++-- 3 files changed, 210 insertions(+), 32 deletions(-) diff --git a/google/image.go b/google/image.go index cc1f813700f..70253dd0aef 100644 --- a/google/image.go +++ b/google/image.go @@ -15,8 +15,8 @@ const ( ) var ( - resolveImageProjectImage = regexp.MustCompile(fmt.Sprintf("^projects/(%s)/global/images/(%s)$", resolveImageProjectRegex, resolveImageImageRegex)) - resolveImageProjectFamily = regexp.MustCompile(fmt.Sprintf("^projects/(%s)/global/images/family/(%s)$", resolveImageProjectRegex, resolveImageFamilyRegex)) + resolveImageProjectImage = regexp.MustCompile(fmt.Sprintf("projects/(%s)/global/images/(%s)$", resolveImageProjectRegex, resolveImageImageRegex)) + resolveImageProjectFamily = regexp.MustCompile(fmt.Sprintf("projects/(%s)/global/images/family/(%s)$", resolveImageProjectRegex, resolveImageFamilyRegex)) resolveImageGlobalImage = regexp.MustCompile(fmt.Sprintf("^global/images/(%s)$", resolveImageImageRegex)) resolveImageGlobalFamily = regexp.MustCompile(fmt.Sprintf("^global/images/family/(%s)$", resolveImageFamilyRegex)) resolveImageFamilyFamily = regexp.MustCompile(fmt.Sprintf("^family/(%s)$", resolveImageFamilyRegex)) @@ -24,9 +24,22 @@ var ( resolveImageProjectFamilyShorthand = regexp.MustCompile(fmt.Sprintf("^(%s)/(%s)$", resolveImageProjectRegex, resolveImageFamilyRegex)) resolveImageFamily = regexp.MustCompile(fmt.Sprintf("^(%s)$", resolveImageFamilyRegex)) resolveImageImage = regexp.MustCompile(fmt.Sprintf("^(%s)$", resolveImageImageRegex)) - resolveImageLink = regexp.MustCompile(fmt.Sprintf("^https://www.googleapis.com/compute/v1/projects/(%s)/global/images/(%s)", resolveImageProjectRegex, resolveImageImageRegex)) + resolveImageLink = regexp.MustCompile(fmt.Sprintf("^https://www.googleapis.com/compute/[a-z0-9]+/projects/(%s)/global/images/(%s)", resolveImageProjectRegex, resolveImageImageRegex)) ) +// built-in projects to look for images/families containing the string +// on the left in +var imageMap = map[string]string{ + "centos": "centos-cloud", + "coreos": "coreos-cloud", + "debian": "debian-cloud", + "opensuse": "opensuse-cloud", + "rhel": "rhel-cloud", + "sles": "suse-cloud", + "ubuntu": "ubuntu-os-cloud", + "windows": "windows-cloud", +} + func resolveImageImageExists(c *Config, project, name string) (bool, error) { if _, err := c.clientCompute.Images.Get(project, name).Do(); err == nil { return true, nil @@ -68,18 +81,6 @@ func sanityTestRegexMatches(expected int, got []string, regexType, name string) // If not, check if it's a family in the current project. If it is, return it as global/images/family/{family}. // If not, check if it could be a GCP-provided family, and if it exists. If it does, return it as projects/{project}/global/images/family/{family} func resolveImage(c *Config, project, name string) (string, error) { - // built-in projects to look for images/families containing the string - // on the left in - imageMap := map[string]string{ - "centos": "centos-cloud", - "coreos": "coreos-cloud", - "debian": "debian-cloud", - "opensuse": "opensuse-cloud", - "rhel": "rhel-cloud", - "sles": "suse-cloud", - "ubuntu": "ubuntu-os-cloud", - "windows": "windows-cloud", - } var builtInProject string for k, v := range imageMap { if strings.Contains(name, k) { diff --git a/google/resource_compute_disk.go b/google/resource_compute_disk.go index 889eb5a72ff..39465800590 100644 --- a/google/resource_compute_disk.go +++ b/google/resource_compute_disk.go @@ -408,21 +408,93 @@ func resourceComputeDiskDelete(d *schema.ResourceData, meta interface{}) error { return nil } +// We cannot suppress the diff for the case when family name is not part of the image name since we can't +// make a network call in a DiffSuppressFunc. func diskImageDiffSuppress(_, old, new string, _ *schema.ResourceData) bool { - parts := strings.Split(old, "/") + // 'old' is read from the API. + // It always has the format 'https://www.googleapis.com/compute/v1/projects/(%s)/global/images/(%s)' + matches := resolveImageLink.FindStringSubmatch(old) + if matches == nil { + // Image read from the API doesn't have the expected format. In practice, it should never happen + return false + } + oldProject := matches[1] + oldName := matches[2] - if parts[len(parts)-1] == new { - return true + // Partial or full self link family + if resolveImageProjectFamily.MatchString(new) { + // Value matches pattern "projects/{project}/global/images/family/{family-name}$" + matches := resolveImageProjectFamily.FindStringSubmatch(new) + newProject := matches[1] + newFamilyName := matches[2] + + return diskImageProjectNameEquals(oldProject, oldName, newProject, newFamilyName) } - // Handle the "latest" image short hand case: - // "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213" => "debian-cloud/debian-8" - newParts := strings.Split(new, "/") - if len(newParts) == 2 { - if strings.Contains(old, newParts[0]) && strings.Contains(old, newParts[1]) { - return true - } + // Partial or full self link image + if resolveImageProjectImage.MatchString(new) { + // Value matches pattern "projects/{project}/global/images/{image-name}$" + // or "projects/{project}/global/images/{image-name-latest}$" + matches := resolveImageProjectImage.FindStringSubmatch(new) + newProject := matches[1] + newImageName := matches[2] + + return diskImageProjectNameEquals(oldProject, oldName, newProject, newImageName) + } + + // Partial link without project family + if resolveImageGlobalFamily.MatchString(new) { + // Value is "global/images/family/{family-name}" + matches := resolveImageGlobalFamily.FindStringSubmatch(new) + familyName := matches[1] + + return strings.Contains(oldName, familyName) + } + + // Partial link without project image + if resolveImageGlobalImage.MatchString(new) { + // Value is "global/images/family/{image-name}" or "global/images/family/{image-name-latest}" + matches := resolveImageGlobalImage.FindStringSubmatch(new) + imageName := matches[1] + + return strings.Contains(oldName, imageName) + } + + // Family shorthand + if resolveImageFamilyFamily.MatchString(new) { + // Value is "family/{family-name}" + matches := resolveImageFamilyFamily.FindStringSubmatch(new) + familyName := matches[1] + + return strings.Contains(oldName, familyName) + } + + // Shorthand for image + if resolveImageProjectImageShorthand.MatchString(new) { + // Value is "{project}/{image-name}" or "{project}/{image-name-latest}" + matches := resolveImageProjectImageShorthand.FindStringSubmatch(new) + newProject := matches[1] + newName := matches[2] + + return diskImageProjectNameEquals(oldProject, oldName, newProject, newName) + } + + // Image or family only + if strings.Contains(oldName, new) { + // Value is "{image-name}" or "{family-name}" or "{image-name-latest}" + return true } return false } + +func diskImageProjectNameEquals(project1, name1, project2, name2 string) bool { + // Convert short project name to full name + // For instance, centos => centos-cloud + fullProjectName, ok := imageMap[project2] + if ok { + project2 = fullProjectName + } + + return project1 == project2 && strings.Contains(name1, name2) +} diff --git a/google/resource_compute_disk_test.go b/google/resource_compute_disk_test.go index 51957b86dae..f6afb5c9c8d 100644 --- a/google/resource_compute_disk_test.go +++ b/google/resource_compute_disk_test.go @@ -17,31 +17,136 @@ func TestDiskImageDiffSuppress(t *testing.T) { Old, New string ExpectDiffSuppress bool }{ - "new is a matching short hand": { + // Full & partial links + "matching self_link with different api version": { + Old: "https://www.googleapis.com/compute/beta/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + ExpectDiffSuppress: true, + }, + "matching image partial self_link": { Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", - New: "debian-cloud/debian-8-jessie-v20171213", + New: "projects/debian-cloud/global/images/debian-8-jessie-v20171213", ExpectDiffSuppress: true, }, - "new is a matching latest short hand": { + "matching image partial no project self_link": { Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", - New: "debian-cloud/debian-8", + New: "global/images/debian-8-jessie-v20171213", ExpectDiffSuppress: true, }, - "new is a different self_link": { + "different image self_link": { Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", New: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-7-jessie-v20171213", ExpectDiffSuppress: false, }, - "new is a different short hand": { + "different image partial self_link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "projects/debian-cloud/global/images/debian-7-jessie-v20171213", + ExpectDiffSuppress: false, + }, + "different image partial no project self_link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "global/images/debian-7-jessie-v20171213", + ExpectDiffSuppress: false, + }, + // Image name + "matching image name": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "debian-8-jessie-v20171213", + ExpectDiffSuppress: true, + }, + "different image name": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "debian-7-jessie-v20171213", + ExpectDiffSuppress: false, + }, + // Image short hand + "matching image short hand": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "debian-cloud/debian-8-jessie-v20171213", + ExpectDiffSuppress: true, + }, + "matching image short hand but different project": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "different-cloud/debian-8-jessie-v20171213", + ExpectDiffSuppress: false, + }, + "different image short hand": { Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", New: "debian-cloud/debian-7-jessie-v20171213", ExpectDiffSuppress: false, }, - "new is a different latest short hand": { + // Latest image short hand + "matching latest image short hand": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "debian-cloud/debian-8", + ExpectDiffSuppress: true, + }, + "matching latest image short hand with project short name": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "debian/debian-8", + ExpectDiffSuppress: true, + }, + "matching latest image short hand but different project": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "different-cloud/debian-8", + ExpectDiffSuppress: false, + }, + "different latest image short hand": { Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", New: "debian-cloud/debian-7", ExpectDiffSuppress: false, }, + // Image Family + "matching image family": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "family/debian-8", + ExpectDiffSuppress: true, + }, + "matching image family self link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/family/debian-8", + ExpectDiffSuppress: true, + }, + "matching image family partial self link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "projects/debian-cloud/global/images/family/debian-8", + ExpectDiffSuppress: true, + }, + "matching image family partial no project self link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "global/images/family/debian-8", + ExpectDiffSuppress: true, + }, + "different image family": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "family/debian-7", + ExpectDiffSuppress: false, + }, + "different image family self link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/family/debian-7", + ExpectDiffSuppress: false, + }, + "different image family partial self link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "projects/debian-cloud/global/images/family/debian-7", + ExpectDiffSuppress: false, + }, + "different image family partial no project self link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "global/images/family/debian-7", + ExpectDiffSuppress: false, + }, + "matching image family but different project in self link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "https://www.googleapis.com/compute/v1/projects/other-cloud/global/images/family/debian-8", + ExpectDiffSuppress: false, + }, + "different image family but different project in partial self link": { + Old: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20171213", + New: "projects/other-cloud/global/images/family/debian-8", + ExpectDiffSuppress: false, + }, } for tn, tc := range cases {