From 85213f20b3e953ee07f8e48f85b507dbb6bb7219 Mon Sep 17 00:00:00 2001 From: Dinesh Kumar <5300055+devdinu@users.noreply.github.com> Date: Thu, 2 Aug 2018 19:30:37 +0530 Subject: [PATCH] Goget Fetcher should error out if gobinpath is not valid (#377) * Goget fetcher should error out if gobinpath is not valid * Propogating the error from goget initialisation to main, to stop application * wrapping errors with op, using exec.command(gobin).Run to verify gobin, clean up * Inlining afero fs to goget call, and new go getfetcher * Revert "Inlining afero fs to goget call, and new go getfetcher" This reverts commit ae31fe6a2b6da3bfb8ff80f8aefba05e2fb94386. * Fixing example test --- cmd/olympus/actions/app.go | 6 +++++- cmd/proxy/actions/app_proxy.go | 6 +++++- pkg/download/goget/goget.go | 20 ++++++++++++++------ pkg/download/goget/goget_test.go | 5 ++++- pkg/module/go_get_fetcher.go | 18 ++++++++++++++++-- pkg/module/go_get_fetcher_test.go | 22 ++++++++++++++++++---- 6 files changed, 62 insertions(+), 15 deletions(-) diff --git a/cmd/olympus/actions/app.go b/cmd/olympus/actions/app.go index bf3627a64..ab85a10e2 100644 --- a/cmd/olympus/actions/app.go +++ b/cmd/olympus/actions/app.go @@ -113,7 +113,11 @@ func App(config *AppConfig) (*buffalo.App, error) { app.POST("/push", pushNotificationHandler(w)) // Download Protocol - dp := download.New(goget.New(), config.Storage) + gg, err := goget.New() + if err != nil { + return nil, err + } + dp := download.New(gg, config.Storage) app.GET(download.PathList, download.ListHandler(dp, lggr, renderEng)) app.GET(download.PathVersionInfo, download.VersionInfoHandler(dp, lggr, renderEng)) app.GET(download.PathVersionModule, download.VersionModuleHandler(dp, lggr, renderEng)) diff --git a/cmd/proxy/actions/app_proxy.go b/cmd/proxy/actions/app_proxy.go index 25dfbd59d..ce2da7f18 100644 --- a/cmd/proxy/actions/app_proxy.go +++ b/cmd/proxy/actions/app_proxy.go @@ -17,7 +17,11 @@ func addProxyRoutes( ) error { app.GET("/", proxyHomeHandler) - dp := download.New(goget.New(), storage) + gg, err := goget.New() + if err != nil { + return err + } + dp := download.New(gg, storage) // Download Protocol app.GET(download.PathList, download.ListHandler(dp, lggr, proxy)) app.GET(download.PathLatest, download.LatestHandler(dp, lggr, proxy)) diff --git a/pkg/download/goget/goget.go b/pkg/download/goget/goget.go index c0400afeb..7a7113023 100644 --- a/pkg/download/goget/goget.go +++ b/pkg/download/goget/goget.go @@ -22,15 +22,24 @@ import ( // New returns a download protocol by using // go get. You must have a modules supported // go binary for this to work. -func New() download.Protocol { - return &goget{ - goBinPath: env.GoBinPath(), - fs: afero.NewOsFs(), +func New() (download.Protocol, error) { + const op errors.Op = "goget.New" + goBin := env.GoBinPath() + fs := afero.NewOsFs() + mf, err := module.NewGoGetFetcher(goBin, fs) + if err != nil { + return nil, errors.E(op, err) } + return &goget{ + goBinPath: goBin, + fetcher: mf, + fs: fs, + }, nil } type goget struct { goBinPath string + fetcher module.Fetcher fs afero.Fs } @@ -139,8 +148,7 @@ func (gg *goget) Zip(ctx context.Context, mod, ver string) (io.ReadCloser, error func (gg *goget) Version(ctx context.Context, mod, ver string) (*storage.Version, error) { const op errors.Op = "goget.Version" - fetcher := module.NewGoGetFetcher(gg.goBinPath, gg.fs) - ref, err := fetcher.Fetch(mod, ver) + ref, err := gg.fetcher.Fetch(mod, ver) if err != nil { return nil, errors.E(op, err) } diff --git a/pkg/download/goget/goget_test.go b/pkg/download/goget/goget_test.go index 6b61b84ea..8dc80f360 100644 --- a/pkg/download/goget/goget_test.go +++ b/pkg/download/goget/goget_test.go @@ -3,6 +3,8 @@ package goget import ( "context" "testing" + + "github.com/stretchr/testify/require" ) type testCase struct { @@ -23,7 +25,8 @@ var tt = []testCase{ } func TestList(t *testing.T) { - dp := New() + dp, err := New() + require.NoError(t, err, "failed to create protocol") ctx := context.Background() for _, tc := range tt { diff --git a/pkg/module/go_get_fetcher.go b/pkg/module/go_get_fetcher.go index 8e3ba4883..10de0518d 100644 --- a/pkg/module/go_get_fetcher.go +++ b/pkg/module/go_get_fetcher.go @@ -19,11 +19,15 @@ type goGetFetcher struct { } // NewGoGetFetcher creates fetcher which uses go get tool to fetch modules -func NewGoGetFetcher(goBinaryName string, fs afero.Fs) Fetcher { +func NewGoGetFetcher(goBinaryName string, fs afero.Fs) (Fetcher, error) { + const op errors.Op = "module.NewGoGetFetcher" + if err := validGoBinary(goBinaryName); err != nil { + return nil, errors.E(op, err) + } return &goGetFetcher{ fs: fs, goBinaryName: goBinaryName, - } + }, nil } // Fetch downloads the sources and returns path where it can be found. Make sure to call Clear @@ -148,3 +152,13 @@ func getRepoDirName(repoURI, version string) string { func getPackagePath(gopath, module string) string { return filepath.Join(gopath, "src", "mod", "cache", "download", module, "@v") } + +func validGoBinary(name string) error { + const op errors.Op = "module.validGoBinary" + err := exec.Command(name).Run() + _, ok := err.(*exec.ExitError) + if err != nil && !ok { + return errors.E(op, err) + } + return nil +} diff --git a/pkg/module/go_get_fetcher_test.go b/pkg/module/go_get_fetcher_test.go index 857282de7..22f08214e 100644 --- a/pkg/module/go_get_fetcher_test.go +++ b/pkg/module/go_get_fetcher_test.go @@ -3,26 +3,37 @@ package module import ( "fmt" "io/ioutil" + "log" "github.com/gomods/athens/pkg/config/env" + "github.com/stretchr/testify/assert" "github.com/spf13/afero" ) func (s *ModuleSuite) TestNewGoGetFetcher() { r := s.Require() - fetcher := NewGoGetFetcher(s.goBinaryName, s.fs) + fetcher, err := NewGoGetFetcher(s.goBinaryName, s.fs) + r.NoError(err) _, ok := fetcher.(*goGetFetcher) r.True(ok) } +func (s *ModuleSuite) TestGoGetFetcherError() { + fetcher, err := NewGoGetFetcher("invalidpath", afero.NewOsFs()) + + assert.Nil(s.T(), fetcher) + assert.EqualError(s.T(), err, "exec: \"invalidpath\": executable file not found in $PATH") +} + func (s *ModuleSuite) TestGoGetFetcherFetch() { r := s.Require() // we need to use an OS filesystem because fetch executes vgo on the command line, which // always writes to the filesystem - fetcher := NewGoGetFetcher(s.goBinaryName, afero.NewOsFs()) - ref, err := fetcher.Fetch(repoURI, version) + fetcher, err := NewGoGetFetcher(s.goBinaryName, afero.NewOsFs()) r.NoError(err) + ref, err := fetcher.Fetch(repoURI, version) + r.NoError(err, "fetch shouldn't error") ver, err := ref.Read() r.NoError(err) defer ver.Zip.Close() @@ -46,7 +57,10 @@ func ExampleFetch() { repoURI := "github.com/arschles/assert" version := "v1.0.0" goBinaryName := env.GoBinPath() - fetcher := NewGoGetFetcher(goBinaryName, afero.NewOsFs()) + fetcher, err := NewGoGetFetcher(goBinaryName, afero.NewOsFs()) + if err != nil { + log.Fatal(err) + } ref, err := fetcher.Fetch(repoURI, version) // handle errors if any if err != nil {