download: add list from go cli #336

marwan-at-work · 2018-07-26T20:32:44Z

No description provided.

arschles

arschles · 2018-07-26T20:39:05Z

pkg/download/goget/goget.go

+	}
+
+	return v.Zip, nil
+}


the code in this file looks very similar to the ./pkg/module/go_get_fetcher.go and disk_ref.go - any way you could build the goget protocol on top of those two primitives?

@arschles I am. Line 162 above in this function

I take this back completely - I see you are using the Fetcher and Ref!

arschles

Any chance you could add some tests for the goget protocol?

If you can do it for the list handler as well, that'd be rad but it is outside the scope of this PR so I don't think we should block on that

arschles · 2018-07-26T20:42:57Z

pkg/download/goget/goget.go

+	}
+
+	return v.Zip, nil
+}


I take this back completely - I see you are using the Fetcher and Ref!

arschles · 2018-07-26T20:51:47Z

Ref #290 - this doesn't synchronously do cache fills but it's related because it's listing tags synchronously

ghost · 2018-07-26T21:01:40Z

cmd/proxy/actions/app_proxy.go

@@ -16,8 +17,9 @@ func addProxyRoutes(
 ) error {
 	app.GET("/", proxyHomeHandler)

+	dp := goget.New()


A nonsense nit but maybe gg, I guess dp was download protocol?

@robjloranger yeah dp for download protocol because the handlers don't care what the implementation is (go get, github, storage, etc) -- I think it would be really cool if in the very near future we made one download.Protocol interface that will do most of the magic for us: go get, storage, cache filling, etc.

And by magic, I don't really mean magic in the meta programming sense lol

That makes more sense, knowing there might be other implementations of the download protocol

arschles

this is looking great. I only have nits, all of which we can address in a follow-up. Merge when you feel comfortable

arschles · 2018-07-26T22:20:04Z

pkg/download/download.go

+	"github.com/gomods/athens/pkg/storage"
+)
+
+// Protocol is the download protocol defined by cmd/go


defined by cmd/go

do you mean that this is taken from cmd/go?

Sorry the wording is definitely misleading. I meant they mirror the HTTP requests that cmd/go expects to use. I'll update the comment.

arschles · 2018-07-26T22:21:06Z

pkg/download/goget/goget.go

+		return errors.E(op, err)
+	}
+	return nil
+}


this code would be super useful in the Fetcher.Fetch method. can we move it down to there and then make list use it?

@arschles this is already there, I just copied it over. Should I export the function from that package?

arschles · 2018-07-26T22:22:03Z

pkg/download/goget/goget_test.go

+// so we can get reproducible results from live VCS repos.
+// For now, I cannot test that github.com/pkg/errors returns v0.8.0
+// from goget.Latest, because they could very well introduce a new tag
+// in the near future.


sounds like a great idea! mind creating an issue for it?

Can we update the comment to ref: #340

codecov-io · 2018-07-27T14:08:42Z

Codecov Report

Merging #336 into master will decrease coverage by 0.28%.
The diff coverage is 31.77%.

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
- Coverage   37.64%   37.36%   -0.29%     
==========================================
  Files         102      104       +2     
  Lines        2757     2858     +101     
==========================================
+ Hits         1038     1068      +30     
- Misses       1615     1681      +66     
- Partials      104      109       +5

Impacted Files	Coverage Δ
pkg/errors/kinds.go	`0% <0%> (ø)`
cmd/olympus/actions/app.go	`80.95% <100%> (+0.22%)`	⬆️
cmd/proxy/actions/app_proxy.go	`100% <100%> (ø)`	⬆️
pkg/download/goget/goget.go	`29.03% <29.03%> (ø)`
pkg/module/go_get_fetcher.go	`49.46% <37.5%> (-1.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ef9d7f...4379bcd. Read the comment docs.

ghost · 2018-07-27T16:33:22Z

pkg/download/goget/goget.go

+	if err != nil {
+		return nil, errors.E(op, err)
+	}
+	defer v.Zip.Close()


It's kind of the same thing but you could probably just call Close as the zip is never used.

ghost · 2018-07-27T16:35:14Z

pkg/download/goget/goget.go

+		return nil, errors.E(op, fmt.Errorf("malformed pseudoInfo %v", lr.Version))
+	}
+	return &storage.RevInfo{
+		Name:    pseudoInfo[2],


Should the name and the short name be the same? I'm not familiar.

@robjloranger the name is meant to be the full sha. But it should work with the short version. I'm currently testing that.

ghost · 2018-07-27T16:45:54Z

pkg/download/goget/goget.go

+	if err != nil {
+		return nil, errors.E(op, err)
+	}
+	defer v.Zip.Close()


Same as with Info, could just close zip now as it's not used.

ghost · 2018-07-27T16:47:44Z

pkg/download/goget/goget_test.go

+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := dp.List(ctx, tc.mod) // TODO ensure list is correct per TODO above.


Will this be in this PR?

Probably after

ghost

Looks great, little comments and questions but not important. Do we need/want an issue for removing the error from the NewGoGetFetcher function signature?

arschles · 2018-07-27T17:23:13Z

@robjloranger I removed that error in #337, but if you create an issue let me know and I'll mention it in that PR

ghost · 2018-07-27T17:48:28Z

No worries then @arschles, just wanted to make sure it didn't go missed.

arschles · 2018-07-27T18:56:22Z

@carolynvs, @marwan-at-work and @Kunde21 discussed this PR on a screenshare. we are merging now, and doing follow-up refactors in #337

🎉

marwan-at-work requested a review from arschles July 26, 2018 20:35

arschles requested changes Jul 26, 2018

View reviewed changes

pkg/download/goget/goget.go Outdated

}

return v.Zip, nil

}

Copy link

Member

arschles Jul 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take this back completely - I see you are using the Fetcher and Ref!

ghost reviewed Jul 26, 2018

View reviewed changes

marwan-at-work force-pushed the betterlist branch from f2030bc to 66b2be1 Compare July 26, 2018 22:17

arschles approved these changes Jul 26, 2018

View reviewed changes

arschles mentioned this pull request Jul 26, 2018

add middlewares to fill the cache #337

Closed

marwan-at-work force-pushed the betterlist branch from da0707e to 710ba72 Compare July 27, 2018 15:52

ghost reviewed Jul 27, 2018

View reviewed changes

ghost approved these changes Jul 27, 2018

View reviewed changes

marwan-at-work added 5 commits July 27, 2018 14:14

download: add list from go cli

c604da2

download: include goget tests + hacky hack

bbb569b

download: move dummyMod to pkg/module

a629644

Olympus: pass dp and lggr to /list

82f60c9

download: add Version to interface

79a5d67

marwan-at-work force-pushed the betterlist branch from 3a7e93d to 79a5d67 Compare July 27, 2018 18:14

download: document Protocol

4379bcd

arschles merged commit 7b590b1 into gomods:master Jul 27, 2018

marwan-at-work deleted the betterlist branch August 3, 2018 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

download: add list from go cli #336

download: add list from go cli #336

marwan-at-work commented Jul 26, 2018

arschles left a comment

arschles Jul 26, 2018

marwan-at-work Jul 26, 2018

arschles Jul 26, 2018

arschles left a comment

arschles Jul 26, 2018

arschles commented Jul 26, 2018

ghost Jul 26, 2018

marwan-at-work Jul 26, 2018

marwan-at-work Jul 26, 2018

ghost Jul 26, 2018

arschles left a comment

arschles Jul 26, 2018

marwan-at-work Jul 27, 2018

arschles Jul 26, 2018

marwan-at-work Jul 27, 2018

arschles Jul 26, 2018

marwan-at-work Jul 27, 2018

ghost Jul 27, 2018

codecov-io commented Jul 27, 2018 •

edited

Loading

ghost Jul 27, 2018

ghost Jul 27, 2018

marwan-at-work Jul 27, 2018 •

edited

Loading

ghost Jul 27, 2018

ghost Jul 27, 2018

marwan-at-work Jul 27, 2018

ghost left a comment

arschles commented Jul 27, 2018

ghost commented Jul 27, 2018

arschles commented Jul 27, 2018

download: add list from go cli #336

download: add list from go cli #336

Conversation

marwan-at-work commented Jul 26, 2018

arschles 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

arschles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles commented Jul 26, 2018

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

arschles 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 27, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marwan-at-work Jul 27, 2018 • 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

ghost left a comment

Choose a reason for hiding this comment

arschles commented Jul 27, 2018

ghost commented Jul 27, 2018

arschles commented Jul 27, 2018

codecov-io commented Jul 27, 2018 •

edited

Loading

marwan-at-work Jul 27, 2018 •

edited

Loading