diff --git a/Makefile b/Makefile index 98137b6..fbdf11a 100644 --- a/Makefile +++ b/Makefile @@ -49,7 +49,7 @@ $(CLIENT_BINARY): client client/*.go client/**/*.go # This avoids having to redownload dependencies and having to configure # toolchains/ go env vars $(TEST_BINARY): $(GRPC_FILES) $(CERTS) *.go go.mod go.sum - -go test -covermode=count -o $@ -tags sudo > /dev/null + go test -covermode=count -c -o $@ -tags sudo > /dev/null .PHONY: test test: $(TEST_BINARY) diff --git a/dispatcher.go b/dispatcher.go index d1e1b75..35388e5 100644 --- a/dispatcher.go +++ b/dispatcher.go @@ -11,8 +11,9 @@ import ( ) var ( - errNoService = status.Error(codes.InvalidArgument, "missing service name") - errServiceNotExist = status.Error(codes.InvalidArgument, "service does not exist") + errNoService = status.Error(codes.InvalidArgument, "missing service name") + errServiceNotExist = status.Error(codes.InvalidArgument, "service does not exist") + errServiceDodgyConf = status.Error(codes.FailedPrecondition, "service config is incorrect") ) type Dispatcher struct { diff --git a/dispatcher_test.go b/dispatcher_test.go index aa18ab5..02aa101 100644 --- a/dispatcher_test.go +++ b/dispatcher_test.go @@ -29,10 +29,7 @@ func newDispatcher() Dispatcher { panic(err) } - supervisor, err := New(pwd + "/testdata/services") - if err != nil { - panic(err) - } + supervisor, _ := New(pwd + "/testdata/services") return Dispatcher{supervisor, dispatcher.UnimplementedDispatcherServer{}} } @@ -142,6 +139,19 @@ func TestDispatcher_Start(t *testing.T) { } }) + t.Run("Service must not have a dodgy config", func(t *testing.T) { + _, err := d.Start(context.Background(), &dispatcher.Service{ + Name: "broken", + }) + if err == nil { + t.Fatalf("expected error") + } + + if err.Error() != "rpc error: code = FailedPrecondition desc = service config is incorrect" { + t.Errorf("error: %#v should be %q", err.Error(), "service config is incorrect") + } + }) + t.Run("Service must exist", func(t *testing.T) { _, err := d.Start(context.Background(), &dispatcher.Service{ Name: "foo", @@ -351,8 +361,17 @@ func TestDispatcher_ReadConfigs(t *testing.T) { currentStatus := d.s.services["app"].status _, err = d.ReadConfigs(context.Background(), new(emptypb.Empty)) - if err != nil { - t.Errorf("unexpected error: %#v", err) + if err == nil { + t.Fatal("expected error, received none") + } + + cpe, ok := err.(ConfigParseError) + if !ok { + t.Fatalf("unexpected error of type: %T", err) + } + + if _, ok = cpe.errors["01-broken"]; !ok { + t.Fatalf("expected an error for service %q in %#v", "01-broken", cpe) } if !reflect.DeepEqual(currentStatus, d.s.services["app"].status) { diff --git a/service.go b/service.go index e9d8c7c..5900261 100644 --- a/service.go +++ b/service.go @@ -42,16 +42,13 @@ type Service struct { // prior to restarting desiredRunning bool - // isDirty is set to true if a call to Supervisor.LoadConfigs + // loadError is set if a call to Supervisor.LoadConfigs // fails and so new config hasn't been picked up. // // we can assume that this means there's a new config that doesn't // look right, since the existence of this service means it must // have been right first time. - // - // but, in any case, isDirty is a flag that means 'Something went - // wrong parsing this service config, go check the logs' - isDirty bool + loadError string } func LoadService(dir string) (s *Service, err error) { diff --git a/supervisor.go b/supervisor.go index dedb108..9261bad 100644 --- a/supervisor.go +++ b/supervisor.go @@ -75,15 +75,19 @@ func (s *Supervisor) LoadConfigs() (err error) { svc, err = LoadService(filepath.Join(s.dir, entry.Name())) if err != nil { - // log error, recover - svc.isDirty = true cpe.Append(entry.Name(), err) - - continue + svc.loadError = err.Error() } name := serviceName(entry.Name()) - groupName := s.Config.ReconcileOverride(name, svc.Config.Grouping.GroupName) + + var groupName string + if svc.loadError == "" { + groupName = s.Config.ReconcileOverride(name, svc.Config.Grouping.GroupName) + } else { + groupName = "" + } + _, ok := groupsServices[groupName] if !ok { groupsServices[groupName] = make([]string, 0) @@ -101,15 +105,13 @@ func (s *Supervisor) LoadConfigs() (err error) { services[name] = svc } - if len(cpe.errors) > 0 { - return cpe - } - - // Only assign new services and groups when everything loads, - // rather than accidentally returning broken state s.groupsServices = groupsServices s.services = services + if len(cpe.errors) > 0 { + err = cpe + } + return } @@ -119,6 +121,10 @@ func (s *Supervisor) Start(name string, wait bool) error { return errServiceNotExist } + if svc.loadError != "" { + return errServiceDodgyConf + } + return svc.Start(wait) } @@ -153,6 +159,12 @@ func (s *Supervisor) StartAll() { var err error for _, group := range s.Config.Groups { + // Ignore anything with an empty group; this signifies + // a config error + if group == "" { + continue + } + services, ok := s.groupsServices[group] if !ok { sugar.Errorw("group either has no services or does not exist", diff --git a/supervisor_test.go b/supervisor_test.go index 2159b70..4fb7a15 100644 --- a/supervisor_test.go +++ b/supervisor_test.go @@ -6,14 +6,12 @@ import ( ) func TestNew(t *testing.T) { - s, err := New("testdata/services") - if err != nil { - t.Errorf("unexpected error %#v", err) - } + s, _ := New("testdata/services") t.Run("groups and service mappings are correct", func(t *testing.T) { got := s.groupsServices expect := map[string][]string{ + "": []string{"broken"}, "system": []string{"app", "app-cronjob", "app-oneoff"}, } diff --git a/testdata/services/01-broken/.config.toml b/testdata/services/01-broken/.config.toml new file mode 100644 index 0000000..1989e2a --- /dev/null +++ b/testdata/services/01-broken/.config.toml @@ -0,0 +1,15 @@ +# 'App' service config file +# + +type = "service" +reload_signal = "nuffin" + +[user] +user = "jspc" +group = "jspc" + +[grouping] +name = "system" + +[command] +args = "-a -b -c 100" diff --git a/testdata/services/01-broken/bin b/testdata/services/01-broken/bin new file mode 100755 index 0000000..33e5bc0 --- /dev/null +++ b/testdata/services/01-broken/bin @@ -0,0 +1,6 @@ +#!/usr/bin/env bash + +while true; do + date + sleep 1s +done diff --git a/testdata/services/01-broken/environment b/testdata/services/01-broken/environment new file mode 100644 index 0000000..96672ac --- /dev/null +++ b/testdata/services/01-broken/environment @@ -0,0 +1 @@ +PATH=/bin:/sbin diff --git a/testdata/services/01-broken/environment_overrides b/testdata/services/01-broken/environment_overrides new file mode 100644 index 0000000..6405eca --- /dev/null +++ b/testdata/services/01-broken/environment_overrides @@ -0,0 +1 @@ +HELLO=world diff --git a/testdata/services/01-broken/wd/.gitkeep b/testdata/services/01-broken/wd/.gitkeep new file mode 100644 index 0000000..e69de29