Manifest Version/Source Validation #1370

JackyChiu · 2017-11-12T22:53:45Z

What does this do / why do we need it?

Constraints/Overrides configs should have a version rule or source.
Warn when it doesn't.

ex.

# Gopkg.toml
[[constraint]]
  name = "github.com/JackyChiu/super-cool-project

$ dep ensure
dep: WARNING: version rule or source should be provided in "constraint"

What should your reviewer look out for in this PR?

Is the warning message clear?

Which issue(s) does this PR fix?

fixes #1336

carolynvs · 2017-11-13T02:14:01Z

I left a comment on the original issue but want to make it clear that I think we should hold off on merging this until the importers are updated to match (or we rethink how we want this to work). Otherwise after using dep init people will immediately get warnings which would be super confusing for users. 😀

darkowlzz

Reviewing this now, although we might put this on a little hold.

Looks good 👍
Suggested some improvements.

darkowlzz · 2017-11-13T16:12:14Z

manifest.go

@@ -117,6 +121,9 @@ func validateManifest(s string) ([]error, error) {
 								warns = append(warns, fmt.Errorf("Invalid key %q in %q", key, prop))
 							}
 						}
+						if !ruleProvided && len(props) > 0 {


Let's warn for no name attribute in constraint/override too. With that we can be sure, we have at least one attribute and not empty. And this check should be performed before checking for missing rule.

Also, since there won't be any changing part in missing name in constraint/override warning, you can create 2 separate variables for them, similar to the error variables in https://github.com/golang/dep/blob/v0.3.2/manifest.go#L31 . And use the same variables in tests to perform comparison.

darkowlzz · 2017-11-13T16:33:52Z

manifest.go

@@ -117,6 +121,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))
+						}


I think this should be a dynamic error with name in it. So, maybe something like:

fmt.Errorf("branch, version, revision, or source should be provided for %q %q", prop, props["name"])

Which would look something like:

branch, version, revision, or source should be provided for "constraint" "github.com/foo/bar"

Since this warning would only show up after checking for missing constraint/override, we are sure that props["name"] won't be empty.

⚠️ Also, no rule check should only be performed for constraint and not for override because just the name attribute is useful in case of overrides. Not the same case with constraints.

darkowlzz · 2017-11-13T16:35:22Z

manifest_test.go

@@ -233,15 +233,20 @@ func TestValidateManifest(t *testing.T) {
 			[[constraint]]
 			  name = "github.com/foo/bar"
 			`,
-			wantWarn:  []error{errors.New("metadata should be a TOML table")},
+			wantWarn: []error{
+				errors.New("metadata should be a TOML table"),


Let's improve this. This error can be put in a variable and reused, like the variables in https://github.com/golang/dep/blob/v0.3.2/manifest.go#L31.

JackyChiu · 2017-11-14T00:11:55Z

Thanks for the review, I'll ping when I've fixed things up 😄

sdboyer · 2017-11-14T00:49:08Z

thanks for creating #1373, @carolynvs - i've marked this as blocked by that.

JackyChiu · 2017-11-14T22:44:00Z

@darkowlzz updated!

darkowlzz

LGTM! 👍

As discussed earlier, since this is blocked by another issue, we will merge it after that's cleared.

Thanks!

darkowlzz · 2017-11-15T13:34:21Z

However, I'm not sure why it's failing on Windows only 🤔

--- FAIL: TestCaseInsentitiveGOPATH (0.03s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x5a2e8b]

darkowlzz

ah! got it. So TestCaseInsentitiveGOPATH is a windows only test and it creates a file with content "[[constraint]]". And that's invalid with this change.

Let's make that a valid constraint.

Updated tests for manifest warnings for new rule

Dynamic version constraint error message Only warn for version constraint when it's a constraint field

carolynvs · 2017-12-03T22:34:59Z

#1414 was just merged and dep init shouldn't create empty constraints anymore, so this can finally be merged! 🎉

JackyChiu requested a review from sdboyer as a code owner November 12, 2017 22:53

googlebot added the cla: yes label Nov 12, 2017

darkowlzz reviewed Nov 13, 2017

View reviewed changes

darkowlzz approved these changes Nov 15, 2017

View reviewed changes

darkowlzz suggested changes Nov 15, 2017

View reviewed changes

JackyChiu force-pushed the master branch 3 times, most recently from 153bafc to 6d46589 Compare November 17, 2017 05:13

darkowlzz approved these changes Nov 18, 2017

View reviewed changes

JackyChiu added 4 commits November 21, 2017 17:20

Warning for constraint and overrides without version or source rule

a364554

Updated tests for manifest warnings for new rule

move invalid metadata error to a var

3d3b5e2

Warn for no name

dd59503

Dynamic version constraint error message Only warn for version constraint when it's a constraint field

Fix invalid manifest in windows test

27c6df2

JackyChiu force-pushed the master branch from 6d46589 to 44c24c4 Compare November 22, 2017 15:35

Fixed tests from different casing

64543bc

JackyChiu force-pushed the master branch from 44c24c4 to 64543bc Compare November 22, 2017 15:41

carolynvs merged commit 8f4a82c into golang:master Dec 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manifest Version/Source Validation #1370

Manifest Version/Source Validation #1370

JackyChiu commented Nov 12, 2017 •

edited

Loading

carolynvs commented Nov 13, 2017

darkowlzz left a comment

darkowlzz Nov 13, 2017

darkowlzz Nov 13, 2017

darkowlzz Nov 13, 2017 •

edited

Loading

darkowlzz Nov 13, 2017

JackyChiu commented Nov 14, 2017 •

edited

Loading

sdboyer commented Nov 14, 2017

JackyChiu commented Nov 14, 2017

darkowlzz left a comment

darkowlzz commented Nov 15, 2017

darkowlzz left a comment

carolynvs commented Dec 3, 2017

Manifest Version/Source Validation #1370

Manifest Version/Source Validation #1370

Conversation

JackyChiu commented Nov 12, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Which issue(s) does this PR fix?

carolynvs commented Nov 13, 2017

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz Nov 13, 2017

Choose a reason for hiding this comment

darkowlzz Nov 13, 2017

Choose a reason for hiding this comment

darkowlzz Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

darkowlzz Nov 13, 2017

Choose a reason for hiding this comment

JackyChiu commented Nov 14, 2017 • edited Loading

sdboyer commented Nov 14, 2017

JackyChiu commented Nov 14, 2017

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz commented Nov 15, 2017

darkowlzz left a comment

Choose a reason for hiding this comment

carolynvs commented Dec 3, 2017

JackyChiu commented Nov 12, 2017 •

edited

Loading

darkowlzz Nov 13, 2017 •

edited

Loading

JackyChiu commented Nov 14, 2017 •

edited

Loading