Skip to content
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

Diff suppress unconventional public image family naming pattern #1024

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions google/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,23 @@ var (
resolveImageFamily = regexp.MustCompile(fmt.Sprintf("^(%s)$", resolveImageFamilyRegex))
resolveImageImage = regexp.MustCompile(fmt.Sprintf("^(%s)$", resolveImageImageRegex))
resolveImageLink = regexp.MustCompile(fmt.Sprintf("^https://www.googleapis.com/compute/[a-z0-9]+/projects/(%s)/global/images/(%s)", ProjectRegex, resolveImageImageRegex))

windowsSqlImage = regexp.MustCompile("^sql-([0-9]{4})-([a-z]+)-windows-([0-9]{4})(?:-r([0-9]+))?-dc-v[0-9]+$")
canonicalUbuntuLtsImage = regexp.MustCompile("^ubuntu-([0-9]+)-")
)

// 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",
"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",
"windows-sql": "windows-sql-cloud",
}

func resolveImageImageExists(c *Config, project, name string) (bool, error) {
Expand Down
103 changes: 96 additions & 7 deletions google/resource_compute_disk.go
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ func diskImageDiffSuppress(_, old, new string, _ *schema.ResourceData) bool {
newProject := matches[1]
newFamilyName := matches[2]

return diskImageProjectNameEquals(oldProject, newProject) && strings.Contains(oldName, newFamilyName)
return diskImageProjectNameEquals(oldProject, newProject) && diskImageFamilyEquals(oldName, newFamilyName)
}

// Partial or full self link image
Expand All @@ -436,7 +436,7 @@ func diskImageDiffSuppress(_, old, new string, _ *schema.ResourceData) bool {
newProject := matches[1]
newImageName := matches[2]

return diskImageProjectNameEquals(oldProject, newProject) && strings.Contains(oldName, newImageName)
return diskImageProjectNameEquals(oldProject, newProject) && diskImageEquals(oldName, newImageName)
}

// Partial link without project family
Expand All @@ -445,7 +445,7 @@ func diskImageDiffSuppress(_, old, new string, _ *schema.ResourceData) bool {
matches := resolveImageGlobalFamily.FindStringSubmatch(new)
familyName := matches[1]

return strings.Contains(oldName, familyName)
return diskImageFamilyEquals(oldName, familyName)
}

// Partial link without project image
Expand All @@ -454,7 +454,7 @@ func diskImageDiffSuppress(_, old, new string, _ *schema.ResourceData) bool {
matches := resolveImageGlobalImage.FindStringSubmatch(new)
imageName := matches[1]

return strings.Contains(oldName, imageName)
return diskImageEquals(oldName, imageName)
}

// Family shorthand
Expand All @@ -463,7 +463,7 @@ func diskImageDiffSuppress(_, old, new string, _ *schema.ResourceData) bool {
matches := resolveImageFamilyFamily.FindStringSubmatch(new)
familyName := matches[1]

return strings.Contains(oldName, familyName)
return diskImageFamilyEquals(oldName, familyName)
}

// Shorthand for image or family
Expand All @@ -473,11 +473,12 @@ func diskImageDiffSuppress(_, old, new string, _ *schema.ResourceData) bool {
newProject := matches[1]
newName := matches[2]

return diskImageProjectNameEquals(oldProject, newProject) && strings.Contains(oldName, newName)
return diskImageProjectNameEquals(oldProject, newProject) &&
(diskImageEquals(oldName, newName) || diskImageFamilyEquals(oldName, newName))
}

// Image or family only
if strings.Contains(oldName, new) {
if diskImageEquals(oldName, new) || diskImageFamilyEquals(oldName, new) {
// Value is "{image-name}" or "{family-name}"
return true
}
Expand All @@ -495,3 +496,91 @@ func diskImageProjectNameEquals(project1, project2 string) bool {

return project1 == project2
}

func diskImageEquals(oldImageName, newImageName string) bool {
return strings.Contains(oldImageName, newImageName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the case where it's the image name that's a substring of the other one (as opposed to the family name that's a substring of the image name)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, could this be oldImageName == newImageName?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see :) Yes! Now that I added a specific one for for testing equality for family.

Done. I kept the diskImageEquals method even if it is trivial. I find that it reads better in diskImageDiffSuppress.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, thanks :)

}

func diskImageFamilyEquals(imageName, familyName string) bool {
// Handles the case when the image name includes the family name
// e.g. image name: debian-9-drawfork-v20180109, family name: debian-9
if strings.Contains(imageName, familyName) {
return true
}

if suppressCanonicalFamilyDiff(imageName, familyName) {
return true
}

if suppressWindowsSqlFamilyDiff(imageName, familyName) {
return true
}

if suppressWindowsFamilyDiff(imageName, familyName) {
return true
}

return false
}

// e.g. image: ubuntu-1404-trusty-v20180122, family: ubuntu-1404-lts
func suppressCanonicalFamilyDiff(imageName, familyName string) bool {
parts := canonicalUbuntuLtsImage.FindStringSubmatch(imageName)
if len(parts) == 2 {
f := fmt.Sprintf("ubuntu-%s-lts", parts[1])
if f == familyName {
return true
}
}

return false
}

// e.g. image: sql-2017-standard-windows-2016-dc-v20180109, family: sql-std-2017-win-2016
// e.g. image: sql-2017-express-windows-2012-r2-dc-v20180109, family: sql-exp-2017-win-2012-r2
func suppressWindowsSqlFamilyDiff(imageName, familyName string) bool {
parts := windowsSqlImage.FindStringSubmatch(imageName)
if len(parts) == 5 {
edition := parts[2] // enterprise, standard or web.
sqlVersion := parts[1]
windowsVersion := parts[3]

// Translate edition
switch edition {
case "enterprise":
edition = "ent"
case "standard":
edition = "std"
case "express":
edition = "exp"
}

var f string
if revision := parts[4]; revision != "" {
// With revision
f = fmt.Sprintf("sql-%s-%s-win-%s-r%s", edition, sqlVersion, windowsVersion, revision)
} else {
// No revision
f = fmt.Sprintf("sql-%s-%s-win-%s", edition, sqlVersion, windowsVersion)
}

if f == familyName {
return true
}
}

return false
}

// e.g. image: windows-server-1709-dc-core-v20180109, family: windows-1709-core
// e.g. image: windows-server-1709-dc-core-for-containers-v20180109, family: "windows-1709-core-for-containers
func suppressWindowsFamilyDiff(imageName, familyName string) bool {
updatedFamilyString := strings.Replace(familyName, "windows-", "windows-server-", 1)
updatedFamilyString = strings.Replace(updatedFamilyString, "-core", "-dc-core", 1)

if strings.Contains(imageName, updatedFamilyString) {
return true
}

return false
}
45 changes: 45 additions & 0 deletions google/resource_compute_disk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
"google.golang.org/api/compute/v1"
"os"
)

func TestDiskImageDiffSuppress(t *testing.T) {
Expand Down Expand Up @@ -86,11 +87,21 @@ func TestDiskImageDiffSuppress(t *testing.T) {
New: "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/family/debian-8",
ExpectDiffSuppress: true,
},
"matching unconventional image family self link": {
Old: "https://www.googleapis.com/compute/v1/projects/ubuntu-os-cloud/global/images/ubuntu-1404-trusty-v20180122",
New: "https://www.googleapis.com/compute/v1/projects/projects/ubuntu-os-cloud/global/images/family/ubuntu-1404-lts",
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 unconventional image family partial self link": {
Old: "https://www.googleapis.com/compute/v1/projects/ubuntu-os-cloud/global/images/ubuntu-1404-trusty-v20180122",
New: "projects/ubuntu-os-cloud/global/images/family/ubuntu-1404-lts",
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",
Expand All @@ -106,6 +117,11 @@ func TestDiskImageDiffSuppress(t *testing.T) {
New: "debian/debian-8",
ExpectDiffSuppress: true,
},
"matching unconventional image family short hand": {
Old: "https://www.googleapis.com/compute/v1/projects/ubuntu-os-cloud/global/images/ubuntu-1404-trusty-v20180122",
New: "ubuntu-os-cloud/ubuntu-1404-lts",
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",
Expand Down Expand Up @@ -155,6 +171,35 @@ func TestDiskImageDiffSuppress(t *testing.T) {
}
}

// Test that all the naming pattern for public images are supported.
func TestAccComputeDisk_imageDiffSuppressPublicVendorsFamilyNames(t *testing.T) {
t.Parallel()

if os.Getenv(resource.TestEnvVar) == "" {
t.Skip(fmt.Sprintf("Network access not allowed; use %s=1 to enable", resource.TestEnvVar))
}

config := getInitializedConfig(t)

for _, publicImageProject := range imageMap {
token := ""
for paginate := true; paginate; {
resp, err := config.clientCompute.Images.List(publicImageProject).Filter("deprecated.replacement ne .*images.*").PageToken(token).Do()
if err != nil {
t.Fatalf("Can't list public images for project %q", publicImageProject)
}

for _, image := range resp.Items {
if !diskImageDiffSuppress("image", image.SelfLink, "family/"+image.Family, nil) {
t.Errorf("should suppress diff for image %q and family %q", image.SelfLink, image.Family)
}
}
token := resp.NextPageToken
paginate = token != ""
}
}
}

func TestAccComputeDisk_basic(t *testing.T) {
t.Parallel()

Expand Down
10 changes: 8 additions & 2 deletions website/docs/r/compute_disk.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,14 @@ The following arguments are supported:
to encrypt this disk.

* `image` - (Optional) The image from which to initialize this disk. This can be
one of: the image's `self_link`, of a full name and version, e.g.
`debian-8-jessie-v20170523`
one of: the image's `self_link`, `projects/{project}/global/images/{image}`,
`projects/{project}/global/images/family/{family}`, `global/images/{image}`,
`global/images/family/{family}`, `family/{family}`, `{project}/{family}`,
`{project}/{image}`, `{family}`, or `{image}`. If referred by family, the
images names must include the family name. If they don't, use the
[google_compute_image data source](/docs/providers/google/d/datasource_compute_image.html).
For instance, the image `centos-6-v20180104` includes its family name `centos-6`.
These images can be referred by family name here.

* `project` - (Optional) The project in which the resource belongs. If it
is not provided, the provider project is used.
Expand Down
7 changes: 4 additions & 3 deletions website/docs/r/compute_instance.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,10 @@ The `initialize_params` block supports:
`projects/{project}/global/images/family/{family}`, `global/images/{image}`,
`global/images/family/{family}`, `family/{family}`, `{project}/{family}`,
`{project}/{image}`, `{family}`, or `{image}`. If referred by family, the
images names must include the family name. For instance, the image
`centos-6-v20180104` includes its family name `centos-6`. These images can
be referred by family name here.
images names must include the family name. If they don't, use the
[google_compute_image data source](/docs/providers/google/d/datasource_compute_image.html).
For instance, the image `centos-6-v20180104` includes its family name `centos-6`.
These images can be referred by family name here.

The `scratch_disk` block supports:

Expand Down