Moving the fetch logic into 'pkg/module' #291

arschles · 2018-07-18T00:42:59Z

This copies the stuff in ./pkg/repo into ./pkg/module, but leaves ./pkg/repo (see #314 and #316 for removing ./pkg/repo).

It also refactors the interface for how we download code from an upstream (VCS), access it, and prepare it for upload to storage/CDN/wherever. During the copy from ./pkg/repo, I only retained the go get implementation in the new package, and not the Github API implementation (see #288 for why)

I am doing this refactor to prepare for #290, where I'll use the new functionality to synchronously fetch packages. I'll do that in a separate PR 😄

TODO:

Make the (./pkg/module).diskRef look in the global cache for the .info, .mod, and .zip files (right now I believe it looks in the wrong place)
~~Remove ./pkg/repo~~ (see Removing ./pkg/repo #316)

This is part of #290
There are a bunch of PRs opened against this branch. Check out #290 (comment) for what they are

ghost · 2018-07-19T04:23:18Z

.gitignore

@@ -24,3 +24,6 @@ cmd/proxy/proxy
 cmd/olympus/olympus
 test-keys.json
 tmp
+cmd/olympus/bin/olympus
+cmd/proxy/bin/proxy
+.vs


as in #303 this appears to be .vscode, at least by default on linux running non-insiders edition.

@robjloranger good catch on these lines. I am going to back these out and just wait for #303 to go in, so we can discuss there

ghost · 2018-07-19T04:45:26Z

This one is meaty, I'll take a proper look tomorrow morning sometime. There is a failing test though.

arschles · 2018-07-19T16:57:56Z

@robjloranger thanks! I'll fix that test today and hopefully finish this up as well.

ghost

Just questions/comments besides the one for the Ref Read method comment in ./pkg/module/ref.go

ghost · 2018-07-19T16:45:24Z

pkg/module/disk_ref.go

+
+// read is the Ref interface implementation.
+func (d *diskRef) Read() (*storage.Version, error) {
+	ver := &storage.Version{}


I prefer to declare variables of a type, and later on return pass the address of that variable. It becomes more clear in larger functions what is being returned. If you just glance at the end of the function and read the return, what is being returned?

@robjloranger so would you prefer that I do var ver *storage.Version at the beginning of the function?

No because that would still be a pointer type, I find it's more clear to read the return of a variable address in place of returning a variable that is not clearly a pointer. i.e. var ver storage.Version then on return return &ver

ah, ok got it. I'll make that change

ghost · 2018-07-19T19:01:48Z

pkg/module/go_get_fetcher.go

+)
+
+const (
+	tmpRepoDir = "%s-%s" // owner-repo-ref


I think this is probably fine as is, but at first I wasn't sure it the comment was meant to reference three parts of the string. If you agree it's a bit confusing, maybe something like owner-ref, owner-version etc

@robjloranger I'll abstract this into a function so that it's much easier to read and more type-safe

ghost · 2018-07-19T19:37:30Z

pkg/module/ref.go

+	// this function may fail, regardless of whether this function returns nil or not
+	Clear() error
+	// Read reads the module into memory and returns it. The caller should call
+	// the returned function.


Which function? Something in the storage.Version?

Oops, that's outdated documentation. Removing

ghost · 2018-07-19T19:42:42Z

pkg/module/go_get_fetcher.go

+
+	packagePath := filepath.Join(gopath, "src", "mod", "cache", "download", repoURI, "@v")
+
+	o, err := cmd.CombinedOutput()


I'm wondering if instead of the error check and eventual return, we can just have the switch.

Since the default case is the same as the return, and if the cmd output contains the limit error we return our custom error.
The only other check is on the files existing, in which case the only time the default case will be executed is when there is some actual error from the vgo get command.

I think you don't want to do these checks (isLimit and checkFiles) unnecessarily @robjloranger .
The alternative would be to do something like
case err != nil && isLimit
case err != nil && checkFiles
but I'm not sure if it's that much better.

I've simplified this to not do the switch inside the if. hopefully it's a bit more clear what's going on, even if it's more nested

marpio · 2018-07-20T13:57:48Z

pkg/module/go_get_fetcher.go

+		case isLimitHit(o):
+			// github quota exceeded
+			return packagePath, ErrLimitExceeded
+		case checkFiles(fs, packagePath, version) == nil:


Shouldn't this be case checkFiles(fs, packagePath, version) != nil:

good catch, change incoming

in favor of a function that does the same thing

hopefully this is easier to read

the ref returned from fetch does it

calls to fetch will fail if vgo is not installed

ghost · 2018-07-25T22:22:59Z

Just want to double check with you two @marpio and @marwan-at-work that your concerns were addressed.

marpio · 2018-07-26T08:56:33Z

Lgtm @robjloranger

marwan-at-work

Looks great. Mostly questions and nits.

I only put a couple of comments about using the new errors package because I'm eager to have this PR merged in. Once it's merged in, I'll go and change most of these files to fully use the errors package.

Plus, I'd like to use this PR to implement go list -m -versions in my List PR.

@arschles I do have one concern though: what happens when you use NewGoGetFetcher to concurrently fetch the same module? Just error out for now?

marwan-at-work · 2018-07-26T03:18:47Z

.travis.yml

@@ -15,6 +15,7 @@ env:
    - MINIO_SECRET_KEY=minio123
    - ATHENS_MONGO_STORAGE_URL=mongodb://127.0.0.1:27017
    - CODE_COV=1
+    - GO_BINARY_NAME=vgo


nit: would GO_BINARY_PATH make more sense? Just because you can put put a whole path to binary and not necessary a name of binary in your PATH environment variable.

That would probably be more clear

marwan-at-work · 2018-07-26T13:39:10Z

pkg/module/go_get_fetcher.go

+// should always call ref.Clear() on the returned ref
+func (g *goGetFetcher) Fetch(mod, ver string) (Ref, error) {
+	var ref Ref
+	ref = noopRef{}


if you wanna make it one line: var ref Ref = noopRef{} or ref := Ref(noopRef{})

marwan-at-work · 2018-07-26T14:00:18Z

pkg/module/go_get_fetcher.go

+	}
+
+	sourcePath := filepath.Join(repoRoot, "mod.go")
+	sourceContent := []byte(`package mod // import "mod"`)


no need for // import "mod" since you have a go.mod file saying that

marwan-at-work · 2018-07-26T14:06:55Z

pkg/module/go_get_fetcher.go

+//
+// If an error was returned, the returned ref's Read method will return an error, but you
+// should always call ref.Clear() on the returned ref
+func (g *goGetFetcher) Fetch(mod, ver string) (Ref, error) {


so why do we always wanna return a Ref? why can't we just return nil? If it's a matter of calling ref.Clear() we can always make sure we clear things inside this function when an error occurs, no?

also, why does Fetch takes a mod/ver when NewGoGetFetcher also takes a mod and ver?

also, why does Fetch takes a mod/ver when NewGoGetFetcher also takes a mod and ver?

good catch, only Fetch should take the mod/ver

@marwan-at-work I guess it can return nil and manually call ref.Clear, yea. Good idea - change incoming

marwan-at-work · 2018-07-26T14:10:55Z

pkg/module/go_get_fetcher.go

+// the location of the module cache so you can delete it if necessary
+func getSources(goBinaryName string, fs afero.Fs, gopath, repoRoot, module, version string) (string, error) {
+	version = strings.TrimPrefix(version, "@")
+	if !strings.HasPrefix(version, "v") {


from my previous review:

you shouldn't need this whole block in the first place. version is always going to include v and therefore you can remove the whole if !strings.HasPrefix(version, "v") block. If you really don't trust the caller, then calling semver.IsValid is probably more wholesome. But imo, just going out and saying go get module@version is good enough because Go will validate the version anyway

Good point. Since the version is ultimately going to come from a REST API call, I want to be defensive against version, so I'll use semver.IsValid

(instead of relying on go get to tell us that the version is invalid when we call)

marwan-at-work · 2018-07-26T14:18:01Z

pkg/module/go_get_fetcher.go

+
+var (
+	// ErrLimitExceeded signals that github.com refused to serve the request due to exceeded quota
+	ErrLimitExceeded = errors.New("github limit exceeded")


This seems to be not used anywhere, furthermore it seems that goget.go and goget_test.go` are still around. Can we delete them in this pr?

It was used on ~129 after the isLimitHit check

marwan-at-work · 2018-07-26T14:21:20Z

pkg/module/go_get_fetcher.go

+	if err != nil {
+		// github quota exceeded
+		if isLimitHit(o) {
+			return packagePath, pkgerrors.WithMessage(err, "github API limit hit")


let's make sure we propagate the rate limit status all the way back to vgo so it knows to show the right message. You can do return packagePath, errors.E(op, err, http.StatusTooManyRequests) -- we should also add that as a known Kind in the errors package. So in the future we can do something like
if errors.Kind(err) == errors.KindRateLimit or wrap that in a function like if errors.ErrRateLimit(err)

the function in the errors package can look like this:

const KindRateLimit = http.StatusTooManyRequests func ErrRateLimit(err error) { return Kind(err) == KindRateLimit }

marwan-at-work · 2018-07-26T14:41:00Z

pkg/module/errors.go

+
+// NewErrModuleAlreadyFetched returns a new ErrModuleAlreadyFetched
+func NewErrModuleAlreadyFetched(mod, ver string) error {
+	return &ErrModuleAlreadyFetched{module: mod, version: ver}


return errors.E(op, errors.M(mod), errors.V(ver), errors.KindAlreadyExists) (passing the op into this function constructor) and pick whichever logrus Level you deem is appropriate. I think error level makes sense here, because why ever fetch something if it's already in storage right? Not passing a level default the error to be logrus.ErrorLevel.

In the errors package, create a new kind enum KindAlreadyExists = http.StatusConflict

marwan-at-work · 2018-07-26T14:41:45Z

pkg/module/errors.go

+
+// ErrModuleAlreadyFetched is an error returned when you try to fetch the same module@version
+// more than once
+type ErrModuleAlreadyFetched struct {


you can remove this struct and its methods :)

marwan-at-work · 2018-07-26T14:42:52Z

pkg/module/fetcher.go

+package module
+
+// Fetcher fetches module from an upstream source
+type Fetcher interface {


sounds good to me 👍

#291 (comment)

and checking version format

marwan-at-work

LGTM 💯 🥇 🔨
Can you remove the pkg/module/error ErrModuleAlreadyFetched struct before merging? No need for a second look on my side!

arschles · 2018-07-26T19:36:02Z

@marwan-at-work after I fix CI and remove that struct, will do - thanks!

arschles mentioned this pull request Jul 19, 2018

Big Kahuna: Do cache fills synchronously #290

Closed

arschles requested review from a user and marwan-at-work July 19, 2018 01:37

arschles added the refactor work to do to refactor code label Jul 19, 2018

ghost reviewed Jul 19, 2018

View reviewed changes

ghost suggested changes Jul 19, 2018

View reviewed changes

marpio reviewed Jul 20, 2018

View reviewed changes

arschles added 9 commits July 20, 2018 11:19

add module fetch and fetchers

350a3aa

Breaking module ref out of the fetcher

3e59902

disk ref in progress

67f4c14

very broken go get fetcher

4d7e642

refactoring ref and fixing up the disk ref

e986b54

adding a test for the disk ref

bf325db

doing test setup right

afdd28a

passing version

2e6af1c

fixing syntax err

30fbe8c

arschles force-pushed the sync-fill branch from 2176005 to 30fbe8c Compare July 20, 2018 18:20

arschles added 11 commits July 20, 2018 11:24

removing raw format string

d277d16

in favor of a function that does the same thing

removing obsolete godoc

a65c0dd

removing the switch in favor of nested if statements

82e77d8

hopefully this is easier to read

passing data into the format strings

f087474

refactoring the closer

128c18d

creating and looking for files in the root

5581590

genericFetcher => goGetFetcher

fbf3fe6

remove the clear method on go get fetcher

5e7cc90

the ref returned from fetch does it

remove unnecessary call to isVgoInstalled

1076d54

calls to fetch will fail if vgo is not installed

remove unneeded dirName

bec1380

adding preliminary test for the go get fetcher

1cb75e4

arschles added 3 commits July 25, 2018 14:14

defer closing the zip

5a9f4e0

closing the zip

862a6c4

adding comment about closing the ver.Zip

9d1cfc6

ghost approved these changes Jul 25, 2018

View reviewed changes

ghost requested review from marpio and marwan-at-work July 25, 2018 22:22

Merge branch 'master' into sync-fill

f2d38b9

marwan-at-work requested changes Jul 26, 2018

View reviewed changes

ghost removed the request for review from marpio July 26, 2018 17:12

arschles added 9 commits July 26, 2018 10:52

GO_BINARY_NAME => GO_BINARY_PATH

33ab8a9

using errors pkg

f415572

fixing compile err

cc5c54e

removing redundant args

209d6ee

removing requirement that caller calls Clear on fetch error

a0781f1

fixing build

d841adb

fix

1af8cc9

adding masterminds semver

7e6464e

returning better error

d1eb1e0

and checking version format

marwan-at-work approved these changes Jul 26, 2018

View reviewed changes

arschles added 4 commits July 26, 2018 12:37

removing unused error

10d453c

using proper semver function

1d5e0ae

fixin compile err

7e80766

rm that shizz

1e0e6cb

arschles merged commit ab9a4c0 into gomods:master Jul 26, 2018

arschles deleted the sync-fill branch July 26, 2018 20:08

arschles mentioned this pull request Jul 26, 2018

Clean up ./pkg/repo #335

Closed


		packagePath := filepath.Join(gopath, "src", "mod", "cache", "download", repoURI, "@v")

		o, err := cmd.CombinedOutput()

Moving the fetch logic into 'pkg/module' #291

Moving the fetch logic into 'pkg/module' #291

Conversation

arschles commented Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 19, 2018

arschles commented Jul 19, 2018

ghost 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

marpio Jul 20, 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 commented Jul 25, 2018

marpio commented Jul 26, 2018

marwan-at-work 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

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 Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

marwan-at-work Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marwan-at-work left a comment

Choose a reason for hiding this comment

arschles commented Jul 26, 2018

arschles commented Jul 18, 2018 •

edited

Loading

marpio Jul 20, 2018 •

edited

Loading

marwan-at-work left a comment •

edited

Loading

marwan-at-work Jul 26, 2018 •

edited

Loading

marwan-at-work Jul 26, 2018 •

edited

Loading