Add support importing from govend #1040

RaviTezu · 2017-08-19T17:14:23Z

What does this do / why do we need it?

This PR includes the support importing from govend.

What should your reviewer look out for in this PR?

dep int now looks for vendor.yml file and converts it to Gopkg.toml and Gopkg.lock.

Do you need help or clarification on anything?

I see the go test is failing for when I run it in cmd/dep.

--- FAIL: TestIntegration (0.00s)
    --- FAIL: TestIntegration/init/govend/case1 (0.00s)
        --- FAIL: TestIntegration/init/govend/case1/external (8.39s)

However, when I run, go test on the govend/case1 is it passing:

λ terminator dep → go test github.com/golang/dep/cmd/dep -run testdata/harness_tests/init/govend/case1         
ok  	github.com/golang/dep/cmd/dep	2.014s

Which issue(s) does this PR fix?

Fixes #998

RaviTezu · 2017-08-19T17:14:54Z

@carolynvs Can you please take a look, when you have a minute? Thank you.

RaviTezu · 2017-08-19T17:27:35Z

Sorry, I forgot to mention this:
https://ci.appveyor.com/project/golang/dep/build/1821
I am not sure, why the test is failing, even if the got and expected are same.

bpicode · 2017-08-20T12:02:19Z

cmd/dep/testdata/harness_tests/init/govend/case1/final/Gopkg.lock

+  analyzer-version = 1
+  inputs-digest = "1ed417a0bec57ffe988fae1cba8f3d49994fb893394d61844e0b3c96d69573fe"
+  solver-name = "gps-cdcl"
+  solver-version = 1


Newline at the end of the file should fix the failing tests ;)

Oops! I am Sorry. Thanks @bpicode .
It solved the failing tests :)

darkowlzz · 2017-08-22T07:08:22Z

cmd/dep/govend_importer.go

+func (g *govendImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error) {
+	g.logger.Println("Converting from vendor.yaml...")
+
+	manifest := &dep.Manifest{


#1019 just got merged. Let's use the new Manifest constructor here.

darkowlzz · 2017-08-22T07:10:29Z

cmd/dep/testdata/harness_tests/init/govend/case1/testcase.json

+    "github.com/sdboyer/deptest",
+    "github.com/sdboyer/deptestdos"
+  ]
+}


No newline at EOF. Let's avoid this and add a newline.

Added a newline char at the EOF. But looks like github is not detecting it?

This works :)

darkowlzz · 2017-08-22T07:20:27Z

cmd/dep/govend_importer_test.go

+	// Disable verbose so that we don't print values that change each test run
+	g := newGovendImporter(logger, false, sm)
+	if !g.HasDepMetadata(projectRoot) {
+		t.Fatal("Expected the importer to detect godep configuration file")


~~godep~~ govend

carolynvs

Just a few very small changes, and I think this is good!

FYI, we have recently merged a PR (#992) which switches the importer tests over to use a set of common validations. However I really don't want to hold up this PR, and we can tackle that in a separate PR once this is in.

EDIT: Oops, ignore this comment. 😊 It's the vndr importer, not this one, that needs to switch to using the new validations. I'll take care of that right now.

carolynvs · 2017-08-21T19:59:20Z

cmd/dep/govend_importer.go

+	g.logger.Println("Detected govend configuration files...")
+	y := filepath.Join(projectDir, govendYAMLName)
+	if g.verbose {
+		g.logger.Printf("	Loading %s", y)


nit: This should be two spaces, not a tab.

carolynvs · 2017-08-21T20:03:07Z

cmd/dep/govend_importer.go

+	for _, pkg := range g.yaml.Imports {
+		// Path must not be empty
+		if pkg.Path == "" || pkg.Revision == "" {
+			return nil, nil, errors.New("Invalid govend configuration, Path or Rev is required")


nit: I think it will help people if we use the exact same words used in the config file, so "path and rev are required".

carolynvs · 2017-08-23T17:10:13Z

cmd/dep/govend_importer.go

+			pp := getProjectPropertiesFromVersion(version)
+			if pp.Constraint != nil {
+				pc, err := g.buildProjectConstraint(pkg, pp.Constraint.String())
+				if err != nil {


I realize this isn't in the other importers yet, but we'd like to have import keep going even when we can't guess a constraint from the revision. See #907.

So when buildProjectConstraint returns an error, let's log a warning like "Unable to infer a constraint for REVISION", and then skip adding the constraint to the manifest.

@carolynvs Done. Please take a look.

carolynvs

Sweet! Thank you for adding support for govend! 💖

RaviTezu requested review from carolynvs and sdboyer as code owners August 19, 2017 17:14

googlebot added the cla: yes label Aug 19, 2017

bpicode reviewed Aug 20, 2017

View reviewed changes

darkowlzz reviewed Aug 22, 2017

View reviewed changes

RaviTezu force-pushed the add-support-for-importing-from-govend branch from b6b6ae7 to 4603971 Compare August 22, 2017 09:25

carolynvs suggested changes Aug 23, 2017

View reviewed changes

carolynvs removed the request for review from sdboyer August 23, 2017 17:14

RaviTezu added 4 commits August 23, 2017 22:45

Support importing from govend

5ddabee

Adding a newline at the end of the file, to fix the failing tests

c21b520

use new manifest constructor and making requested changes

25e1d69

Log a warning, if we cannont infer a constrain; includes minor fixes

b28420b

RaviTezu force-pushed the add-support-for-importing-from-govend branch from 4603971 to b28420b Compare August 24, 2017 05:31

carolynvs approved these changes Aug 24, 2017

View reviewed changes

carolynvs merged commit 7c04369 into golang:master Aug 24, 2017

RaviTezu deleted the add-support-for-importing-from-govend branch August 24, 2017 15:01

This was referenced Aug 24, 2017

Remove 'override' constraint from Gopkg.toml bpicode/fritzctl#62

Closed

cobra/doc has some problems with dep (blackfriday major version change) spf13/cobra#519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support importing from govend #1040

Add support importing from govend #1040

RaviTezu commented Aug 19, 2017

RaviTezu commented Aug 19, 2017

RaviTezu commented Aug 19, 2017

bpicode Aug 20, 2017

RaviTezu Aug 20, 2017

darkowlzz Aug 22, 2017

darkowlzz Aug 22, 2017 •

edited

Loading

RaviTezu Aug 22, 2017

darkowlzz Aug 22, 2017

darkowlzz Aug 22, 2017

carolynvs left a comment •

edited

Loading

carolynvs Aug 21, 2017

carolynvs Aug 21, 2017

carolynvs Aug 23, 2017

RaviTezu Aug 24, 2017

carolynvs left a comment

Add support importing from govend #1040

Add support importing from govend #1040

Conversation

RaviTezu commented Aug 19, 2017

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?

RaviTezu commented Aug 19, 2017

RaviTezu commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz Aug 22, 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

carolynvs left a comment • 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

carolynvs left a comment

Choose a reason for hiding this comment

darkowlzz Aug 22, 2017 •

edited

Loading

carolynvs left a comment •

edited

Loading