fix(manifest): validate project roots in manifest #1116

darkowlzz · 2017-09-03T15:30:54Z

Validating ProjectRoot(s) require source manager, which is created after
loading the project. Hence, ProjectRoot validation can't be done in
existing validateManifest.

This change adds ValidateProjectRoots() which validates only the Constraint
and Override names to be valid ProjectRoot. Warnings are issued at stderr when
invalid ProjectRoot is found in manifest.

ensure and status call ValidateProjectRoots() expecitly.

What does this do / why do we need it?

Validates ProjectRoots in manifest.

What should your reviewer look out for in this PR?

The warning and error messages.

Do you need help or clarification on anything?

Should we warn or just error out when an invalid ProjectRoot is found?

Which issue(s) does this PR fix?

fixes #984

carolynvs · 2017-09-03T22:45:09Z

manifest.go

+			return errors.Wrapf(err, "could not deduce project root for %s", pr)
+		}
+		if origPR != pr {
+			c.Err.Printf("dep: WARNING: name %q in Gopkg.toml should be project root", pr)


It would help if the warning printed out the current name in the manifest and what it should be changed to, e.g.

"... the name for {origPR} in Gopkg.toml should be changed to {pr}"

yeah, they need not struggle to figure out what's a project root.

sdboyer · 2017-09-05T01:26:29Z

manifest.go

+		projectRoots = append(projectRoots, pr)
+	}
+
+	for _, pr := range projectRoots {


let's do these concurrently, otherwise any that require http requests for vanity URLs will end up being serialized.

sdboyer · 2017-09-05T01:28:09Z

manifest.go

+			return errors.Wrapf(err, "could not deduce project root for %s", pr)
+		}
+		if origPR != pr {
+			c.Err.Printf("dep: WARNING: the name for %q in Gopkg.toml should be changed to %q", pr, origPR)


are we doing the dep: prefix anywhere else? i don't recall. i don't think we are.

in any case, though, per your question in the original PR, i think we want to make this a case where we straight up fail, not just warn. there's nothing valid about these, at all, and we should immediately fail out if we encounter them.

darkowlzz · 2017-09-05T14:22:12Z

First time writing concurrent go code. Please suggest improvements 😊

This is how the errors are shown:

$ dep status
The Following issues were found in Gopkg.toml:

  ✗ github.com/go-yaml is not a valid path for a source on github.com
  ✗ the name for "github.com/Masterminds/vcs/foo" should be changed to "github.com/Masterminds/vcs"

ProjectRoot name validation failed

jmank88

Concurrency LGTM.

jmank88 · 2017-09-05T14:29:27Z

manifest.go

+			} else if origPR != pr {
+				errorCh <- fmt.Errorf("the name for %q should be changed to %q", pr, origPR)
+			}
+		}(pr)


If you define this function ahead of time, then you can launch the go routines from the two separate loops over the original slices, and avoid building the projectRoots slice and looping over them again.

That's a great idea 😃 Thanks!

jmank88 · 2017-09-05T15:23:13Z

manifest.go

+	} else if origPR != pr {
+		errorCh <- fmt.Errorf("the name for %q should be changed to %q", pr, origPR)
+	}
+}


Ah, this works, but I actually meant you could define a variable to hold the closure which you were already using, rather than invoking it in place. Then the function doesn't have to expand to include those 3 extra arguments.

ah! first class functions 😊 Haven't used them in that way much. Anyway, now I know how to use waitGroup with an external function 😀

Changing to hold the function in a variable.

Thanks again!

darkowlzz · 2017-09-07T10:58:29Z

Travis failure in osx:

$ go test -race ./...

ok  	github.com/golang/dep	6.118s

ok  	github.com/golang/dep/cmd/dep	110.191s

?   	github.com/golang/dep/hack/licenseok	[no test files]

ok  	github.com/golang/dep/internal/feedback	1.081s

ok  	github.com/golang/dep/internal/fs	1.146s

--- FAIL: TestMultiFetchThreadsafe (17.64s)

    --- FAIL: TestMultiFetchThreadsafe/second (0.10s)

        --- FAIL: TestMultiFetchThreadsafe/second/sync:1:github.com/sdboyer/gpkt (1.16s)

        	manager_test.go:577: https://github.com/sdboyer/gpkt does not exist in the local cache and fetching failed: unable to get repository: Cloning into '/var/folders/my/m6ynh3bn6tq06h7xr3js0z7r0000gn/T/smcache192671536/sources/https---git.luolix.top-sdboyer-gpkt'...

        		fatal: remote-curl: fetch attempted without a local repo

        		: command failed: [git clone --recursive -v --progress https://github.com/sdboyer/gpkt /var/folders/my/m6ynh3bn6tq06h7xr3js0z7r0000gn/T/smcache192671536/sources/https---git.luolix.top-sdboyer-gpkt]: exit status 128

Signal received: waiting for 1 ops to complete...

FAIL

FAIL	github.com/golang/dep/internal/gps	92.879s

ok  	github.com/golang/dep/internal/gps/paths	1.031s

ok  	github.com/golang/dep/internal/gps/pkgtree	1.178s

?   	github.com/golang/dep/internal/gps/testdata_digest/github.com/alice/match	[no test files]

?   	github.com/golang/dep/internal/gps/testdata_digest/github.com/alice/mismatch	[no test files]

?   	github.com/golang/dep/internal/gps/testdata_digest/github.com/alice/notInLock	[no test files]

?   	github.com/golang/dep/internal/gps/testdata_digest/github.com/bob/emptyDigest	[no test files]

?   	github.com/golang/dep/internal/gps/testdata_digest/github.com/bob/match	[no test files]

?   	github.com/golang/dep/internal/gps/testdata_digest/launchpad.net/match	[no test files]

?   	github.com/golang/dep/internal/test	[no test files]

?   	github.com/golang/dep/internal/test/integration	[no test files]

The command "go test -race ./..." exited with 1.

Done. Your build exited with 1.

Restarting.

ibrasho

Overall, LGTM with minor nits.

ibrasho · 2017-09-08T15:53:20Z

manifest.go

+	var valErr error
+	if len(errorCh) > 0 {
+		valErr = errInvalidProjectRoot
+		c.Err.Printf("The Following issues were found in Gopkg.toml:\n\n")


s/Following/following/

ibrasho · 2017-09-08T15:54:15Z

manifest.go

+		defer wg.Done()
+		origPR, err := sm.DeduceProjectRoot(string(pr))
+		if err != nil {
+			errorCh <- err


Maybe prefix the pr just in case the error message is not clear?

the error messages is returned with PR in it already

✗ github.com/go-yaml is not a valid path for a source on github.com

So, it's fine :)

ibrasho · 2017-09-08T15:55:18Z

manifest_test.go

+			name: "valid project root",
+			manifest: Manifest{
+				Constraints: map[gps.ProjectRoot]gps.ProjectProperties{
+					gps.ProjectRoot("github.com/goland/dep"): {


goland? 😁

I keep doing this a lot. 😂 goland

Validating ProjectRoot(s) require source manager, which is created after loading the project. Hence, ProjectRoot validation can't be done in existing validateManifest. This change adds `ValidateProjectRoots()` which validates only the Constraint and Override names to be valid ProjectRoot. Warnings are issued at stderr when invalid ProjectRoot is found in manifest. `ensure` and `status` call `ValidateProjectRoots()` expecitly.

darkowlzz requested review from ibrasho and sdboyer as code owners September 3, 2017 15:30

googlebot added the cla: yes label Sep 3, 2017

carolynvs reviewed Sep 3, 2017

View reviewed changes

darkowlzz force-pushed the manifest-pr-validate branch from 34e1de4 to 9a78083 Compare September 4, 2017 13:45

sdboyer suggested changes Sep 5, 2017

View reviewed changes

darkowlzz force-pushed the manifest-pr-validate branch from 9a78083 to 793785b Compare September 5, 2017 14:16

jmank88 approved these changes Sep 5, 2017

View reviewed changes

jmank88 reviewed Sep 5, 2017

View reviewed changes

darkowlzz force-pushed the manifest-pr-validate branch from 2fbfda9 to 07ef084 Compare September 5, 2017 15:46

ibrasho reviewed Sep 8, 2017

View reviewed changes

darkowlzz added 4 commits September 9, 2017 18:18

change ProjectRoot Validation warning message

ee6e7ad

manifest: make ValidateProjectRoots concurrent

0e4d48c

manifest: move validate goroutine in var and reuse

c0d23a6

darkowlzz force-pushed the manifest-pr-validate branch from 07ef084 to ddc0326 Compare September 9, 2017 12:56

manifest: fix typos

fa811f2

darkowlzz force-pushed the manifest-pr-validate branch from ddc0326 to fa811f2 Compare September 9, 2017 12:57

ibrasho approved these changes Sep 9, 2017

View reviewed changes

darkowlzz merged commit 28fb6b0 into golang:master Sep 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(manifest): validate project roots in manifest #1116

fix(manifest): validate project roots in manifest #1116

darkowlzz commented Sep 3, 2017 •

edited

Loading

carolynvs Sep 3, 2017

darkowlzz Sep 4, 2017 •

edited

Loading

sdboyer Sep 5, 2017

sdboyer Sep 5, 2017

darkowlzz commented Sep 5, 2017

jmank88 left a comment

jmank88 Sep 5, 2017

darkowlzz Sep 5, 2017

jmank88 Sep 5, 2017

darkowlzz Sep 5, 2017

darkowlzz commented Sep 7, 2017

ibrasho left a comment

ibrasho Sep 8, 2017 •

edited

Loading

ibrasho Sep 8, 2017

darkowlzz Sep 9, 2017

ibrasho Sep 8, 2017

darkowlzz Sep 9, 2017

fix(manifest): validate project roots in manifest #1116

fix(manifest): validate project roots in manifest #1116

Conversation

darkowlzz commented Sep 3, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

Choose a reason for hiding this comment

darkowlzz Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented Sep 5, 2017

jmank88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented Sep 7, 2017

ibrasho left a comment

Choose a reason for hiding this comment

ibrasho Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented Sep 3, 2017 •

edited

Loading

darkowlzz Sep 4, 2017 •

edited

Loading

ibrasho Sep 8, 2017 •

edited

Loading