diff --git a/internal/client/status/viewmodel/render.go b/internal/client/status/viewmodel/render.go index 971e420..c11ee95 100644 --- a/internal/client/status/viewmodel/render.go +++ b/internal/client/status/viewmodel/render.go @@ -11,7 +11,7 @@ import ( yaml "github.com/zrepl/yaml-config" "github.com/zrepl/zrepl/internal/client/status/viewmodel/stringbuilder" - "github.com/zrepl/zrepl/internal/daemon" + "github.com/zrepl/zrepl/internal/config" "github.com/zrepl/zrepl/internal/daemon/job" "github.com/zrepl/zrepl/internal/daemon/pruner" "github.com/zrepl/zrepl/internal/daemon/snapper" @@ -85,7 +85,7 @@ func (m *M) Update(p Params) { // filter out internal jobs var jobsList []*Job for _, j := range m.jobsList { - if daemon.IsInternalJobName(j.name) { + if config.IsInternalJobName(j.name) { continue } jobsList = append(jobsList, j) diff --git a/internal/config/config.go b/internal/config/config.go index 583380c..a70fcc3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,6 +6,7 @@ import ( "os" pathpkg "path" "path/filepath" + "strings" "time" "github.com/pkg/errors" @@ -693,9 +694,32 @@ func ParseConfig(path string) (rootConfig *Config, err error) { return nil, err } + if err = validateJobNames(rootConfig); err != nil { + return nil, err + } + return rootConfig, err } +func IsInternalJobName(s string) bool { + return strings.HasPrefix(s, "_") +} + +func validateJobNames(config *Config) error { + seen := make(map[string]struct{}) + for _, job := range config.Jobs { + name := job.Name() + if IsInternalJobName(name) { + return errors.Errorf("job name %q is reserved for internal use (starts with _)", name) + } + if _, ok := seen[name]; ok { + return errors.Errorf("duplicate job name %q", name) + } + seen[name] = struct{}{} + } + return nil +} + func expandConfigInclude(configPath string, config *Config) (err error) { var includeConfigPaths []string for _, path := range config.Include { diff --git a/internal/config/config_include_test.go b/internal/config/config_include_test.go deleted file mode 100644 index 3a95648..0000000 --- a/internal/config/config_include_test.go +++ /dev/null @@ -1,44 +0,0 @@ -package config - -import ( - "testing" - - "github.com/kr/pretty" - "github.com/stretchr/testify/require" -) - -func TestIncludeSingle(t *testing.T) { - path := "./samples/include.yml" - - t.Run(path, func(t *testing.T) { - config, err := ParseConfig(path) - if err != nil { - t.Errorf("error parsing %s:\n%+v", path, err) - } - - require.NotNil(t, config) - require.NotNil(t, config.Global) - require.NotEmpty(t, config.Jobs) - - t.Logf("file: %s", path) - t.Log(pretty.Sprint(config)) - }) -} - -func TestIncludeDirectory(t *testing.T) { - path := "./samples/include_directory.yml" - - t.Run(path, func(t *testing.T) { - config, err := ParseConfig(path) - if err != nil { - t.Errorf("error parsing %s:\n%+v", path, err) - } - - require.NotNil(t, config) - require.NotNil(t, config.Global) - require.NotEmpty(t, config.Jobs) - - t.Logf("file: %s", path) - t.Log(pretty.Sprint(config)) - }) -} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 4a59399..a6c385e 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -45,6 +45,20 @@ func TestSampleConfigsAreParsedWithoutErrors(t *testing.T) { } +func TestInvalidSampleConfigsFailToParse(t *testing.T) { + paths, err := filepath.Glob("./samples/invalid/*/zrepl.yml") + require.NoError(t, err, "glob failed") + require.NotEmpty(t, paths, "no invalid sample configs found") + + for _, p := range paths { + t.Run(p, func(t *testing.T) { + _, err := ParseConfig(p) + require.Error(t, err, "expected config %s to fail parsing", p) + t.Logf("config %s failed as expected: %v", p, err) + }) + } +} + // template must be a template/text template with a single '{{ . }}' as placeholder for val // //nolint:deadcode,unused diff --git a/internal/config/samples/invalid/duplicate_job_names/zrepl.yml b/internal/config/samples/invalid/duplicate_job_names/zrepl.yml new file mode 100644 index 0000000..3dfc2d7 --- /dev/null +++ b/internal/config/samples/invalid/duplicate_job_names/zrepl.yml @@ -0,0 +1,24 @@ +jobs: + - type: snap + name: "my_job" + filesystems: { + "<": true, + } + snapshotting: + type: manual + pruning: + keep: + - type: last_n + count: 10 + + - type: snap + name: "my_job" + filesystems: { + "<": true, + } + snapshotting: + type: manual + pruning: + keep: + - type: last_n + count: 5 diff --git a/internal/config/samples/invalid/duplicate_job_names_with_include/included.yml b/internal/config/samples/invalid/duplicate_job_names_with_include/included.yml new file mode 100644 index 0000000..198bd0a --- /dev/null +++ b/internal/config/samples/invalid/duplicate_job_names_with_include/included.yml @@ -0,0 +1,12 @@ +jobs: + - type: snap + name: "my_job" + filesystems: { + "<": true, + } + snapshotting: + type: manual + pruning: + keep: + - type: last_n + count: 5 diff --git a/internal/config/samples/invalid/duplicate_job_names_with_include/zrepl.yml b/internal/config/samples/invalid/duplicate_job_names_with_include/zrepl.yml new file mode 100644 index 0000000..7a0e674 --- /dev/null +++ b/internal/config/samples/invalid/duplicate_job_names_with_include/zrepl.yml @@ -0,0 +1,15 @@ +jobs: + - type: snap + name: "my_job" + filesystems: { + "<": true, + } + snapshotting: + type: manual + pruning: + keep: + - type: last_n + count: 10 + +include: + - ./included.yml diff --git a/internal/config/samples/invalid/internal_job_name/zrepl.yml b/internal/config/samples/invalid/internal_job_name/zrepl.yml new file mode 100644 index 0000000..dec4ac5 --- /dev/null +++ b/internal/config/samples/invalid/internal_job_name/zrepl.yml @@ -0,0 +1,12 @@ +jobs: + - type: snap + name: "_internal_job" + filesystems: { + "<": true, + } + snapshotting: + type: manual + pruning: + keep: + - type: last_n + count: 10 diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index 95d1f54..c353285 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "os/signal" - "strings" "sync" "syscall" "time" @@ -64,7 +63,7 @@ func Run(ctx context.Context, conf *config.Config) error { }) for _, job := range confJobs { - if IsInternalJobName(job.Name()) { + if config.IsInternalJobName(job.Name()) { panic(fmt.Sprintf("internal job name used for config job '%s'", job.Name())) //FIXME } } @@ -211,10 +210,6 @@ const ( jobNameControl = "_control" ) -func IsInternalJobName(s string) bool { - return strings.HasPrefix(s, "_") -} - func (s *jobs) start(ctx context.Context, j job.Job, internal bool) { s.m.Lock() defer s.m.Unlock() @@ -222,10 +217,12 @@ func (s *jobs) start(ctx context.Context, j job.Job, internal bool) { ctx = logging.WithInjectedField(ctx, logging.JobField, j.Name()) jobName := j.Name() - if !internal && IsInternalJobName(jobName) { + + // package `config` enforces these with clean errors, these are just assertions + if !internal && config.IsInternalJobName(jobName) { panic(fmt.Sprintf("internal job name used for non-internal job %s", jobName)) } - if internal && !IsInternalJobName(jobName) { + if internal && !config.IsInternalJobName(jobName) { panic(fmt.Sprintf("internal job does not use internal job name %s", jobName)) } if _, ok := s.jobs[jobName]; ok {