Download: get /list from upstream #300

marwan-at-work · 2018-07-18T19:39:45Z

Implements the /list part of #290

codecov-io · 2018-07-18T19:46:59Z

Codecov Report

Merging #300 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
+ Coverage   40.72%   40.75%   +0.02%     
==========================================
  Files          97       97              
  Lines        2394     2395       +1     
==========================================
+ Hits          975      976       +1     
  Misses       1316     1316              
  Partials      103      103

Impacted Files	Coverage Δ
cmd/proxy/actions/app_proxy.go	`100% <100%> (ø)`	⬆️
cmd/olympus/actions/app.go	`80.95% <100%> (+0.22%)`	⬆️

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 016b999...181a99c. Read the comment docs.

arschles

A few comments, overall I love this!

One thought I had was that we could further separate the proxy vs. registry endpoint logic & upstream "fetchers"

I have some of that in progress at #291, and we can discuss in follow-ups. This is a massive and awesome start

arschles · 2018-07-18T22:32:52Z

Makefile

-	docker-compose -p athensdev up -d mysql
-	docker-compose -p athensdev up -d postgres
+	# docker-compose -p athensdev up -d mysql
+	# docker-compose -p athensdev up -d postgres


@marwan-at-work why comment these out?

@arschles ops, didn't meant this to be in the PR. But now that it's here: we're not using mysql/postgres in development are we? I keep commenting this out because they take a while to build up and tear down

@marwan-at-work I think you need them up if you want to run i.e. the storage tests but you're right - not in development since the storage defaults to mem or fs i think.
Maybe we should have a separate target for setting up the test dependencies?

agree - we should have a separate target for test deps. is it cool if we leave these uncommented and do that separation in a follow-up PR?

arschles · 2018-07-18T22:33:03Z

cmd/olympus/actions/app.go

@@ -62,6 +62,7 @@ func App(config *AppConfig) (*buffalo.App, error) {
 			return nil, err
 		}

+		lggr := log.New(env.CloudRuntime(), env.LogLevel())


💯 !!! really excited to see these loggers

arschles · 2018-07-18T22:36:20Z

pkg/download/list.go

-			return c.Render(http.StatusNotFound, eng.JSON(err.Error()))
+		if storage.IsNotFoundError(err) || len(versions) == 0 {
+			if isProxy {
+				return c.Redirect(http.StatusMovedPermanently, env.OlympusGlobalEndpointWithDefault(""))


could we pass in the olympus global endpoint?

also can you add a comment here explaining what this redirect will do to vgo, so we remember?

Yes, will make sure I pass it. Will add a comment about redirects as well.

arschles · 2018-07-18T22:36:52Z

pkg/download/list.go

+				lggr.WithFields(logrus.Fields{
+					"module": mod,
+					"method": "ListHandler.getInfoFromPath",
+				}).SystemErr(err)


should this return?

we do return, below :)

doh, thanks

arschles · 2018-07-18T22:38:21Z

pkg/download/list.go

+
+			if err != nil || info.typ != sourceGithub {
+				c.Response().WriteHeader(http.StatusNotFound)
+				return nil


can you use c.Error(http.StatusNotFound, errors.New("something")) here? so we have a message that could be logged at some point in the future. same with the above c.Response()... calls

@arschles I cannot use c.Error Buffalo doesn't do well with logging c.Error See gobuffalo/buffalo#1171

Either way, we are logging the error above so it won't get away :)

nice, ok - thanks for explaining

marwan-at-work · 2018-07-19T01:41:45Z

@arschles updated the PR but I had to move the Olympus Env Var out of actions to avoid import cycles.

arschles

@marwan-at-work this is looking 🔥 - some more comments

arschles · 2018-07-19T19:26:43Z

Makefile

-	docker-compose -p athensdev up -d mysql
-	docker-compose -p athensdev up -d postgres
+	# docker-compose -p athensdev up -d mysql
+	# docker-compose -p athensdev up -d postgres


agree - we should have a separate target for test deps. is it cool if we leave these uncommented and do that separation in a follow-up PR?

arschles · 2018-07-19T19:28:41Z

pkg/download/list.go

+				lggr.WithFields(logrus.Fields{
+					"module": mod,
+					"method": "ListHandler.getInfoFromPath",
+				}).SystemErr(err)


doh, thanks

arschles · 2018-07-19T19:29:10Z

pkg/download/list.go

+
+			if err != nil || info.typ != sourceGithub {
+				c.Response().WriteHeader(http.StatusNotFound)
+				return nil


nice, ok - thanks for explaining

arschles · 2018-07-19T19:30:40Z

pkg/download/list.go

 	"github.com/gomods/athens/pkg/paths"
 	"github.com/gomods/athens/pkg/storage"
+	"github.com/google/go-github/github"
+	"github.com/marwan-at-work/vgop/semver"


@marwan-at-work I remember you saying you had to copy some logic from the go modules tree to verify semvers. Instead of that would you be open to using https://github.com/Masterminds/semver, so you don't have to maintain more code?

@arschles i looked into this package and they seem to be relaxed on the v prefix restriction (they allow 1.3.2 while semver.IsValid would not allow it - not sure if there are other inconsistencies between the Masterminds/semver and go/semver - should we just use Masterminds/semver anyway?

arschles · 2018-07-19T19:31:29Z

pkg/download/list.go

+				// as that server returns an answer that Go expects.
+				return c.Redirect(http.StatusMovedPermanently, env.GetOlympusEndpoint())
+			} else if !isGithubPath(mod) {
+				c.Response().WriteHeader(http.StatusNotFound)


I think if there are no versions, this can return an empty list. What do you think?

@arschles returning no versions would cause Go to make an /@latest which is fine once we have a go get set up on the Proxy to work with SHAs. Although does Go tip expose a CLI command to retrieve latest??? This go get stuff you're doing wouldn't work otherwise 😓

arschles · 2018-07-19T19:32:31Z

pkg/download/list.go

+				// Go uses the http.DefaultClient which implicitly handles up to 10 reqdirects.
+				// therefore, it's safe to redirect to whatever server/endpoint we like as long
+				// as that server returns an answer that Go expects.
+				return c.Redirect(http.StatusMovedPermanently, env.GetOlympusEndpoint())


@marwan-at-work do you happen to know if the redirect is "officially supported" in go get? I'd like to make sure it's a somewhat properly specified feature of go get and we're not relying on an implementation detail in the CLI

as long as go get uses the standard library net/http DefaultClient (which it does) redirects are officially supported.

arschles · 2018-07-19T19:35:29Z

pkg/download/list.go

+	sourceVCS
+)
+
+func getInfoFromPath(ctx context.Context, path string) (importInfo, error) {


❤️ this function, but could we make it more vcs-agnostic? Perhaps we could do a go get -u $path and list the tags using git tags (only supporting github for now)?

Longer term, I think we should get this logic moved into the go toolchain so we can get tags for a module from the CLI. Thoughts?

@arschles do you mean only supporting git or only supporting github? If only supporting github, then calling the API directly should always work.

But if only supporting git, then go get makes sense here. However, what happens when we do go get some/package and then it's not git?

Download: get /list from upstream

181a99c

marwan-at-work added the do not merge discussion prs, opinion prs, etc. label Jul 18, 2018

arschles requested changes Jul 19, 2018

View reviewed changes

Proxy: move Olympus Endpoint to env

7e8c7c2

arschles requested changes Jul 19, 2018

View reviewed changes

revert makefile

5c7e869

arschles mentioned this pull request Jul 20, 2018

Big Kahuna: Do cache fills synchronously #290

Closed

Proxy: getCacheMissPath calls env for olympus ep

7ca7b67

marwan-at-work closed this Jul 26, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download: get /list from upstream #300

Download: get /list from upstream #300

marwan-at-work commented Jul 18, 2018

codecov-io commented Jul 18, 2018

arschles left a comment

arschles Jul 18, 2018

marwan-at-work Jul 19, 2018

marpio Jul 19, 2018 •

edited

Loading

arschles Jul 19, 2018

arschles Jul 18, 2018

marwan-at-work Jul 19, 2018

arschles Jul 18, 2018

marwan-at-work Jul 19, 2018

arschles Jul 18, 2018

marwan-at-work Jul 19, 2018

arschles Jul 19, 2018

arschles Jul 18, 2018

marwan-at-work Jul 19, 2018

arschles Jul 19, 2018

marwan-at-work commented Jul 19, 2018

arschles left a comment

arschles Jul 19, 2018

arschles Jul 19, 2018

arschles Jul 19, 2018

arschles Jul 19, 2018

marwan-at-work Jul 20, 2018

arschles Jul 19, 2018

marwan-at-work Jul 19, 2018

arschles Jul 19, 2018

marwan-at-work Jul 19, 2018

arschles Jul 19, 2018

marwan-at-work Jul 20, 2018

Download: get /list from upstream #300

Download: get /list from upstream #300

Conversation

marwan-at-work commented Jul 18, 2018

codecov-io commented Jul 18, 2018

Codecov Report

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

marpio Jul 19, 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

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

marwan-at-work commented Jul 19, 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

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

marpio Jul 19, 2018 •

edited

Loading