Repository: gomods/athens
Pull request #1309 (2019-07-19 04:03:08)

ref: upstream lister and it's impl to module pkg
it's a refactor about upstream lister which should in module package.

cc @arschles 

<!-- 
example: Fixes #123
-->


yittg: @marwan-at-work comments fixed, PTAL
arschles: @marwan-at-work are you ok with this to merge?

Patch:
diff --git a/cmd/proxy/actions/app_proxy.go b/cmd/proxy/actions/app_proxy.go
index 4906b4fb..d4784d31 100644
--- a/cmd/proxy/actions/app_proxy.go
+++ b/cmd/proxy/actions/app_proxy.go
@@ -78,7 +78,7 @@ func addProxyRoutes(
 		return err
 	}
 
-	lister := download.NewVCSLister(c.GoBinary, c.GoProxy, fs)
+	lister := module.NewVCSLister(c.GoBinary, c.GoProxy, fs)
 
 	withSingleFlight, err := getSingleFlight(c, s)
 	if err != nil {
diff --git a/pkg/download/protocol.go b/pkg/download/protocol.go
index b3e9a87a..e79b7b33 100644
--- a/pkg/download/protocol.go
+++ b/pkg/download/protocol.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/gomods/athens/pkg/download/mode"
 	"github.com/gomods/athens/pkg/errors"
+	"github.com/gomods/athens/pkg/module"
 	"github.com/gomods/athens/pkg/observ"
 	"github.com/gomods/athens/pkg/stash"
 	"github.com/gomods/athens/pkg/storage"
@@ -40,7 +41,7 @@ type Wrapper func(Protocol) Protocol
 type Opts struct {
 	Storage      storage.Backend
 	Stasher      stash.Stasher
-	Lister       UpstreamLister
+	Lister       module.UpstreamLister
 	DownloadFile *mode.DownloadFile
 }
 
@@ -65,7 +66,7 @@ type protocol struct {
 	df      *mode.DownloadFile
 	storage storage.Backend
 	stasher stash.Stasher
-	lister  UpstreamLister
+	lister  module.UpstreamLister
 }
 
 func (p *protocol) List(ctx context.Context, mod string) ([]string, error) {
diff --git a/pkg/module/go_get_fetcher.go b/pkg/module/go_get_fetcher.go
index 4f83baf7..81feac3d 100644
--- a/pkg/module/go_get_fetcher.go
+++ b/pkg/module/go_get_fetcher.go
@@ -8,7 +8,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"runtime"
 	"strings"
 
 	"github.com/gomods/athens/pkg/errors"
@@ -63,13 +62,13 @@ func (g *goGetFetcher) Fetch(ctx context.Context, mod, ver string) (*storage.Ver
 	sourcePath := filepath.Join(goPathRoot, "src")
 	modPath := filepath.Join(sourcePath, getRepoDirName(mod, ver))
 	if err := g.fs.MkdirAll(modPath, os.ModeDir|os.ModePerm); err != nil {
-		ClearFiles(g.fs, goPathRoot)
+		clearFiles(g.fs, goPathRoot)
 		return nil, errors.E(op, err)
 	}
 
 	m, err := downloadModule(g.goBinaryName, g.goProxy, g.fs, goPathRoot, modPath, mod, ver)
 	if err != nil {
-		ClearFiles(g.fs, goPathRoot)
+		clearFiles(g.fs, goPathRoot)
 		return nil, errors.E(op, err)
 	}
 
@@ -108,7 +107,7 @@ func downloadModule(goBinaryName, goProxy string, fs afero.Fs, gopath, repoRoot,
 	fullURI := fmt.Sprintf("%s@%s", uri, version)
 
 	cmd := exec.Command(goBinaryName, "mod", "download", "-json", fullURI)
-	cmd.Env = PrepareEnv(gopath, goProxy)
+	cmd.Env = prepareEnv(gopath, goProxy)
 	cmd.Dir = repoRoot
 	stdout := &bytes.Buffer{}
 	stderr := &bytes.Buffer{}
@@ -139,65 +138,6 @@ func downloadModule(goBinaryName, goProxy string, fs afero.Fs, gopath, repoRoot,
 	return m, nil
 }
 
-// PrepareEnv will return all the appropriate
-// environment variables for a Go Command to run
-// successfully (such as GOPATH, GOCACHE, PATH etc)
-func PrepareEnv(gopath, goProxy string) []string {
-	pathEnv := fmt.Sprintf("PATH=%s", os.Getenv("PATH"))
-	homeEnv := fmt.Sprintf("HOME=%s", os.Getenv("HOME"))
-	httpProxy := fmt.Sprintf("HTTP_PROXY=%s", os.Getenv("HTTP_PROXY"))
-	httpsProxy := fmt.Sprintf("HTTPS_PROXY=%s", os.Getenv("HTTPS_PROXY"))
-	noProxy := fmt.Sprintf("NO_PROXY=%s", os.Getenv("NO_PROXY"))
-	// need to also check the lower case version of just these three env variables
-	httpProxyLower := fmt.Sprintf("http_proxy=%s", os.Getenv("http_proxy"))
-	httpsProxyLower := fmt.Sprintf("https_proxy=%s", os.Getenv("https_proxy"))
-	noProxyLower := fmt.Sprintf("no_proxy=%s", os.Getenv("no_proxy"))
-	gopathEnv := fmt.Sprintf("GOPATH=%s", gopath)
-	goProxyEnv := fmt.Sprintf("GOPROXY=%s", goProxy)
-	cacheEnv := fmt.Sprintf("GOCACHE=%s", filepath.Join(gopath, "cache"))
-	gitSSH := fmt.Sprintf("GIT_SSH=%s", os.Getenv("GIT_SSH"))
-	gitSSHCmd := fmt.Sprintf("GIT_SSH_COMMAND=%s", os.Getenv("GIT_SSH_COMMAND"))
-	disableCgo := "CGO_ENABLED=0"
-	enableGoModules := "GO111MODULE=on"
-	cmdEnv := []string{
-		pathEnv,
-		homeEnv,
-		gopathEnv,
-		goProxyEnv,
-		cacheEnv,
-		disableCgo,
-		enableGoModules,
-		httpProxy,
-		httpsProxy,
-		noProxy,
-		httpProxyLower,
-		httpsProxyLower,
-		noProxyLower,
-		gitSSH,
-		gitSSHCmd,
-	}
-
-	if sshAuthSockVal, hasSSHAuthSock := os.LookupEnv("SSH_AUTH_SOCK"); hasSSHAuthSock {
-		// Verify that the ssh agent unix socket exists and is a unix socket.
-		st, err := os.Stat(sshAuthSockVal)
-		if err == nil && st.Mode()&os.ModeSocket != 0 {
-			sshAuthSock := fmt.Sprintf("SSH_AUTH_SOCK=%s", sshAuthSockVal)
-			cmdEnv = append(cmdEnv, sshAuthSock)
-		}
-	}
-
-	// add Windows specific ENV VARS
-	if runtime.GOOS == "windows" {
-		cmdEnv = append(cmdEnv, fmt.Sprintf("USERPROFILE=%s", os.Getenv("USERPROFILE")))
-		cmdEnv = append(cmdEnv, fmt.Sprintf("SystemRoot=%s", os.Getenv("SystemRoot")))
-		cmdEnv = append(cmdEnv, fmt.Sprintf("ALLUSERSPROFILE=%s", os.Getenv("ALLUSERSPROFILE")))
-		cmdEnv = append(cmdEnv, fmt.Sprintf("HOMEDRIVE=%s", os.Getenv("HOMEDRIVE")))
-		cmdEnv = append(cmdEnv, fmt.Sprintf("HOMEPATH=%s", os.Getenv("HOMEPATH")))
-	}
-
-	return cmdEnv
-}
-
 func isLimitHit(o string) bool {
 	return strings.Contains(o, "403 response from api.github.com")
 }
diff --git a/pkg/download/upstream_lister.go b/pkg/module/go_vcs_lister.go
similarity index 85%
rename from pkg/download/upstream_lister.go
rename to pkg/module/go_vcs_lister.go
index 7d625647..57f54f85 100644
--- a/pkg/download/upstream_lister.go
+++ b/pkg/module/go_vcs_lister.go
@@ -1,4 +1,4 @@
-package download
+package module
 
 import (
 	"bytes"
@@ -10,18 +10,11 @@ import (
 
 	"github.com/gomods/athens/pkg/config"
 	"github.com/gomods/athens/pkg/errors"
-	"github.com/gomods/athens/pkg/module"
 	"github.com/gomods/athens/pkg/observ"
 	"github.com/gomods/athens/pkg/storage"
 	"github.com/spf13/afero"
 )
 
-// UpstreamLister retrieves a list of available module versions from upstream
-// i.e. VCS, and a Storage backend.
-type UpstreamLister interface {
-	List(ctx context.Context, mod string) (*storage.RevInfo, []string, error)
-}
-
 type listResp struct {
 	Path     string
 	Version  string
@@ -61,8 +54,8 @@ func (l *vcsLister) List(ctx context.Context, mod string) (*storage.RevInfo, []s
 	if err != nil {
 		return nil, nil, errors.E(op, err)
 	}
-	defer module.ClearFiles(l.fs, gopath)
-	cmd.Env = module.PrepareEnv(gopath, l.goProxy)
+	defer clearFiles(l.fs, gopath)
+	cmd.Env = prepareEnv(gopath, l.goProxy)
 
 	err = cmd.Run()
 	if err != nil {
diff --git a/pkg/module/prepare_env.go b/pkg/module/prepare_env.go
new file mode 100644
index 00000000..2b3042da
--- /dev/null
+++ b/pkg/module/prepare_env.go
@@ -0,0 +1,67 @@
+package module
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"runtime"
+)
+
+// prepareEnv will return all the appropriate
+// environment variables for a Go Command to run
+// successfully (such as GOPATH, GOCACHE, PATH etc)
+func prepareEnv(gopath, goProxy string) []string {
+	pathEnv := fmt.Sprintf("PATH=%s", os.Getenv("PATH"))
+	homeEnv := fmt.Sprintf("HOME=%s", os.Getenv("HOME"))
+	httpProxy := fmt.Sprintf("HTTP_PROXY=%s", os.Getenv("HTTP_PROXY"))
+	httpsProxy := fmt.Sprintf("HTTPS_PROXY=%s", os.Getenv("HTTPS_PROXY"))
+	noProxy := fmt.Sprintf("NO_PROXY=%s", os.Getenv("NO_PROXY"))
+	// need to also check the lower case version of just these three env variables
+	httpProxyLower := fmt.Sprintf("http_proxy=%s", os.Getenv("http_proxy"))
+	httpsProxyLower := fmt.Sprintf("https_proxy=%s", os.Getenv("https_proxy"))
+	noProxyLower := fmt.Sprintf("no_proxy=%s", os.Getenv("no_proxy"))
+	gopathEnv := fmt.Sprintf("GOPATH=%s", gopath)
+	goProxyEnv := fmt.Sprintf("GOPROXY=%s", goProxy)
+	cacheEnv := fmt.Sprintf("GOCACHE=%s", filepath.Join(gopath, "cache"))
+	gitSSH := fmt.Sprintf("GIT_SSH=%s", os.Getenv("GIT_SSH"))
+	gitSSHCmd := fmt.Sprintf("GIT_SSH_COMMAND=%s", os.Getenv("GIT_SSH_COMMAND"))
+	disableCgo := "CGO_ENABLED=0"
+	enableGoModules := "GO111MODULE=on"
+	cmdEnv := []string{
+		pathEnv,
+		homeEnv,
+		gopathEnv,
+		goProxyEnv,
+		cacheEnv,
+		disableCgo,
+		enableGoModules,
+		httpProxy,
+		httpsProxy,
+		noProxy,
+		httpProxyLower,
+		httpsProxyLower,
+		noProxyLower,
+		gitSSH,
+		gitSSHCmd,
+	}
+
+	if sshAuthSockVal, hasSSHAuthSock := os.LookupEnv("SSH_AUTH_SOCK"); hasSSHAuthSock {
+		// Verify that the ssh agent unix socket exists and is a unix socket.
+		st, err := os.Stat(sshAuthSockVal)
+		if err == nil && st.Mode()&os.ModeSocket != 0 {
+			sshAuthSock := fmt.Sprintf("SSH_AUTH_SOCK=%s", sshAuthSockVal)
+			cmdEnv = append(cmdEnv, sshAuthSock)
+		}
+	}
+
+	// add Windows specific ENV VARS
+	if runtime.GOOS == "windows" {
+		cmdEnv = append(cmdEnv, fmt.Sprintf("USERPROFILE=%s", os.Getenv("USERPROFILE")))
+		cmdEnv = append(cmdEnv, fmt.Sprintf("SystemRoot=%s", os.Getenv("SystemRoot")))
+		cmdEnv = append(cmdEnv, fmt.Sprintf("ALLUSERSPROFILE=%s", os.Getenv("ALLUSERSPROFILE")))
+		cmdEnv = append(cmdEnv, fmt.Sprintf("HOMEDRIVE=%s", os.Getenv("HOMEDRIVE")))
+		cmdEnv = append(cmdEnv, fmt.Sprintf("HOMEPATH=%s", os.Getenv("HOMEPATH")))
+	}
+
+	return cmdEnv
+}
diff --git a/pkg/module/upstream_lister.go b/pkg/module/upstream_lister.go
new file mode 100644
index 00000000..19fb1d6e
--- /dev/null
+++ b/pkg/module/upstream_lister.go
@@ -0,0 +1,13 @@
+package module
+
+import (
+	"context"
+
+	"github.com/gomods/athens/pkg/storage"
+)
+
+// UpstreamLister retrieves a list of available module versions from upstream
+// i.e. VCS, and a Storage backend.
+type UpstreamLister interface {
+	List(ctx context.Context, mod string) (*storage.RevInfo, []string, error)
+}
diff --git a/pkg/module/zip_read_closer.go b/pkg/module/zip_read_closer.go
index 724f216f..3f24f933 100644
--- a/pkg/module/zip_read_closer.go
+++ b/pkg/module/zip_read_closer.go
@@ -18,16 +18,16 @@ type zipReadCloser struct {
 // It is the caller's responsibility to call this method to free up utilized disk space
 func (rc *zipReadCloser) Close() error {
 	rc.zip.Close()
-	return ClearFiles(rc.fs, rc.goPath)
+	return clearFiles(rc.fs, rc.goPath)
 }
 
 func (rc *zipReadCloser) Read(p []byte) (n int, err error) {
 	return rc.zip.Read(p)
 }
 
-// ClearFiles deletes all data from the given fs at path root
+// clearFiles deletes all data from the given fs at path root
 // This function must be called when zip is closed to cleanup the entire GOPATH created by the diskref
-func ClearFiles(fs afero.Fs, root string) error {
+func clearFiles(fs afero.Fs, root string) error {
 	const op errors.Op = "module.ClearFiles"
 	// This is required because vgo ensures dependencies are read-only
 	// See https://github.com/golang/go/issues/24111 and


Test patch:
diff --git a/pkg/download/protocol_test.go b/pkg/download/protocol_test.go
index 74ad937f..efb5ebee 100644
--- a/pkg/download/protocol_test.go
+++ b/pkg/download/protocol_test.go
@@ -46,7 +46,7 @@ func getDP(t *testing.T) Protocol {
 		t.Fatal(err)
 	}
 	st := stash.New(mf, s)
-	return New(&Opts{s, st, NewVCSLister(goBin, goProxy, fs), nil})
+	return New(&Opts{s, st, module.NewVCSLister(goBin, goProxy, fs), nil})
 }
 
 type listTest struct {