From a3645543fcdd15da89b405e9ca352758556cd819 Mon Sep 17 00:00:00 2001 From: Jacky Chiu Date: Sun, 12 Nov 2017 17:36:37 -0500 Subject: [PATCH 1/5] Warning for constraint and overrides without version or source rule Updated tests for manifest warnings for new rule --- manifest.go | 13 ++++++++++--- manifest_test.go | 21 +++++++++++++++------ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/manifest.go b/manifest.go index 6115a5f482..f360b542ca 100644 --- a/manifest.go +++ b/manifest.go @@ -133,13 +133,17 @@ func validateManifest(s string) ([]error, error) { if valid { // Iterate through each array of tables for _, v := range rawProj { + ruleProvided := false + props := v.(map[string]interface{}) // Check the individual field's key to be valid - for key, value := range v.(map[string]interface{}) { + for key, value := range props { // Check if the key is valid switch key { - case "name", "branch", "version", "source": - // valid key + case "name": + case "branch", "version", "source": + ruleProvided = true case "revision": + ruleProvided = true if valueStr, ok := value.(string); ok { if abbrevRevHash.MatchString(valueStr) { warns = append(warns, fmt.Errorf("revision %q should not be in abbreviated form", valueStr)) @@ -155,6 +159,9 @@ func validateManifest(s string) ([]error, error) { warns = append(warns, fmt.Errorf("invalid key %q in %q", key, prop)) } } + if !ruleProvided && len(props) > 0 { + warns = append(warns, fmt.Errorf("version rule or source should be provided in %q", prop)) + } } } } else { diff --git a/manifest_test.go b/manifest_test.go index 93eb95ab83..1b50641ca4 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -255,6 +255,7 @@ func TestValidateManifest(t *testing.T) { `, wantWarn: []error{ errInvalidMetadata, + errors.New("version rule or source should be provided in \"constraint\""), }, wantError: nil, }, @@ -264,7 +265,9 @@ func TestValidateManifest(t *testing.T) { [[constraint]] name = "github.com/foo/bar" `, - wantWarn: []error{}, + wantWarn: []error{ + errors.New("version rule or source should be provided in \"constraint\""), + }, wantError: nil, }, { @@ -297,7 +300,9 @@ func TestValidateManifest(t *testing.T) { [[override]] name = "github.com/foo/bar" `, - wantWarn: []error{}, + wantWarn: []error{ + errors.New("version rule or source should be provided in \"override\""), + }, wantError: nil, }, { @@ -337,10 +342,12 @@ func TestValidateManifest(t *testing.T) { nick = "foo" `, wantWarn: []error{ - errors.New("invalid key \"location\" in \"constraint\""), - errors.New("invalid key \"link\" in \"constraint\""), - errors.New("invalid key \"nick\" in \"override\""), + errors.New("Invalid key \"location\" in \"constraint\""), + errors.New("Invalid key \"link\" in \"constraint\""), + errors.New("version rule or source should be provided in \"constraint\""), + errors.New("Invalid key \"nick\" in \"override\""), errors.New("metadata in \"constraint\" should be a TOML table"), + errors.New("version rule or source should be provided in \"override\""), }, wantError: nil, }, @@ -353,7 +360,9 @@ func TestValidateManifest(t *testing.T) { [constraint.metadata] color = "blue" `, - wantWarn: []error{}, + wantWarn: []error{ + errors.New("version rule or source should be provided in \"constraint\""), + }, wantError: nil, }, { From 3d3b5e206f3472671f5918a14abf6513f4d832e3 Mon Sep 17 00:00:00 2001 From: Jacky Chiu Date: Mon, 13 Nov 2017 18:53:58 -0500 Subject: [PATCH 2/5] move invalid metadata error to a var --- manifest.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/manifest.go b/manifest.go index f360b542ca..477e46c6c7 100644 --- a/manifest.go +++ b/manifest.go @@ -31,8 +31,7 @@ var ( errInvalidPrune = errors.Errorf("%q must be a TOML table of booleans", "prune") errInvalidPruneProject = errors.Errorf("%q must be a TOML array of tables", "prune.project") errInvalidMetadata = errors.New("metadata should be a TOML table") - - errInvalidProjectRoot = errors.New("ProjectRoot name validation failed") + errInvalidProjectRoot = errors.New("ProjectRoot name validation failed") errInvalidPruneValue = errors.New("prune options values must be booleans") errPruneSubProject = errors.New("prune projects should not contain sub projects") From dd595031660958d1bac23b8de35e5492807de226 Mon Sep 17 00:00:00 2001 From: Jacky Chiu Date: Tue, 14 Nov 2017 17:39:17 -0500 Subject: [PATCH 3/5] Warn for no name Dynamic version constraint error message Only warn for version constraint when it's a constraint field --- manifest.go | 7 +++++-- manifest_test.go | 24 +++++++++++++----------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/manifest.go b/manifest.go index 477e46c6c7..0bc48fea49 100644 --- a/manifest.go +++ b/manifest.go @@ -39,6 +39,7 @@ var ( errRootPruneContainsName = errors.Errorf("%q should not include a name", "prune") errInvalidRootPruneValue = errors.New("root prune options must be omitted instead of being set to false") errInvalidPruneProjectName = errors.Errorf("%q in %q must be a string", "name", "prune.project") + errNoName = errors.New("no name provided") ) // Manifest holds manifest file data and implements gps.RootManifest. @@ -158,8 +159,10 @@ func validateManifest(s string) ([]error, error) { warns = append(warns, fmt.Errorf("invalid key %q in %q", key, prop)) } } - if !ruleProvided && len(props) > 0 { - warns = append(warns, fmt.Errorf("version rule or source should be provided in %q", prop)) + if _, ok := props["name"]; !ok { + warns = append(warns, errNoName) + } else if !ruleProvided && prop == "constraint" { + warns = append(warns, fmt.Errorf("branch, version, revision, or source should be provided for %q", props["name"])) } } } diff --git a/manifest_test.go b/manifest_test.go index 1b50641ca4..92b5d2a67e 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -255,7 +255,7 @@ func TestValidateManifest(t *testing.T) { `, wantWarn: []error{ errInvalidMetadata, - errors.New("version rule or source should be provided in \"constraint\""), + errors.New("branch, version, revision, or source should be provided for \"github.com/foo/bar\""), }, wantError: nil, }, @@ -266,7 +266,7 @@ func TestValidateManifest(t *testing.T) { name = "github.com/foo/bar" `, wantWarn: []error{ - errors.New("version rule or source should be provided in \"constraint\""), + errors.New("branch, version, revision, or source should be provided for \"github.com/foo/bar\""), }, wantError: nil, }, @@ -275,7 +275,9 @@ func TestValidateManifest(t *testing.T) { tomlString: ` [[constraint]] `, - wantWarn: []error{}, + wantWarn: []error{ + errNoName, + }, wantError: nil, }, { @@ -300,9 +302,7 @@ func TestValidateManifest(t *testing.T) { [[override]] name = "github.com/foo/bar" `, - wantWarn: []error{ - errors.New("version rule or source should be provided in \"override\""), - }, + wantWarn: []error{}, wantError: nil, }, { @@ -310,7 +310,9 @@ func TestValidateManifest(t *testing.T) { tomlString: ` [[override]] `, - wantWarn: []error{}, + wantWarn: []error{ + errNoName, + }, wantError: nil, }, { @@ -344,10 +346,10 @@ func TestValidateManifest(t *testing.T) { wantWarn: []error{ errors.New("Invalid key \"location\" in \"constraint\""), errors.New("Invalid key \"link\" in \"constraint\""), - errors.New("version rule or source should be provided in \"constraint\""), - errors.New("Invalid key \"nick\" in \"override\""), errors.New("metadata in \"constraint\" should be a TOML table"), - errors.New("version rule or source should be provided in \"override\""), + errors.New("branch, version, revision, or source should be provided for \"github.com/foo/bar\""), + errors.New("Invalid key \"nick\" in \"override\""), + errNoName, }, wantError: nil, }, @@ -361,7 +363,7 @@ func TestValidateManifest(t *testing.T) { color = "blue" `, wantWarn: []error{ - errors.New("version rule or source should be provided in \"constraint\""), + errors.New("branch, version, revision, or source should be provided for \"github.com/foo/bar\""), }, wantError: nil, }, From 27c6df2e1028a2bcb63662bf4dbf2e0209c07f12 Mon Sep 17 00:00:00 2001 From: Jacky Chiu Date: Thu, 16 Nov 2017 21:13:46 -0500 Subject: [PATCH 4/5] Fix invalid manifest in windows test --- context_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/context_test.go b/context_test.go index 7e5eda6e25..041067e468 100644 --- a/context_test.go +++ b/context_test.go @@ -325,7 +325,10 @@ func TestCaseInsentitiveGOPATH(t *testing.T) { h.TempDir("src") h.TempDir("src/test1") - h.TempFile(filepath.Join("src/test1", ManifestName), `[[constraint]]`) + h.TempFile(filepath.Join("src/test1", ManifestName), ` + [[constraint]] + name = github.com/foo/bar + branch = master`) // Shuffle letter case rs := []rune(strings.ToLower(h.Path("."))) From 64543bc06044a07e3af4d4dd22fa0817a8aa7f3e Mon Sep 17 00:00:00 2001 From: Jacky Chiu Date: Wed, 22 Nov 2017 10:35:00 -0500 Subject: [PATCH 5/5] Fixed tests from different casing --- context_test.go | 4 ++-- manifest.go | 3 ++- manifest_test.go | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/context_test.go b/context_test.go index 041067e468..5ee5ba15fb 100644 --- a/context_test.go +++ b/context_test.go @@ -327,8 +327,8 @@ func TestCaseInsentitiveGOPATH(t *testing.T) { h.TempDir("src/test1") h.TempFile(filepath.Join("src/test1", ManifestName), ` [[constraint]] - name = github.com/foo/bar - branch = master`) + name = "github.com/foo/bar" + branch = "master"`) // Shuffle letter case rs := []rune(strings.ToLower(h.Path("."))) diff --git a/manifest.go b/manifest.go index 0bc48fea49..2af9c9ae4b 100644 --- a/manifest.go +++ b/manifest.go @@ -31,7 +31,8 @@ var ( errInvalidPrune = errors.Errorf("%q must be a TOML table of booleans", "prune") errInvalidPruneProject = errors.Errorf("%q must be a TOML array of tables", "prune.project") errInvalidMetadata = errors.New("metadata should be a TOML table") - errInvalidProjectRoot = errors.New("ProjectRoot name validation failed") + + errInvalidProjectRoot = errors.New("ProjectRoot name validation failed") errInvalidPruneValue = errors.New("prune options values must be booleans") errPruneSubProject = errors.New("prune projects should not contain sub projects") diff --git a/manifest_test.go b/manifest_test.go index 92b5d2a67e..fe154d5d29 100644 --- a/manifest_test.go +++ b/manifest_test.go @@ -344,11 +344,11 @@ func TestValidateManifest(t *testing.T) { nick = "foo" `, wantWarn: []error{ - errors.New("Invalid key \"location\" in \"constraint\""), - errors.New("Invalid key \"link\" in \"constraint\""), + errors.New("invalid key \"location\" in \"constraint\""), + errors.New("invalid key \"link\" in \"constraint\""), errors.New("metadata in \"constraint\" should be a TOML table"), errors.New("branch, version, revision, or source should be provided for \"github.com/foo/bar\""), - errors.New("Invalid key \"nick\" in \"override\""), + errors.New("invalid key \"nick\" in \"override\""), errNoName, }, wantError: nil,