From 77614b3cab4975bdd8c1bf124c8912e8a27fc875 Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Thu, 6 Jun 2019 16:31:11 -0400 Subject: [PATCH 1/8] progress --- config/module/versions.go | 20 ++++++++++++++++++++ config/module/versions_test.go | 23 +++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/config/module/versions.go b/config/module/versions.go index 8348d4b19535..fcf8f22cd0b5 100644 --- a/config/module/versions.go +++ b/config/module/versions.go @@ -3,7 +3,9 @@ package module import ( "errors" "fmt" + "regexp" "sort" + "strings" version "github.com/hashicorp/go-version" "github.com/hashicorp/terraform/registry/response" @@ -58,6 +60,24 @@ func newest(versions []string, constraint string) (string, error) { continue } if cs.Check(v) { + // We matched the constraint. But did it?? + // If the constraint is an equal for something with build data + // we should see if our match IS that + // >0.9.0+def [< 0.9.0+abc] + constraintMeta := strings.SplitAfterN(cs.String(), "+", 2) + fmt.Println(constraintMeta[1]) + // if we have some metadata there + // does the version have metadata? + fmt.Println(v.Metadata()) + // What kind of constraint do we have? + // if it is pure equality, does our metadata match?? + + ourregex := regexp.MustCompile("^=[0-9]") + if ourregex.MatchString(cs.String()) { + if constraintMeta[1] != v.Metadata() { + continue + } + } return versions[i], nil } } diff --git a/config/module/versions_test.go b/config/module/versions_test.go index b7ff6e6271e9..12037b515408 100644 --- a/config/module/versions_test.go +++ b/config/module/versions_test.go @@ -58,3 +58,26 @@ func TestNewestInvalidModuleVersion(t *testing.T) { t.Fatalf("expected version %q, got %q", expected, m.Version) } } + +func TestNewestModulesWithMetadata(t *testing.T) { + mpv := &response.ModuleProviderVersions{ + Source: "registry/test/module", + Versions: []*response.ModuleVersion{ + {Version: "0.0.4"}, + {Version: "0.3.1"}, + {Version: "2.0.1"}, + {Version: "1.2.0"}, + {Version: "0.9.0"}, + {Version: "0.9.0+def"}, + {Version: "0.9.0+abc"}, + {Version: "0.9.0+xyz"}, + {Version: "0.9.0+trees"}, + }, + } + // with metadata and explicit version request + expected := "0.9.0+def" + m, _ := newestVersion(mpv.Versions, "=0.9.0+def") + if m.Version != expected { + t.Fatalf("expected version %q, got %q", expected, m.Version) + } +} From c4488eadf069bfefecaa14e80376f60010ff4a51 Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Thu, 6 Jun 2019 16:58:49 -0400 Subject: [PATCH 2/8] Tidying up, add another test case --- config/module/versions.go | 23 +++++++++-------------- config/module/versions_test.go | 13 ++++++++----- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/config/module/versions.go b/config/module/versions.go index fcf8f22cd0b5..5bdbb69fe5ac 100644 --- a/config/module/versions.go +++ b/config/module/versions.go @@ -13,6 +13,8 @@ import ( const anyVersion = ">=0.0.0" +var explicitEqualityConstraint = regexp.MustCompile("^=[0-9]") + // return the newest version that satisfies the provided constraint func newest(versions []string, constraint string) (string, error) { if constraint == "" { @@ -52,6 +54,11 @@ func newest(versions []string, constraint string) (string, error) { return iv.GreaterThan(jv) }) + // Store whether the constraint contains a metadata requirement, + // so we can check if we should be returning a specific metadata version + constraintMeta := strings.SplitAfterN(cs.String(), "+", 2) + equalsConstraint := explicitEqualityConstraint.MatchString(cs.String()) + // versions are now in order, so just find the first which satisfies the // constraint for i := range versions { @@ -60,20 +67,8 @@ func newest(versions []string, constraint string) (string, error) { continue } if cs.Check(v) { - // We matched the constraint. But did it?? - // If the constraint is an equal for something with build data - // we should see if our match IS that - // >0.9.0+def [< 0.9.0+abc] - constraintMeta := strings.SplitAfterN(cs.String(), "+", 2) - fmt.Println(constraintMeta[1]) - // if we have some metadata there - // does the version have metadata? - fmt.Println(v.Metadata()) - // What kind of constraint do we have? - // if it is pure equality, does our metadata match?? - - ourregex := regexp.MustCompile("^=[0-9]") - if ourregex.MatchString(cs.String()) { + // Constraint has metadata and is explicit equality + if len(constraintMeta) > 1 && equalsConstraint { if constraintMeta[1] != v.Metadata() { continue } diff --git a/config/module/versions_test.go b/config/module/versions_test.go index 12037b515408..c470eba149d8 100644 --- a/config/module/versions_test.go +++ b/config/module/versions_test.go @@ -63,21 +63,24 @@ func TestNewestModulesWithMetadata(t *testing.T) { mpv := &response.ModuleProviderVersions{ Source: "registry/test/module", Versions: []*response.ModuleVersion{ - {Version: "0.0.4"}, - {Version: "0.3.1"}, - {Version: "2.0.1"}, - {Version: "1.2.0"}, {Version: "0.9.0"}, {Version: "0.9.0+def"}, {Version: "0.9.0+abc"}, {Version: "0.9.0+xyz"}, - {Version: "0.9.0+trees"}, }, } + // with metadata and explicit version request expected := "0.9.0+def" m, _ := newestVersion(mpv.Versions, "=0.9.0+def") if m.Version != expected { t.Fatalf("expected version %q, got %q", expected, m.Version) } + + // respect explicit equality, but >/0.9.0+abc") + if m.Version != expected { + t.Fatalf("expected version %q, got %q", expected, m.Version) + } } From edab4e7e9b5a24993541c3e5c2d75377941eea4c Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Fri, 7 Jun 2019 11:50:53 -0400 Subject: [PATCH 3/8] Update to ensure there is only one constraint present --- config/module/versions.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/config/module/versions.go b/config/module/versions.go index 5bdbb69fe5ac..c4734548f448 100644 --- a/config/module/versions.go +++ b/config/module/versions.go @@ -54,10 +54,15 @@ func newest(versions []string, constraint string) (string, error) { return iv.GreaterThan(jv) }) - // Store whether the constraint contains a metadata requirement, - // so we can check if we should be returning a specific metadata version - constraintMeta := strings.SplitAfterN(cs.String(), "+", 2) - equalsConstraint := explicitEqualityConstraint.MatchString(cs.String()) + // Store whether the constraint is an explicit equality that + // contains a metadata requirement, so we can return a specific + // if requested metadata version + var constraintMeta []string + var equalsConstraint bool + if len(cs) == 1 { + constraintMeta = strings.SplitAfterN(cs.String(), "+", 2) + equalsConstraint = explicitEqualityConstraint.MatchString(cs.String()) + } // versions are now in order, so just find the first which satisfies the // constraint @@ -68,7 +73,7 @@ func newest(versions []string, constraint string) (string, error) { } if cs.Check(v) { // Constraint has metadata and is explicit equality - if len(constraintMeta) > 1 && equalsConstraint { + if equalsConstraint && len(constraintMeta) > 1 { if constraintMeta[1] != v.Metadata() { continue } From 363f12a9134612c2940d9cae7e6d530e634ab200 Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Fri, 7 Jun 2019 12:28:49 -0400 Subject: [PATCH 4/8] Give an error when someone is doing something weird with metadata --- config/module/versions.go | 38 +++++++++++++++++++++++----------- config/module/versions_test.go | 13 ++++++++---- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/config/module/versions.go b/config/module/versions.go index c4734548f448..54d5d0c454b3 100644 --- a/config/module/versions.go +++ b/config/module/versions.go @@ -25,6 +25,30 @@ func newest(versions []string, constraint string) (string, error) { return "", err } + // Store whether the constraint is an explicit equality that + // contains a metadata requirement, so we can return a specific + // if requested metadata version + var constraintMetas []string + var equalsConstraint bool + for i := range cs { + constraintMeta := strings.SplitAfterN(cs[i].String(), "+", 2) + if len(constraintMeta) > 1 { + constraintMetas = append(constraintMetas, constraintMeta[1]) + } + } + + if len(cs) == 1 { + equalsConstraint = explicitEqualityConstraint.MatchString(cs.String()) + } + + if (len(cs) > 1 || !equalsConstraint) && len(constraintMetas) > 0 { + return "", fmt.Errorf("Constraints including metadata must have explicit equality, or are otherwise too ambiguous: %s", cs.String()) + } + + // If the version string includes metadata, this is valid in go-version, + // However, it's confusing as to what expected behavior should be, + // so give an error so the user can do something more logical + switch len(versions) { case 0: return "", errors.New("no versions found") @@ -54,16 +78,6 @@ func newest(versions []string, constraint string) (string, error) { return iv.GreaterThan(jv) }) - // Store whether the constraint is an explicit equality that - // contains a metadata requirement, so we can return a specific - // if requested metadata version - var constraintMeta []string - var equalsConstraint bool - if len(cs) == 1 { - constraintMeta = strings.SplitAfterN(cs.String(), "+", 2) - equalsConstraint = explicitEqualityConstraint.MatchString(cs.String()) - } - // versions are now in order, so just find the first which satisfies the // constraint for i := range versions { @@ -73,8 +87,8 @@ func newest(versions []string, constraint string) (string, error) { } if cs.Check(v) { // Constraint has metadata and is explicit equality - if equalsConstraint && len(constraintMeta) > 1 { - if constraintMeta[1] != v.Metadata() { + if equalsConstraint && len(constraintMetas) > 0 { + if constraintMetas[0] != v.Metadata() { continue } } diff --git a/config/module/versions_test.go b/config/module/versions_test.go index c470eba149d8..49dcd0d8154f 100644 --- a/config/module/versions_test.go +++ b/config/module/versions_test.go @@ -78,9 +78,14 @@ func TestNewestModulesWithMetadata(t *testing.T) { } // respect explicit equality, but >/0.9.0+abc") - if m.Version != expected { - t.Fatalf("expected version %q, got %q", expected, m.Version) + _, err := newestVersion(mpv.Versions, "~>0.9.0+abc") + if err == nil { + t.Fatalf("expected an error, but did not get one") + } + + // if there's build metadata in one of multiple constraints, expect an error + _, err = newestVersion(mpv.Versions, ">0.8.0+abc, <1.0.0") + if err == nil { + t.Fatalf("expected an error, but did not get one") } } From 0d81c86728297a1e86e298746f213cbc1d2e703a Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Fri, 7 Jun 2019 12:30:25 -0400 Subject: [PATCH 5/8] update test comment --- config/module/versions_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/config/module/versions_test.go b/config/module/versions_test.go index 49dcd0d8154f..a9d4941eb08a 100644 --- a/config/module/versions_test.go +++ b/config/module/versions_test.go @@ -77,13 +77,12 @@ func TestNewestModulesWithMetadata(t *testing.T) { t.Fatalf("expected version %q, got %q", expected, m.Version) } - // respect explicit equality, but >//0.9.0+abc") if err == nil { t.Fatalf("expected an error, but did not get one") } - // if there's build metadata in one of multiple constraints, expect an error _, err = newestVersion(mpv.Versions, ">0.8.0+abc, <1.0.0") if err == nil { t.Fatalf("expected an error, but did not get one") From bb68d2d233e784f3f90eda26b87842cb20b27183 Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Fri, 7 Jun 2019 13:02:23 -0400 Subject: [PATCH 6/8] Make error message clearer --- config/module/versions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/module/versions.go b/config/module/versions.go index 54d5d0c454b3..1108ead64c4c 100644 --- a/config/module/versions.go +++ b/config/module/versions.go @@ -42,7 +42,7 @@ func newest(versions []string, constraint string) (string, error) { } if (len(cs) > 1 || !equalsConstraint) && len(constraintMetas) > 0 { - return "", fmt.Errorf("Constraints including metadata must have explicit equality, or are otherwise too ambiguous: %s", cs.String()) + return "", fmt.Errorf("Constraints including build metadata must have explicit equality, or are otherwise too ambiguous: %s", cs.String()) } // If the version string includes metadata, this is valid in go-version, From f68f8435a5eb160338b878b4e2459d7d48ccd10f Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Fri, 7 Jun 2019 13:03:33 -0400 Subject: [PATCH 7/8] Move comment to correct place --- config/module/versions.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/config/module/versions.go b/config/module/versions.go index 1108ead64c4c..84d210e519a1 100644 --- a/config/module/versions.go +++ b/config/module/versions.go @@ -41,13 +41,12 @@ func newest(versions []string, constraint string) (string, error) { equalsConstraint = explicitEqualityConstraint.MatchString(cs.String()) } - if (len(cs) > 1 || !equalsConstraint) && len(constraintMetas) > 0 { - return "", fmt.Errorf("Constraints including build metadata must have explicit equality, or are otherwise too ambiguous: %s", cs.String()) - } - // If the version string includes metadata, this is valid in go-version, // However, it's confusing as to what expected behavior should be, // so give an error so the user can do something more logical + if (len(cs) > 1 || !equalsConstraint) && len(constraintMetas) > 0 { + return "", fmt.Errorf("Constraints including build metadata must have explicit equality, or are otherwise too ambiguous: %s", cs.String()) + } switch len(versions) { case 0: From 4b84cd3899a020bb15f4e941e793d00497b60d9a Mon Sep 17 00:00:00 2001 From: Pam Selle Date: Fri, 7 Jun 2019 13:05:14 -0400 Subject: [PATCH 8/8] Fixup comment --- config/module/versions.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/config/module/versions.go b/config/module/versions.go index 84d210e519a1..29701b931a9c 100644 --- a/config/module/versions.go +++ b/config/module/versions.go @@ -25,9 +25,10 @@ func newest(versions []string, constraint string) (string, error) { return "", err } - // Store whether the constraint is an explicit equality that - // contains a metadata requirement, so we can return a specific - // if requested metadata version + // Find any build metadata in the constraints, and + // store whether the constraint is an explicit equality that + // contains a build metadata requirement, so we can return a specific, + // if requested, build metadata version var constraintMetas []string var equalsConstraint bool for i := range cs {