no more panics on errors

for the time being, AccountantFactory lives in accountant.go and does not return an error -- if the accountant-creation logic grows sufficiently, it will make sense to follow the example of WorkerFactory and place the type and default implementation in its own file. closes concourse/concourse#5994 Signed-off-by: Clara Fu <[email protected]> Co-authored-by: Jamie Klassen <[email protected]>
vmware-archive · Aug 18, 2020 · 9b0220f · 9b0220f
1 parent 9460a18
commit 9b0220f
Show file tree

Hide file tree

Showing 8 changed files with 259 additions and 183 deletions.
diff --git a/accounts/accountant.go b/accounts/accountant.go
@@ -8,16 +8,18 @@ import (
 	"github.com/concourse/flag"
 )
 
-func NewDBAccountant(pgc flag.PostgresConfig) Accountant {
-	return &DBAccountant{postgresConfig: pgc}
+type AccountantFactory func(Command) Accountant
+
+var DefaultAccountantFactory = func(cmd Command) Accountant {
+	return &DBAccountant{PostgresConfig: cmd.Postgres}
 }
 
 type DBAccountant struct {
-	postgresConfig flag.PostgresConfig
+	PostgresConfig flag.PostgresConfig
 }
 
 func (da *DBAccountant) Account(containers []Container) ([]Sample, error) {
-	conn, err := sql.Open("postgres", da.postgresConfig.ConnectionString())
+	conn, err := sql.Open("postgres", da.PostgresConfig.ConnectionString())
 	if err != nil {
 		return nil, err
 	}

diff --git a/accounts/accountant_test.go b/accounts/accountant_test.go
@@ -266,14 +266,16 @@ var _ = Describe("DBAccountant", func() {
 		}
 		createResources(resources)
 		checkResources()
-		accountant := accounts.NewDBAccountant(flag.PostgresConfig{
-			Host:     dbHost(),
-			Port:     5432,
-			User:     "postgres",
-			Password: "password",
-			Database: testDBName(),
-			SSLMode:  "disable",
-		})
+		accountant := &accounts.DBAccountant{
+			PostgresConfig: flag.PostgresConfig{
+				Host:     dbHost(),
+				Port:     5432,
+				User:     "postgres",
+				Password: "password",
+				Database: testDBName(),
+				SSLMode:  "disable",
+			},
+		}
 		Eventually(team.Containers).ShouldNot(BeEmpty())
 		containers := []accounts.Container{}
 		dbContainers, _ := team.Containers()
@@ -357,6 +359,14 @@ var _ = Describe("DBAccountant", func() {
 			}
 			return fakeProcess, nil
 		}
+		fakeGClientContainer.AttachStub = func(ctx context.Context, foo string, pi garden.ProcessIO) (garden.Process, error) {
+			fakeProcess := new(gardenfakes.FakeProcess)
+			fakeProcess.WaitStub = func() (int, error) {
+				io.WriteString(pi.Stdout, "[]")
+				return 0, nil
+			}
+			return fakeProcess, nil
+		}
 		fakeGClient.CreateReturns(fakeGClientContainer, nil)
 		fakeBaggageclaimClient := new(baggageclaimfakes.FakeClient)
 		fakeBaggageclaimVolume := new(baggageclaimfakes.FakeVolume)
@@ -382,14 +392,16 @@ var _ = Describe("DBAccountant", func() {
 
 		// TODO wait for build to complete?
 
-		accountant := accounts.NewDBAccountant(flag.PostgresConfig{
-			Host:     dbHost(),
-			Port:     5432,
-			User:     "postgres",
-			Password: "password",
-			Database: testDBName(),
-			SSLMode:  "disable",
-		})
+		accountant := &accounts.DBAccountant{
+			PostgresConfig: flag.PostgresConfig{
+				Host:     dbHost(),
+				Port:     5432,
+				User:     "postgres",
+				Password: "password",
+				Database: testDBName(),
+				SSLMode:  "disable",
+			},
+		}
 		Eventually(team.Containers).ShouldNot(BeEmpty())
 		containers := []accounts.Container{}
 		dbContainers, _ := team.Containers()

diff --git a/accounts/accounts.go b/accounts/accounts.go
@@ -11,28 +11,43 @@ import (
 	"github.com/jessevdk/go-flags"
 )
 
-func Execute(workerFactory WorkerFactory, args []string, stdout io.Writer) int {
+func Execute(workerFactory WorkerFactory, accountantFactory AccountantFactory, args []string, stdout io.Writer) int {
 	cmd, err := parseArgs(args)
 	if err != nil {
-		panic(err)
+		fmt.Fprintln(stdout, err.Error())
+		return flagErrorReturnCode(err)
 	}
-	worker, err := workerFactory.CreateWorker(cmd)
+	worker, err := workerFactory(cmd)
 	if err != nil {
-		panic(err)
+		fmt.Fprintf(stdout, "configuration error: %s\n", err.Error())
+		return 1
 	}
-	accountant := NewDBAccountant(cmd.Postgres)
-	samples, err := Account(worker, accountant)
+	accountant := accountantFactory(cmd)
+	containers, err := worker.Containers()
 	if err != nil {
-		fmt.Fprintln(stdout, err.Error())
+		fmt.Fprintf(stdout, "worker error: %s\n", err.Error())
+		return 1
+	}
+	samples, err := accountant.Account(containers)
+	if err != nil {
+		fmt.Fprintf(stdout, "accountant error: %s\n", err.Error())
 		return 1
 	}
 	err = printSamples(stdout, samples)
 	if err != nil {
-		panic(err)
+		return 1
 	}
 	return 0
 }
 
+func flagErrorReturnCode(err error) int {
+	ourErr, ok := err.(*flags.Error)
+	if ok && ourErr.Type == flags.ErrHelp {
+		return 0
+	}
+	return 1
+}
+
 func parseArgs(args []string) (Command, error) {
 	cmd := Command{}
 	parser := flags.NewParser(&cmd, flags.HelpFlag|flags.PassDoubleDash)
@@ -115,11 +130,3 @@ type Worker interface {
 }
 
 type StatsOption func()
-
-func Account(w Worker, a Accountant) ([]Sample, error) {
-	containers, err := w.Containers()
-	if err != nil {
-		return nil, err
-	}
-	return a.Account(containers)
-}
diff --git a/accounts/accounts_test.go b/accounts/accounts_test.go
@@ -10,49 +10,107 @@ import (
 	"github.com/onsi/gomega/gbytes"
 )
 
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 io.Writer
+
 var _ = Describe("Accounts", func() {
 	Describe("#Execute", func() {
 		It("handles worker errors", func() {
 			buf := gbytes.NewBuffer()
 			fakeWorker := new(accountsfakes.FakeWorker)
-			fakeWorker.ContainersReturns([]accounts.Container{}, errors.New("worker error"))
-			fakeWorkerFactory := new(accountsfakes.FakeWorkerFactory)
-			fakeWorkerFactory.CreateWorkerReturns(fakeWorker, nil)
+			fakeWorker.ContainersReturns([]accounts.Container{}, errors.New("pod not found"))
 
-			returnCode := accounts.Execute(fakeWorkerFactory, []string{}, buf)
+			returnCode := accounts.Execute(
+				func(accounts.Command) (accounts.Worker, error) {
+					return fakeWorker, nil
+				}, func(accounts.Command) accounts.Accountant {
+					return nil
+				},
+				[]string{},
+				buf,
+			)
 
 			Expect(returnCode).To(Equal(1))
-			Expect(buf).To(gbytes.Say("worker error"))
+			Expect(buf).To(gbytes.Say("worker error: pod not found\n"))
+		})
+
+		It("prints help text when `-h` is passed", func() {
+			buf := gbytes.NewBuffer()
+			returnCode := accounts.Execute(nil, nil, []string{"-h"}, buf)
+			Expect(returnCode).To(Equal(0))
+			Expect(buf).To(gbytes.Say("Usage"))
+		})
+
+		It("fails on flag parsing errors", func() {
+			buf := gbytes.NewBuffer()
+			returnCode := accounts.Execute(nil, nil, []string{"--invalid-flag"}, buf)
+			Expect(returnCode).To(Equal(1))
+			Expect(buf).To(gbytes.Say("invalid-flag"))
 		})
-	})
 
-	Describe("#Account", func() {
-		var (
-			worker     *accountsfakes.FakeWorker
-			accountant *accountsfakes.FakeAccountant
-		)
+		It("fails on kubectl errors", func() {
+			buf := gbytes.NewBuffer()
+			returnCode := accounts.Execute(func(accounts.Command) (accounts.Worker, error) {
+				return nil, errors.New("error loading config file")
+			}, nil, []string{}, buf)
 
-		BeforeEach(func() {
-			worker = new(accountsfakes.FakeWorker)
-			accountant = new(accountsfakes.FakeAccountant)
+			Expect(returnCode).To(Equal(1))
+			Expect(buf).To(gbytes.Say("configuration error: error loading config file\n"))
 		})
 
-		It("accounts the workloads for each container", func() {
+		It("fails on accountant errors", func() {
+			buf := gbytes.NewBuffer()
+			fakeWorker := new(accountsfakes.FakeWorker)
 			container := accounts.Container{Handle: "abc123"}
 			containers := []accounts.Container{container}
-			worker.ContainersReturns(containers, nil)
+			fakeWorker.ContainersReturns(containers, nil)
+			fakeAccountant := new(accountsfakes.FakeAccountant)
+			fakeAccountant.AccountReturns(
+				nil,
+				errors.New("accountant error"),
+			)
 
-			accounts.Account(worker, accountant)
+			returnCode := accounts.Execute(
+				func(accounts.Command) (accounts.Worker, error) {
+					return fakeWorker, nil
+				}, func(accounts.Command) accounts.Accountant {
+					return fakeAccountant
+				},
+				[]string{},
+				buf,
+			)
 
-			Expect(accountant.AccountArgsForCall(0)).To(Equal(containers))
+			Expect(returnCode).To(Equal(1))
+			Expect(buf).To(gbytes.Say("accountant error: accountant error\n"))
 		})
 
-		It("surfaces worker errors", func() {
-			worker.ContainersReturns(nil, errors.New("HTTP error"))
+		It("fails on terminal io errors", func() {
+			fakeWriter := new(accountsfakes.FakeWriter)
+			fakeWriter.WriteReturns(0, errors.New("terminal io error"))
+			fakeWorker := new(accountsfakes.FakeWorker)
+			container := accounts.Container{Handle: "abc123"}
+			containers := []accounts.Container{container}
+			fakeWorker.ContainersReturns(containers, nil)
+			fakeAccountant := new(accountsfakes.FakeAccountant)
+			fakeAccountant.AccountReturns(
+				[]accounts.Sample{
+					accounts.Sample{
+						Container: container,
+					},
+				},
+				nil,
+			)
 
-			_, err := accounts.Account(worker, accountant)
+			returnCode := accounts.Execute(
+				func(accounts.Command) (accounts.Worker, error) {
+					return fakeWorker, nil
+				}, func(accounts.Command) accounts.Accountant {
+					return fakeAccountant
+				},
+				[]string{},
+				fakeWriter,
+			)
 
-			Expect(err).NotTo(BeNil())
+			Expect(returnCode).To(Equal(1))
 		})
 	})
 })
diff --git a/accounts/accountsfakes/fake_worker_factory.go b/accounts/accountsfakes/fake_worker_factory.go