diff --git a/internal/config/config.go b/internal/config/config.go index c3bcf7e..c753e54 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -34,12 +34,8 @@ type AdminBypass struct { AllowedUsers []string `toml:"allowed_users"` } -func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) { - if !strings.HasSuffix(path, "/") { - path += "/" - } - - defaultConfig := &Config{ +func newDefaultConfig() *Config { + return &Config{ MaxReviews: nil, MinReviews: nil, UnskippableReviewers: []string{}, @@ -51,8 +47,16 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) SelfApprovalViaTeams: false, DisableSmartDismissal: false, RequireBothBranchReviewers: false, + SuppressUnownedWarning: false, + AllowSelfApproval: false, DisableReviewStatusComments: false, } +} + +func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) { + if !strings.HasSuffix(path, "/") { + path += "/" + } // Use filesystem reader if none provided if fileReader == nil { @@ -62,22 +66,15 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) fileName := path + "codeowners.toml" if !fileReader.PathExists(fileName) { - return defaultConfig, nil + return newDefaultConfig(), nil } file, err := fileReader.ReadFile(fileName) if err != nil { - return defaultConfig, err - } - config := defaultConfig - err = toml.Unmarshal(file, &config) - if err != nil { - return defaultConfig, err - } - if config.Enforcement == nil { - config.Enforcement = defaultConfig.Enforcement + return newDefaultConfig(), err } - if config.AdminBypass == nil { - config.AdminBypass = defaultConfig.AdminBypass + config := newDefaultConfig() + if err := toml.Unmarshal(file, config); err != nil { + return newDefaultConfig(), err } return config, nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f09c117..22385f8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,8 +1,11 @@ package owners import ( + "fmt" "os" "path/filepath" + "reflect" + "strings" "testing" ) @@ -250,6 +253,128 @@ func TestReadConfigFileError(t *testing.T) { } } +func TestReadConfigInvalidTomlReturnsDefaults(t *testing.T) { + testDir := t.TempDir() + content := ` +max_reviews = 9 +min_reviews = 9 +unskippable_reviewers = ["@someone"] +ignore = ["vendor"] +high_priority_labels = ["urgent"] +detailed_reviewers = true +disable_smart_dismissal = true +require_both_branch_reviewers = true +suppress_unowned_warning = true +allow_self_approval = true +self_approval_via_teams = true +disable_review_status_comments = true +[enforcement] +approval = true +fail_check = false +[admin_bypass] +enabled = true +allowed_users = ["someone"] +trailing = invalid +` + if err := os.WriteFile(filepath.Join(testDir, "codeowners.toml"), []byte(content), 0644); err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + config, err := ReadConfig(testDir, nil) + if err == nil { + t.Fatal("expected a parse error") + } + if config == nil { + t.Fatal("expected a config alongside the error") + } + + if !reflect.DeepEqual(config, newDefaultConfig()) { + t.Errorf("expected pristine defaults after a failed parse, got %s", configDiff(config, newDefaultConfig())) + } +} + +func TestReadConfigNeverReturnsNilOnError(t *testing.T) { + testDir := t.TempDir() + unreadable := filepath.Join(testDir, "unreadable") + if err := os.Mkdir(unreadable, 0o000); err != nil { + t.Fatalf("mkdir: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(unreadable, 0o755) }) + + malformed := t.TempDir() + if err := os.WriteFile(filepath.Join(malformed, "codeowners.toml"), []byte("trailing = invalid\n"), 0644); err != nil { + t.Fatalf("write: %v", err) + } + + for _, dir := range []string{unreadable, malformed} { + config, err := ReadConfig(dir, nil) + if err == nil { + continue + } + if config == nil { + t.Fatalf("%s: callers warn and then dereference the config, so an error path must still return one", dir) + } + if !reflect.DeepEqual(config, newDefaultConfig()) { + t.Errorf("%s: expected pristine defaults, got %s", dir, configDiff(config, newDefaultConfig())) + } + } +} + +func configDiff(got, want *Config) string { + problems := make([]string, 0, 8) + add := func(format string, args ...any) { + problems = append(problems, fmt.Sprintf(format, args...)) + } + if got.MaxReviews != nil { + add("MaxReviews=%d (want nil)", *got.MaxReviews) + } + if got.MinReviews != nil { + add("MinReviews=%d (want nil)", *got.MinReviews) + } + if !sliceEqual(got.UnskippableReviewers, want.UnskippableReviewers) { + add("UnskippableReviewers=%v", got.UnskippableReviewers) + } + if !sliceEqual(got.Ignore, want.Ignore) { + add("Ignore=%v", got.Ignore) + } + if !sliceEqual(got.HighPriorityLabels, want.HighPriorityLabels) { + add("HighPriorityLabels=%v", got.HighPriorityLabels) + } + if got.Enforcement == nil { + add("Enforcement=nil") + } else if *got.Enforcement != *want.Enforcement { + add("Enforcement=%+v (want %+v)", *got.Enforcement, *want.Enforcement) + } + if got.AdminBypass == nil { + add("AdminBypass=nil") + } else { + if got.AdminBypass.Enabled != want.AdminBypass.Enabled { + add("AdminBypass.Enabled=%v", got.AdminBypass.Enabled) + } + if !sliceEqual(got.AdminBypass.AllowedUsers, want.AdminBypass.AllowedUsers) { + add("AdminBypass.AllowedUsers=%v", got.AdminBypass.AllowedUsers) + } + } + for _, f := range []struct { + name string + got bool + want bool + }{ + {"DetailedReviewers", got.DetailedReviewers, want.DetailedReviewers}, + {"DisableSmartDismissal", got.DisableSmartDismissal, want.DisableSmartDismissal}, + {"RequireBothBranchReviewers", got.RequireBothBranchReviewers, want.RequireBothBranchReviewers}, + {"SuppressUnownedWarning", got.SuppressUnownedWarning, want.SuppressUnownedWarning}, + {"AllowSelfApproval", got.AllowSelfApproval, want.AllowSelfApproval}, + {"SelfApprovalViaTeams", got.SelfApprovalViaTeams, want.SelfApprovalViaTeams}, + {"DisableReviewStatusComments", got.DisableReviewStatusComments, want.DisableReviewStatusComments}, + } { + if f.got != f.want { + add("%s=%v (want %v)", f.name, f.got, f.want) + } + } + return strings.Join(problems, "; ") +} + // Helper functions func intPtr(i int) *int { return &i diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 59a2244..729e1ad 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -129,7 +129,7 @@ Binary files a/assets/img/offline.png and b/assets/img/offline.png differ`, expectedErr: false, expectedFiles: 2, expectedHunks: map[string]int{ - "file1.go": 1, + "file1.go": 1, "assets/img/offline.png": 0, }, },