From 9b0220f7772933f71f869417014c7a9e00df7af4 Mon Sep 17 00:00:00 2001 From: Clara Fu Date: Tue, 18 Aug 2020 16:21:05 -0400 Subject: [PATCH] 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 Co-authored-by: Jamie Klassen --- accounts/accountant.go | 10 +- accounts/accountant_test.go | 44 ++++--- accounts/accounts.go | 39 +++--- accounts/accounts_test.go | 102 +++++++++++---- accounts/accountsfakes/fake_worker_factory.go | 115 ----------------- accounts/accountsfakes/fake_writer.go | 119 ++++++++++++++++++ accounts/worker_factory.go | 12 +- main.go | 1 + 8 files changed, 259 insertions(+), 183 deletions(-) delete mode 100644 accounts/accountsfakes/fake_worker_factory.go create mode 100644 accounts/accountsfakes/fake_writer.go diff --git a/accounts/accountant.go b/accounts/accountant.go index 6a9abb5..01f505b 100644 --- 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 index 6dbe1bc..a3a68e0 100644 --- 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 index 481ea1e..a33d70e 100644 --- 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 index 1186700..530bfa8 100644 --- 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 deleted file mode 100644 index c78804e..0000000 --- a/accounts/accountsfakes/fake_worker_factory.go +++ /dev/null @@ -1,115 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package accountsfakes - -import ( - "sync" - - "github.com/concourse/ft/accounts" -) - -type FakeWorkerFactory struct { - CreateWorkerStub func(accounts.Command) (accounts.Worker, error) - createWorkerMutex sync.RWMutex - createWorkerArgsForCall []struct { - arg1 accounts.Command - } - createWorkerReturns struct { - result1 accounts.Worker - result2 error - } - createWorkerReturnsOnCall map[int]struct { - result1 accounts.Worker - result2 error - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakeWorkerFactory) CreateWorker(arg1 accounts.Command) (accounts.Worker, error) { - fake.createWorkerMutex.Lock() - ret, specificReturn := fake.createWorkerReturnsOnCall[len(fake.createWorkerArgsForCall)] - fake.createWorkerArgsForCall = append(fake.createWorkerArgsForCall, struct { - arg1 accounts.Command - }{arg1}) - fake.recordInvocation("CreateWorker", []interface{}{arg1}) - fake.createWorkerMutex.Unlock() - if fake.CreateWorkerStub != nil { - return fake.CreateWorkerStub(arg1) - } - if specificReturn { - return ret.result1, ret.result2 - } - fakeReturns := fake.createWorkerReturns - return fakeReturns.result1, fakeReturns.result2 -} - -func (fake *FakeWorkerFactory) CreateWorkerCallCount() int { - fake.createWorkerMutex.RLock() - defer fake.createWorkerMutex.RUnlock() - return len(fake.createWorkerArgsForCall) -} - -func (fake *FakeWorkerFactory) CreateWorkerCalls(stub func(accounts.Command) (accounts.Worker, error)) { - fake.createWorkerMutex.Lock() - defer fake.createWorkerMutex.Unlock() - fake.CreateWorkerStub = stub -} - -func (fake *FakeWorkerFactory) CreateWorkerArgsForCall(i int) accounts.Command { - fake.createWorkerMutex.RLock() - defer fake.createWorkerMutex.RUnlock() - argsForCall := fake.createWorkerArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeWorkerFactory) CreateWorkerReturns(result1 accounts.Worker, result2 error) { - fake.createWorkerMutex.Lock() - defer fake.createWorkerMutex.Unlock() - fake.CreateWorkerStub = nil - fake.createWorkerReturns = struct { - result1 accounts.Worker - result2 error - }{result1, result2} -} - -func (fake *FakeWorkerFactory) CreateWorkerReturnsOnCall(i int, result1 accounts.Worker, result2 error) { - fake.createWorkerMutex.Lock() - defer fake.createWorkerMutex.Unlock() - fake.CreateWorkerStub = nil - if fake.createWorkerReturnsOnCall == nil { - fake.createWorkerReturnsOnCall = make(map[int]struct { - result1 accounts.Worker - result2 error - }) - } - fake.createWorkerReturnsOnCall[i] = struct { - result1 accounts.Worker - result2 error - }{result1, result2} -} - -func (fake *FakeWorkerFactory) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.createWorkerMutex.RLock() - defer fake.createWorkerMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakeWorkerFactory) recordInvocation(key string, args []interface{}) { - fake.invocationsMutex.Lock() - defer fake.invocationsMutex.Unlock() - if fake.invocations == nil { - fake.invocations = map[string][][]interface{}{} - } - if fake.invocations[key] == nil { - fake.invocations[key] = [][]interface{}{} - } - fake.invocations[key] = append(fake.invocations[key], args) -} - -var _ accounts.WorkerFactory = new(FakeWorkerFactory) diff --git a/accounts/accountsfakes/fake_writer.go b/accounts/accountsfakes/fake_writer.go new file mode 100644 index 0000000..75738da --- /dev/null +++ b/accounts/accountsfakes/fake_writer.go @@ -0,0 +1,119 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package accountsfakes + +import ( + "io" + "sync" +) + +type FakeWriter struct { + WriteStub func([]byte) (int, error) + writeMutex sync.RWMutex + writeArgsForCall []struct { + arg1 []byte + } + writeReturns struct { + result1 int + result2 error + } + writeReturnsOnCall map[int]struct { + result1 int + result2 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeWriter) Write(arg1 []byte) (int, error) { + var arg1Copy []byte + if arg1 != nil { + arg1Copy = make([]byte, len(arg1)) + copy(arg1Copy, arg1) + } + fake.writeMutex.Lock() + ret, specificReturn := fake.writeReturnsOnCall[len(fake.writeArgsForCall)] + fake.writeArgsForCall = append(fake.writeArgsForCall, struct { + arg1 []byte + }{arg1Copy}) + fake.recordInvocation("Write", []interface{}{arg1Copy}) + fake.writeMutex.Unlock() + if fake.WriteStub != nil { + return fake.WriteStub(arg1) + } + if specificReturn { + return ret.result1, ret.result2 + } + fakeReturns := fake.writeReturns + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeWriter) WriteCallCount() int { + fake.writeMutex.RLock() + defer fake.writeMutex.RUnlock() + return len(fake.writeArgsForCall) +} + +func (fake *FakeWriter) WriteCalls(stub func([]byte) (int, error)) { + fake.writeMutex.Lock() + defer fake.writeMutex.Unlock() + fake.WriteStub = stub +} + +func (fake *FakeWriter) WriteArgsForCall(i int) []byte { + fake.writeMutex.RLock() + defer fake.writeMutex.RUnlock() + argsForCall := fake.writeArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeWriter) WriteReturns(result1 int, result2 error) { + fake.writeMutex.Lock() + defer fake.writeMutex.Unlock() + fake.WriteStub = nil + fake.writeReturns = struct { + result1 int + result2 error + }{result1, result2} +} + +func (fake *FakeWriter) WriteReturnsOnCall(i int, result1 int, result2 error) { + fake.writeMutex.Lock() + defer fake.writeMutex.Unlock() + fake.WriteStub = nil + if fake.writeReturnsOnCall == nil { + fake.writeReturnsOnCall = make(map[int]struct { + result1 int + result2 error + }) + } + fake.writeReturnsOnCall[i] = struct { + result1 int + result2 error + }{result1, result2} +} + +func (fake *FakeWriter) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.writeMutex.RLock() + defer fake.writeMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeWriter) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ io.Writer = new(FakeWriter) diff --git a/accounts/worker_factory.go b/accounts/worker_factory.go index c5b5696..4f4d5c4 100644 --- a/accounts/worker_factory.go +++ b/accounts/worker_factory.go @@ -10,17 +10,9 @@ type Command struct { K8sPod string `long:"k8s-pod"` } -//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . WorkerFactory +type WorkerFactory func(Command) (Worker, error) -type WorkerFactory interface { - CreateWorker(Command) (Worker, error) -} - -type workerFactory struct{} - -var DefaultWorkerFactory WorkerFactory = &workerFactory{} - -func (wf *workerFactory) CreateWorker(cmd Command) (Worker, error) { +var DefaultWorkerFactory WorkerFactory = func(cmd Command) (Worker, error) { var dialer GardenDialer if cmd.K8sNamespace != "" && cmd.K8sPod != "" { restConfig, err := RESTConfig() diff --git a/main.go b/main.go index 4590395..0499e23 100644 --- a/main.go +++ b/main.go @@ -11,6 +11,7 @@ import ( func main() { returnCode := accounts.Execute( accounts.DefaultWorkerFactory, + accounts.DefaultAccountantFactory, os.Args[1:], os.Stdout, )