From bfde91ecb1ddb2386cf7b37215b29c453d7d9d53 Mon Sep 17 00:00:00 2001 From: Paddy Date: Mon, 23 Jan 2017 16:45:06 -0800 Subject: [PATCH 1/5] Start adding tests for image resolution. Add tests that show what we want image input strings to resolve to, so we can test that behaviour. --- image_test.go | 61 ++++++++++++++++++++++++++++++++++++++++ resource_compute_disk.go | 1 + 2 files changed, 62 insertions(+) create mode 100644 image_test.go diff --git a/image_test.go b/image_test.go new file mode 100644 index 00000000..f500c9a4 --- /dev/null +++ b/image_test.go @@ -0,0 +1,61 @@ +package google + +import ( + "testing" + + "github.com/hashicorp/terraform/helper/resource" +) + +func TestAccComputeImage_resolveImage(t *testing.T) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeImageDestroy, + Steps: []resource.TestStep{ + { + Config: testAccComputeImage_basedondisk, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeImageExists( + "google_compute_image.foobar", &image), + ), + }, + }, + }) + images := map[string]string{ + "family/debian-8": "projects/debian-cloud/global/images/family/debian-8-jessie", + "projects/debian-cloud/global/images/debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", + "debian-8-jessie": "projects/debian-cloud/global/images/family/debian-8-jessie", + "debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", + "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110": "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110", + + // TODO(paddy): we need private images/families here to actually test this + "global/images/my-private-image": "global/images/my-private-image", + "global/images/family/my-private-family": "global/images/family/my-private-family", + "my-private-image": "global/images/my-private-image", + "my-private-family": "global/images/family/my-private-family", + "my-project/my-private-image": "projects/my-project/global/images/my-private-image", + "my-project/my-private-family": "projects/my-project/global/images/family/my-private-family", + "insert-URL-here": "insert-URL-here", + } + config := &Config{ + Credentials: credentials, + Project: project, + Region: region, + } + + err := config.loadAndValidate() + if err != nil { + t.Fatalf("Error loading config: %s\n", err) + } + for input, expectation := range images { + result, err := resolveImage(config, input) + if err != nil { + t.Errorf("Error resolving input %s to image: %+v\n", input, err) + continue + } + if result != expectation { + t.Errorf("Expected input '%s' to resolve to '%s', it resolved to '%s' instead.\n", input, expectation, result) + continue + } + } +} diff --git a/resource_compute_disk.go b/resource_compute_disk.go index c8ef8007..94d23d34 100644 --- a/resource_compute_disk.go +++ b/resource_compute_disk.go @@ -112,6 +112,7 @@ func resourceComputeDiskCreate(d *schema.ResourceData, meta interface{}) error { } disk.SourceImage = imageUrl + log.Printf("[DEBUG] Image name resolved to: %s", imageUrl) } if v, ok := d.GetOk("type"); ok { From e55e5e9119f1c3c4ca8a47010e82edb20c902ad9 Mon Sep 17 00:00:00 2001 From: Paddy Date: Thu, 23 Feb 2017 21:55:30 -0800 Subject: [PATCH 2/5] provider/google: update image resolution code. Add tests that ensure that image syntax resolves to API input the way we want it to. Add a lot of different input forms for images, to more closely map to what the API accepts, so anything that's valid input to the API should also be valid input in a config. Stop resolving image families to specific image URLs, allowing things like instance templates to evolve over time as new images are pushed. --- image.go | 244 +++++++++++++++++++++++++++++++++----------------- image_test.go | 112 ++++++++++++++++------- 2 files changed, 242 insertions(+), 114 deletions(-) diff --git a/image.go b/image.go index e4a50905..912821b5 100644 --- a/image.go +++ b/image.go @@ -2,96 +2,178 @@ package google import ( "fmt" + "regexp" "strings" + + "google.golang.org/api/googleapi" ) +const ( + resolveImageProjectRegex = "[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?" // TODO(paddy): this isn't based on any documentation; we're just copying the image name restrictions. Need to follow up with @danawillow and/or @evandbrown and see if there's an actual limit to this + resolveImageFamilyRegex = "[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?" // TODO(paddy): this isn't based on any documentation; we're just copying the image name restrictions. Need to follow up with @danawillow and/or @evandbrown and see if there's an actual limit to this + resolveImageImageRegex = "[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?" // 1-63 characters, lowercase letters, numbers, and hyphens only, beginning and ending in a lowercase letter or number +) + +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)) + 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)) + resolveImageProjectImageShorthand = regexp.MustCompile(fmt.Sprintf("^(%s)/(%s)$", resolveImageProjectRegex, resolveImageImageRegex)) + 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)) +) + +func resolveImageImageExists(c *Config, project, name string) (bool, error) { + if _, err := c.clientCompute.Images.Get(project, name).Do(); err == nil { + return true, nil + } else if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 { + return false, nil + } else { + return false, fmt.Errorf("Error checking if image %s exists: %s", name, err) + } +} + +func resolveImageFamilyExists(c *Config, project, name string) (bool, error) { + if _, err := c.clientCompute.Images.GetFromFamily(project, name).Do(); err == nil { + return true, nil + } else if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 { + return false, nil + } else { + return false, fmt.Errorf("Error checking if family %s exists: %s", name, err) + } +} + // If the given name is a URL, return it. // If it is of the form project/name, search the specified project first, then // search image families in the specified project. // If it is of the form name then look in the configured project, then hosted // image projects, and lastly at image families in hosted image projects. func resolveImage(c *Config, name string) (string, error) { - - if strings.HasPrefix(name, "https://www.googleapis.com/compute/v1/") { - return name, nil - - } else { - splitName := strings.Split(name, "/") - if len(splitName) == 1 { - - // Must infer the project name: - - // First, try the configured project for a specific image: - image, err := c.clientCompute.Images.Get(c.Project, name).Do() - if err == nil { - return image.SelfLink, nil - } - - // If it doesn't exist, try to see if it works as an image family: - image, err = c.clientCompute.Images.GetFromFamily(c.Project, name).Do() - if err == nil { - return image.SelfLink, nil - } - - // If we match a lookup for an alternate project, then try that next. - // If not, we return the original error. - - // If the image name contains the left hand side, we use the project from - // the right hand side. - 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 project string - for k, v := range imageMap { - if strings.Contains(name, k) { - project = v - break - } - } - if project == "" { - return "", err - } - - // There was a match, but the image still may not exist, so check it: - image, err = c.clientCompute.Images.Get(project, name).Do() - if err == nil { - return image.SelfLink, nil - } - - // If it doesn't exist, try to see if it works as an image family: - image, err = c.clientCompute.Images.GetFromFamily(project, name).Do() - if err == nil { - return image.SelfLink, nil - } - - return "", err - - } else if len(splitName) == 2 { - - // Check if image exists in the specified project: - image, err := c.clientCompute.Images.Get(splitName[0], splitName[1]).Do() - if err == nil { - return image.SelfLink, nil - } - - // If it doesn't, check if it exists as an image family: - image, err = c.clientCompute.Images.GetFromFamily(splitName[0], splitName[1]).Do() - if err == nil { - return image.SelfLink, nil - } - - return "", err - - } else { - return "", fmt.Errorf("Invalid image name, require URL, project/name, or just name: %s", name) + // 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) { + builtInProject = v + break } } - + switch { + case resolveImageLink.MatchString(name): // https://www.googleapis.com/compute/v1/projects/xyz/global/images/xyz + return name, nil + case resolveImageProjectImage.MatchString(name): // projects/xyz/global/images/xyz + res := resolveImageProjectImage.FindStringSubmatch(name) + if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d project image regex matches, got %d for %s", 2, len(res)-1, name) + } + return fmt.Sprintf("projects/%s/global/images/%s", res[1], res[2]), nil + case resolveImageProjectFamily.MatchString(name): // projects/xyz/global/images/family/xyz + res := resolveImageProjectFamily.FindStringSubmatch(name) + if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d project family regex matches, got %d for %s", 2, len(res)-1, name) + } + return fmt.Sprintf("projects/%s/global/images/family/%s", res[1], res[2]), nil + case resolveImageGlobalImage.MatchString(name): // global/images/xyz + res := resolveImageGlobalImage.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d global image regex matches, got %d for %s", 1, len(res)-1, name) + } + return fmt.Sprintf("global/images/%s", res[1]), nil + case resolveImageGlobalFamily.MatchString(name): // global/images/family/xyz + res := resolveImageGlobalFamily.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d global family regex matches, got %d for %s", 1, len(res)-1, name) + } + return fmt.Sprintf("global/images/family/%s", res[1]), nil + case resolveImageFamilyFamily.MatchString(name): // family/xyz + res := resolveImageFamilyFamily.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d family family regex matches, got %d for %s", 1, len(res)-1, name) + } + if ok, err := resolveImageFamilyExists(c, c.Project, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("global/images/family/%s", res[1]), nil + } + if builtInProject != "" { + if ok, err := resolveImageFamilyExists(c, builtInProject, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/family/%s", builtInProject, res[1]), nil + } + } + case resolveImageProjectImageShorthand.MatchString(name): // xyz/xyz + res := resolveImageProjectImageShorthand.FindStringSubmatch(name) + if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d project image shorthand regex matches, got %d for %s", 2, len(res)-1, name) + } + if ok, err := resolveImageImageExists(c, res[1], res[2]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/%s", res[1], res[2]), nil + } + fallthrough // check if it's a family + case resolveImageProjectFamilyShorthand.MatchString(name): // xyz/xyz + res := resolveImageProjectFamilyShorthand.FindStringSubmatch(name) + if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d project family shorthand regex matches, got %d for %s", 2, len(res)-1, name) + } + if ok, err := resolveImageFamilyExists(c, res[1], res[2]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/family/%s", res[1], res[2]), nil + } + case resolveImageImage.MatchString(name): // xyz + res := resolveImageImage.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d image regex matches, got %d for %s", 1, len(res)-1, name) + } + if ok, err := resolveImageImageExists(c, c.Project, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("global/images/%s", res[1]), nil + } + if builtInProject != "" { + // check the images GCP provides + if ok, err := resolveImageImageExists(c, builtInProject, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/%s", builtInProject, res[1]), nil + } + } + fallthrough // check if the name is a family, instead of an image + case resolveImageFamily.MatchString(name): // xyz + res := resolveImageFamily.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d family regex matches, got %d for %s", 1, len(res)-1, name) + } + if ok, err := resolveImageFamilyExists(c, c.Project, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("global/images/family/%s", res[1]), nil + } + if builtInProject != "" { + // check the families GCP provides + if ok, err := resolveImageFamilyExists(c, builtInProject, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/family/%s", builtInProject, res[1]), nil + } + } + } + return "", fmt.Errorf("Could not find image or family %s", name) } diff --git a/image_test.go b/image_test.go index f500c9a4..e0f56518 100644 --- a/image_test.go +++ b/image_test.go @@ -1,61 +1,107 @@ package google import ( + "fmt" "testing" + compute "google.golang.org/api/compute/v1" + + "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" + "github.com/hashicorp/terraform/terraform" ) func TestAccComputeImage_resolveImage(t *testing.T) { + var image compute.Image + rand := acctest.RandString(10) + name := fmt.Sprintf("test-image-%s", rand) + fam := fmt.Sprintf("test-image-family-%s", rand) + resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, CheckDestroy: testAccCheckComputeImageDestroy, Steps: []resource.TestStep{ { - Config: testAccComputeImage_basedondisk, + Config: testAccComputeImage_resolving(name, fam), Check: resource.ComposeTestCheckFunc( testAccCheckComputeImageExists( "google_compute_image.foobar", &image), + testAccCheckComputeImageResolution("google_compute_image.foobar"), ), }, }, }) - images := map[string]string{ - "family/debian-8": "projects/debian-cloud/global/images/family/debian-8-jessie", - "projects/debian-cloud/global/images/debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", - "debian-8-jessie": "projects/debian-cloud/global/images/family/debian-8-jessie", - "debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", - "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110": "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110", +} - // TODO(paddy): we need private images/families here to actually test this - "global/images/my-private-image": "global/images/my-private-image", - "global/images/family/my-private-family": "global/images/family/my-private-family", - "my-private-image": "global/images/my-private-image", - "my-private-family": "global/images/family/my-private-family", - "my-project/my-private-image": "projects/my-project/global/images/my-private-image", - "my-project/my-private-family": "projects/my-project/global/images/family/my-private-family", - "insert-URL-here": "insert-URL-here", - } - config := &Config{ - Credentials: credentials, - Project: project, - Region: region, - } +func testAccCheckComputeImageResolution(n string) resource.TestCheckFunc { + return func(s *terraform.State) error { + config := testAccProvider.Meta().(*Config) + project := config.Project - err := config.loadAndValidate() - if err != nil { - t.Fatalf("Error loading config: %s\n", err) - } - for input, expectation := range images { - result, err := resolveImage(config, input) - if err != nil { - t.Errorf("Error resolving input %s to image: %+v\n", input, err) - continue + rs, ok := s.RootModule().Resources[n] + if !ok { + return fmt.Errorf("Resource not found: %s", n) } - if result != expectation { - t.Errorf("Expected input '%s' to resolve to '%s', it resolved to '%s' instead.\n", input, expectation, result) - continue + + if rs.Primary.ID == "" { + return fmt.Errorf("No ID is set") } + if rs.Primary.Attributes["name"] == "" { + return fmt.Errorf("No image name is set") + } + if rs.Primary.Attributes["family"] == "" { + return fmt.Errorf("No image family is set") + } + if rs.Primary.Attributes["self_link"] == "" { + return fmt.Errorf("No self_link is set") + } + + name := rs.Primary.Attributes["name"] + family := rs.Primary.Attributes["family"] + link := rs.Primary.Attributes["self_link"] + + images := map[string]string{ + "family/debian-8": "projects/debian-cloud/global/images/family/debian-8", + "projects/debian-cloud/global/images/debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", + "debian-8": "projects/debian-cloud/global/images/family/debian-8", + "debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", + "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110": "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110", + + "global/images/" + name: "global/images/" + name, + "global/images/family/" + family: "global/images/family/" + family, + name: "global/images/" + name, + family: "global/images/family/" + family, + "family/" + family: "global/images/family/" + family, + project + "/" + name: "projects/" + project + "/global/images/" + name, + project + "/" + family: "projects/" + project + "/global/images/family/" + family, + link: link, + } + + for input, expectation := range images { + result, err := resolveImage(config, input) + if err != nil { + return fmt.Errorf("Error resolving input %s to image: %+v\n", input, err) + } + if result != expectation { + return fmt.Errorf("Expected input '%s' to resolve to '%s', it resolved to '%s' instead.\n", input, expectation, result) + } + } + return nil } } + +func testAccComputeImage_resolving(name, family string) string { + return fmt.Sprintf(` +resource "google_compute_disk" "foobar" { + name = "%s" + zone = "us-central1-a" + image = "debian-8-jessie-v20160803" +} +resource "google_compute_image" "foobar" { + name = "%s" + family = "%s" + source_disk = "${google_compute_disk.foobar.self_link}" +} +`, name, name, family) +} From 869c7a610374a630635f0009400a8704da4c350c Mon Sep 17 00:00:00 2001 From: Paddy Date: Thu, 23 Feb 2017 22:09:07 -0800 Subject: [PATCH 3/5] Update the docs for resolveImage. Update the explanation of the logic being followed in resolveImage. --- image.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/image.go b/image.go index 912821b5..e772d95e 100644 --- a/image.go +++ b/image.go @@ -48,10 +48,18 @@ func resolveImageFamilyExists(c *Config, project, name string) (bool, error) { } // If the given name is a URL, return it. -// If it is of the form project/name, search the specified project first, then -// search image families in the specified project. -// If it is of the form name then look in the configured project, then hosted -// image projects, and lastly at image families in hosted image projects. +// If it's in the form projects/{project}/global/images/{image}, return it +// If it's in the form projects/{project}/global/images/family/{family}, return it +// If it's in the form global/images/{image}, return it +// If it's in the form global/images/family/{family}, return it +// If it's in the form family/{family}, 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}. +// If it's in the form {project}/{family-or-image}, check if it's an image in the named project. If it is, return it as projects/{project}/global/images/{image}. +// If not, check if it's a family in the named project. If it is, return it as projects/{project}/global/images/family/{family}. +// If it's in the form {family-or-image}, check if it's an image in the current project. If it is, return it as global/images/{image}. +// If not, check if it could be a GCP-provided image, and if it exists. If it does, return it as projects/{project}/global/images/{image}. +// 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, name string) (string, error) { // built-in projects to look for images/families containing the string // on the left in From a6338537700050d0e61d29077a80e3d59d349d4b Mon Sep 17 00:00:00 2001 From: Paddy Date: Mon, 13 Mar 2017 16:39:42 -0700 Subject: [PATCH 4/5] Update with @danawillow's feedback. * Make our regexes more permissive (though still separated out for readability, despite being identical) * Add a helper that will improve readability while sanity testing our regex results. --- image.go | 49 ++++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/image.go b/image.go index e772d95e..500b601f 100644 --- a/image.go +++ b/image.go @@ -9,9 +9,9 @@ import ( ) const ( - resolveImageProjectRegex = "[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?" // TODO(paddy): this isn't based on any documentation; we're just copying the image name restrictions. Need to follow up with @danawillow and/or @evandbrown and see if there's an actual limit to this - resolveImageFamilyRegex = "[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?" // TODO(paddy): this isn't based on any documentation; we're just copying the image name restrictions. Need to follow up with @danawillow and/or @evandbrown and see if there's an actual limit to this - resolveImageImageRegex = "[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?" // 1-63 characters, lowercase letters, numbers, and hyphens only, beginning and ending in a lowercase letter or number + resolveImageProjectRegex = "[-_a-zA-Z0-9]*" + resolveImageFamilyRegex = "[-_a-zA-Z0-9]*" + resolveImageImageRegex = "[-_a-zA-Z0-9]*" ) var ( @@ -47,6 +47,13 @@ func resolveImageFamilyExists(c *Config, project, name string) (bool, error) { } } +func sanityTestRegexMatches(expected int, got []string, regexType, name string) error { + if len(got)-1 != expected { // subtract one, index zero is the entire matched expression + return fmt.Errorf("Expected %d %s regex matches, got %d for %s", 2, regexType, len(got)-1, name) + } + return nil +} + // If the given name is a URL, return it. // If it's in the form projects/{project}/global/images/{image}, return it // If it's in the form projects/{project}/global/images/family/{family}, return it @@ -85,32 +92,32 @@ func resolveImage(c *Config, name string) (string, error) { return name, nil case resolveImageProjectImage.MatchString(name): // projects/xyz/global/images/xyz res := resolveImageProjectImage.FindStringSubmatch(name) - if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression - return "", fmt.Errorf("Expected %d project image regex matches, got %d for %s", 2, len(res)-1, name) + if err := sanityTestRegexMatches(2, res, "project image", name); err != nil { + return "", err } return fmt.Sprintf("projects/%s/global/images/%s", res[1], res[2]), nil case resolveImageProjectFamily.MatchString(name): // projects/xyz/global/images/family/xyz res := resolveImageProjectFamily.FindStringSubmatch(name) - if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression - return "", fmt.Errorf("Expected %d project family regex matches, got %d for %s", 2, len(res)-1, name) + if err := sanityTestRegexMatches(2, res, "project family", name); err != nil { + return "", err } return fmt.Sprintf("projects/%s/global/images/family/%s", res[1], res[2]), nil case resolveImageGlobalImage.MatchString(name): // global/images/xyz res := resolveImageGlobalImage.FindStringSubmatch(name) - if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression - return "", fmt.Errorf("Expected %d global image regex matches, got %d for %s", 1, len(res)-1, name) + if err := sanityTestRegexMatches(1, res, "global image", name); err != nil { + return "", err } return fmt.Sprintf("global/images/%s", res[1]), nil case resolveImageGlobalFamily.MatchString(name): // global/images/family/xyz res := resolveImageGlobalFamily.FindStringSubmatch(name) - if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression - return "", fmt.Errorf("Expected %d global family regex matches, got %d for %s", 1, len(res)-1, name) + if err := sanityTestRegexMatches(1, res, "global family", name); err != nil { + return "", err } return fmt.Sprintf("global/images/family/%s", res[1]), nil case resolveImageFamilyFamily.MatchString(name): // family/xyz res := resolveImageFamilyFamily.FindStringSubmatch(name) - if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression - return "", fmt.Errorf("Expected %d family family regex matches, got %d for %s", 1, len(res)-1, name) + if err := sanityTestRegexMatches(1, res, "family family", name); err != nil { + return "", err } if ok, err := resolveImageFamilyExists(c, c.Project, res[1]); err != nil { return "", err @@ -126,8 +133,8 @@ func resolveImage(c *Config, name string) (string, error) { } case resolveImageProjectImageShorthand.MatchString(name): // xyz/xyz res := resolveImageProjectImageShorthand.FindStringSubmatch(name) - if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression - return "", fmt.Errorf("Expected %d project image shorthand regex matches, got %d for %s", 2, len(res)-1, name) + if err := sanityTestRegexMatches(2, res, "project image shorthand", name); err != nil { + return "", err } if ok, err := resolveImageImageExists(c, res[1], res[2]); err != nil { return "", err @@ -137,8 +144,8 @@ func resolveImage(c *Config, name string) (string, error) { fallthrough // check if it's a family case resolveImageProjectFamilyShorthand.MatchString(name): // xyz/xyz res := resolveImageProjectFamilyShorthand.FindStringSubmatch(name) - if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression - return "", fmt.Errorf("Expected %d project family shorthand regex matches, got %d for %s", 2, len(res)-1, name) + if err := sanityTestRegexMatches(2, res, "project family shorthand", name); err != nil { + return "", err } if ok, err := resolveImageFamilyExists(c, res[1], res[2]); err != nil { return "", err @@ -147,8 +154,8 @@ func resolveImage(c *Config, name string) (string, error) { } case resolveImageImage.MatchString(name): // xyz res := resolveImageImage.FindStringSubmatch(name) - if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression - return "", fmt.Errorf("Expected %d image regex matches, got %d for %s", 1, len(res)-1, name) + if err := sanityTestRegexMatches(1, res, "image", name); err != nil { + return "", err } if ok, err := resolveImageImageExists(c, c.Project, res[1]); err != nil { return "", err @@ -166,8 +173,8 @@ func resolveImage(c *Config, name string) (string, error) { fallthrough // check if the name is a family, instead of an image case resolveImageFamily.MatchString(name): // xyz res := resolveImageFamily.FindStringSubmatch(name) - if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression - return "", fmt.Errorf("Expected %d family regex matches, got %d for %s", 1, len(res)-1, name) + if err := sanityTestRegexMatches(1, res, "family", name); err != nil { + return "", err } if ok, err := resolveImageFamilyExists(c, c.Project, res[1]); err != nil { return "", err From 354d919d75cd1d4614a11f389a0e1a6cd96f20c3 Mon Sep 17 00:00:00 2001 From: Paddy Date: Mon, 13 Mar 2017 22:04:08 -0700 Subject: [PATCH 5/5] Update typo. We never updated the error to use the expectation, not hardcode it to 2. --- image.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/image.go b/image.go index 500b601f..d21210d9 100644 --- a/image.go +++ b/image.go @@ -49,7 +49,7 @@ func resolveImageFamilyExists(c *Config, project, name string) (bool, error) { func sanityTestRegexMatches(expected int, got []string, regexType, name string) error { if len(got)-1 != expected { // subtract one, index zero is the entire matched expression - return fmt.Errorf("Expected %d %s regex matches, got %d for %s", 2, regexType, len(got)-1, name) + return fmt.Errorf("Expected %d %s regex matches, got %d for %s", expected, regexType, len(got)-1, name) } return nil }