diff --git a/cmd/addgroupguests.go b/cmd/addgroupguests.go index 5a5de23..37bf209 100644 --- a/cmd/addgroupguests.go +++ b/cmd/addgroupguests.go @@ -35,7 +35,10 @@ Example: return } - courseConfig := config.GetCourseConfig(courseName) + courseConfig, err := config.GetCourseConfig(courseName) + if err != nil { + er(err) + } subgroupPath := config.GetCourseSubgroupPath(courseName) fmt.Printf("Course: %s\n", courseName) @@ -64,7 +67,7 @@ Example: fmt.Scanln() //nolint:errcheck c := gitlab.NewClient() - err := c.AddGroupGuests(courseName) + err = c.AddGroupGuests(courseName) if err != nil { fmt.Printf("Error: %v\n", err) return diff --git a/cmd/archive.go b/cmd/archive.go index a19126b..a27c4a3 100644 --- a/cmd/archive.go +++ b/cmd/archive.go @@ -21,7 +21,10 @@ var ( Long: `Archive or unarchive repositories.`, Args: cobra.MinimumNArgs(2), //nolint:gomnd Run: func(cmd *cobra.Command, args []string) { - assignmentConfig := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + assignmentConfig, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } assignmentConfig.Show() fmt.Println(aurora.Magenta("Config okay? Press 'Enter' to continue or 'Ctrl-C' to stop ...")) fmt.Scanln() //nolint:errcheck diff --git a/cmd/check.go b/cmd/check.go index 3344316..a6c9249 100644 --- a/cmd/check.go +++ b/cmd/check.go @@ -20,7 +20,10 @@ var checkCmd = &cobra.Command{ viper.Set("show-success", true) c := gitlab.NewClient() if len(args) == 1 { - cfg := config.GetCourseConfig(args[0]) + cfg, err := config.GetCourseConfig(args[0]) + if err != nil { + er(err) + } if cfg != nil { c.CheckCourse(cfg) } diff --git a/cmd/clone.go b/cmd/clone.go index 7693962..7f871f0 100644 --- a/cmd/clone.go +++ b/cmd/clone.go @@ -17,7 +17,10 @@ var ( You can specify students or groups in order to clone only for these.`, Args: cobra.MinimumNArgs(2), //nolint:gomnd Run: func(cmd *cobra.Command, args []string) { - assignmentConfig := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + assignmentConfig, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } if len(Branch) > 0 { assignmentConfig.SetBranch(Branch) } diff --git a/cmd/delete.go b/cmd/delete.go index 061a8b5..d647dbb 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -20,7 +20,10 @@ var deleteCmd = &cobra.Command{ You can specify students or groups in order to delete only for these.`, Args: cobra.MinimumNArgs(2), //nolint:gomnd Run: func(cmd *cobra.Command, args []string) { - assignmentConfig := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + assignmentConfig, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } assignmentConfig.Show() fmt.Println(aurora.Magenta("Do you really want to delete the repos? Press 'Enter' to continue or 'Ctrl-C' to stop ...")) fmt.Scanln() //nolint:errcheck diff --git a/cmd/generate.go b/cmd/generate.go index 38a918c..6b0712d 100644 --- a/cmd/generate.go +++ b/cmd/generate.go @@ -21,7 +21,10 @@ You can specify students or groups in order to generate only for these. A student needs to exist on GitLab, a group needs to exist in the configuration file.`, Args: cobra.MinimumNArgs(2), //nolint:gomnd Run: func(cmd *cobra.Command, args []string) { - assignmentConfig := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + assignmentConfig, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } assignmentConfig.Show() fmt.Println(aurora.Magenta("Config okay? Press 'Enter' to continue or 'Ctrl-C' to stop ...")) fmt.Scanln() //nolint:errcheck diff --git a/cmd/protect.go b/cmd/protect.go index e92d728..9a9d94a 100644 --- a/cmd/protect.go +++ b/cmd/protect.go @@ -21,7 +21,10 @@ var ( Long: `Protect branch for exisiting repositories.`, Args: cobra.MinimumNArgs(2), //nolint:gomnd Run: func(cmd *cobra.Command, args []string) { - assignmentConfig := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + assignmentConfig, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } if len(ProtectBranch) > 0 { assignmentConfig.SetProtectToBranch(ProtectBranch) } diff --git a/cmd/push.go b/cmd/push.go index ec7400b..6ca75a1 100644 --- a/cmd/push.go +++ b/cmd/push.go @@ -20,12 +20,15 @@ var pushCmd = &cobra.Command{ course := args[0] assignment := args[1] branchname := args[2] - assignmentConfig := config.GetAssignmentConfig(course, assignment, args[3:]...) + assignmentConfig, err := config.GetAssignmentConfig(course, assignment, args[3:]...) + if err != nil { + er(err) + } assignmentConfig.Show() fmt.Println(aurora.Magenta("Config okay? Press 'Enter' to continue or 'Ctrl-C' to stop ...")) fmt.Scanln() //nolint:errcheck c := gitlab.NewClient() - err := c.Push(assignmentConfig, branchname) + err = c.Push(assignmentConfig, branchname) if err != nil { fmt.Printf("error: %s", err.Error()) } diff --git a/cmd/report.go b/cmd/report.go index 6f5b150..90e024d 100644 --- a/cmd/report.go +++ b/cmd/report.go @@ -63,7 +63,10 @@ var ( } return } - assignmentConfig := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + assignmentConfig, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } c := gitlab.NewClient() if Json { c.ReportJSON(assignmentConfig, output) diff --git a/cmd/setaccess.go b/cmd/setaccess.go index 7c1cad8..9eb4990 100644 --- a/cmd/setaccess.go +++ b/cmd/setaccess.go @@ -21,7 +21,10 @@ var ( Long: `Set access level for exisiting repositories.`, Args: cobra.MinimumNArgs(2), //nolint:gomnd Run: func(cmd *cobra.Command, args []string) { - assignmentConfig := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + assignmentConfig, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } if len(Level) > 0 { assignmentConfig.SetAccessLevel(Level) } diff --git a/cmd/show.go b/cmd/show.go index b846ae7..855acd7 100644 --- a/cmd/show.go +++ b/cmd/show.go @@ -15,7 +15,10 @@ var showConfigCmd = &cobra.Command{ Long: `Show config of an assignment`, Args: cobra.MinimumNArgs(2), Run: func(cmd *cobra.Command, args []string) { - cfg := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + cfg, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } cfg.Show() }, } diff --git a/cmd/update.go b/cmd/update.go index a3e159d..ca6458d 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -22,7 +22,10 @@ var updateCmd = &cobra.Command{ Use only with fresh, i.e. untouched, repositories.`, Args: cobra.MinimumNArgs(2), //nolint:gomnd Run: func(cmd *cobra.Command, args []string) { - assignmentConfig := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + assignmentConfig, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } assignmentConfig.Show() fmt.Println(aurora.Red("Heads up! Use only with untouched projects.")) fmt.Println(aurora.Magenta("Config okay? Press 'Enter' to continue or 'Ctrl-C' to stop ...")) diff --git a/cmd/urls.go b/cmd/urls.go index ca533ca..3357269 100644 --- a/cmd/urls.go +++ b/cmd/urls.go @@ -16,7 +16,10 @@ var ( if len(args) == 1 { config.GetCourseURL(args[0]) } else { - assignmentConfig := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + assignmentConfig, err := config.GetAssignmentConfig(args[0], args[1], args[2:]...) + if err != nil { + er(err) + } if startercode { assignmentConfig.StartercodeURL() } else { diff --git a/config/assignment.go b/config/assignment.go index 4ae3f47..55026aa 100644 --- a/config/assignment.go +++ b/config/assignment.go @@ -5,7 +5,6 @@ import ( "strings" "github.com/go-viper/mapstructure/v2" - "github.com/rs/zerolog/log" "github.com/spf13/viper" ) @@ -13,33 +12,28 @@ func GetCourseURL(course string) { fmt.Printf("%s/%s\n", viper.GetString("gitlab.host"), coursePath(course)) } -func GetAssignmentConfig(course, assignment string, onlyForStudentsOrGroups ...string) *AssignmentConfig { +func GetAssignmentConfig(course, assignment string, onlyForStudentsOrGroups ...string) (*AssignmentConfig, error) { if !viper.IsSet(course) { - log.Fatal(). - Str("course", course). - Msg("configuration for course not found") + return nil, fmt.Errorf("configuration for course %s not found", course) } if !viper.IsSet(course + "." + assignment) { - log.Fatal(). - Str("course", course). - Str("assignment", assignment). - Msg("configuration for assignment not found") + return nil, fmt.Errorf("course %s: configuration for assignment %s not found", course, assignment) } // Abstract assignments are bases for `extends` only and must not be used // directly. Checked before resolving inheritance so the (own) flag is read, // not an inherited one. if assignmentIsAbstract(course, assignment) { - log.Fatal(). - Str("course", course). - Str("assignment", assignment). - Msg("assignment is abstract (a base for 'extends') and cannot be used directly") + return nil, fmt.Errorf("course %s: assignment %s is abstract (a base for 'extends') and cannot be used directly", + course, assignment) } // Resolve `extends` inheritance before reading any fields so the rest of // the config loading sees the merged, effective configuration. - resolveAssignmentInheritance(course, assignment) + if err := resolveAssignmentInheritance(course, assignment); err != nil { + return nil, err + } assignmentKey := course + "." + assignment per := per(assignmentKey) @@ -53,8 +47,14 @@ func GetAssignmentConfig(course, assignment string, onlyForStudentsOrGroups ...s containerRegistry = true } - starter := startercode(assignmentKey) - branchRules := branches(assignmentKey, starter) + starter, err := startercode(assignmentKey) + if err != nil { + return nil, err + } + branchRules, err := branches(assignmentKey, starter) + if err != nil { + return nil, err + } defaultCloneBranch := defaultBranch(branchRules, "main") deferredBranches := make(map[string]*DeferredBranch) @@ -93,6 +93,15 @@ func GetAssignmentConfig(course, assignment string, onlyForStudentsOrGroups ...s } } + mr, err := mergeRequest(assignmentKey) + if err != nil { + return nil, err + } + seed, err := seeder(assignmentKey) + if err != nil { + return nil, err + } + assignmentConfig := &AssignmentConfig{ Course: course, Name: assignment, @@ -109,7 +118,7 @@ func GetAssignmentConfig(course, assignment string, onlyForStudentsOrGroups ...s Description: description(assignmentKey), ContainerRegistry: containerRegistry, AccessLevel: accessLevel(assignmentKey), - MergeRequest: mergeRequest(assignmentKey), + MergeRequest: mr, Branches: branchRules, Issues: issues(assignmentKey), Students: students(per, course, assignment, onlyForStudentsOrGroups...), @@ -117,11 +126,11 @@ func GetAssignmentConfig(course, assignment string, onlyForStudentsOrGroups ...s Startercode: starter, Clone: clone(assignmentKey, defaultCloneBranch), Release: release, - Seeder: seeder(assignmentKey), + Seeder: seed, DeferredBranches: deferredBranches, } - return assignmentConfig + return assignmentConfig, nil } // Using email addresses instead of usernames/user-id's results in @ in the student's name. @@ -215,7 +224,7 @@ func description(assignmentKey string) string { return description } -func mergeRequest(assignmentKey string) *MergeRequest { +func mergeRequest(assignmentKey string) (*MergeRequest, error) { mergeMethod := MergeCommit switch viper.GetString(assignmentKey + ".mergeRequest.mergeMethod") { case "semi_linear": @@ -238,6 +247,15 @@ func mergeRequest(assignmentKey string) *MergeRequest { squashOption = SquashDefaultOff } + approvals, err := mergeRequestApprovals(assignmentKey) + if err != nil { + return nil, err + } + settings, err := mergeRequestApprovalSettings(assignmentKey) + if err != nil { + return nil, err + } + return &MergeRequest{ MergeMethod: mergeMethod, SquashOption: squashOption, @@ -245,21 +263,21 @@ func mergeRequest(assignmentKey string) *MergeRequest { SkippedPipelinesAreSuccessful: viper.GetBool(assignmentKey + ".mergeRequest.skippedPipelinesAreSuccessful"), AllThreadsMustBeResolved: viper.GetBool(assignmentKey + ".mergeRequest.allThreadsMustBeResolved"), StatusChecksMustSucceed: viper.GetBool(assignmentKey + ".mergeRequest.statusChecksMustSucceed"), - Approvals: mergeRequestApprovals(assignmentKey), - ApprovalSettings: mergeRequestApprovalSettings(assignmentKey), - } + Approvals: approvals, + ApprovalSettings: settings, + }, nil } -func mergeRequestApprovals(assignmentKey string) []MergeRequestApprovalRule { +func mergeRequestApprovals(assignmentKey string) ([]MergeRequestApprovalRule, error) { raw := viper.Get(assignmentKey + ".mergeRequest.approvals") raw = extractMergeRequestApprovalRulesRaw(raw) raw = normalizeMergeRequestApprovalConfigKeys(raw) if raw == nil { - return nil + return nil, nil } if containsLegacyApprovalUsersKey(raw) { - log.Fatal().Str("assignmentKey", assignmentKey).Msg("mergeRequest.approvals.rules[].users is no longer supported; use usernames") + return nil, fmt.Errorf("%s: mergeRequest.approvals.rules[].users is no longer supported; use usernames", assignmentKey) } var configured []MergeRequestApprovalRule @@ -269,10 +287,10 @@ func mergeRequestApprovals(assignmentKey string) []MergeRequestApprovalRule { WeaklyTypedInput: true, }) if err != nil { - log.Fatal().Err(err).Str("assignmentKey", assignmentKey).Msg("cannot create mergeRequest.approvals decoder") + return nil, fmt.Errorf("%s: cannot create mergeRequest.approvals decoder: %w", assignmentKey, err) } if err := decoder.Decode(raw); err != nil { - log.Fatal().Err(err).Str("assignmentKey", assignmentKey).Msg("cannot parse mergeRequest.approvals config") + return nil, fmt.Errorf("%s: cannot parse mergeRequest.approvals config: %w", assignmentKey, err) } normalized := make([]MergeRequestApprovalRule, 0, len(configured)) @@ -327,7 +345,7 @@ func mergeRequestApprovals(assignmentKey string) []MergeRequestApprovalRule { normalized = append(normalized, rule) } - return normalized + return normalized, nil } func extractMergeRequestApprovalRulesRaw(value any) any { @@ -374,10 +392,10 @@ func containsLegacyApprovalUsersKey(value any) bool { } } -func mergeRequestApprovalSettings(assignmentKey string) *MergeRequestApprovalSettings { +func mergeRequestApprovalSettings(assignmentKey string) (*MergeRequestApprovalSettings, error) { prefix := assignmentKey + ".mergeRequest.approvals.settings" if !viper.IsSet(prefix) { - return nil + return nil, nil } settings := &MergeRequestApprovalSettings{} @@ -404,10 +422,9 @@ func mergeRequestApprovalSettings(assignmentKey string) *MergeRequestApprovalSet case ApprovalKeepApprovals, ApprovalRemoveAllApprovals, ApprovalRemoveCodeOwnerApprovalsIfFilesChanged: settings.WhenCommitAdded = &v default: - log.Fatal(). - Str("assignmentKey", assignmentKey). - Str("whenCommitAdded", string(v)). - Msg("invalid mergeRequest.approvals.settings.whenCommitAdded") + return nil, fmt.Errorf("%s: invalid mergeRequest.approvals.settings.whenCommitAdded %q, must be one of %s, %s, %s", + assignmentKey, v, + ApprovalKeepApprovals, ApprovalRemoveAllApprovals, ApprovalRemoveCodeOwnerApprovalsIfFilesChanged) } } @@ -416,10 +433,10 @@ func mergeRequestApprovalSettings(assignmentKey string) *MergeRequestApprovalSet settings.PreventEditingApprovalRulesInMergeRequests == nil && settings.RequireUserReauthenticationToApprove == nil && settings.WhenCommitAdded == nil { - return nil + return nil, nil } - return settings + return settings, nil } func normalizeMergeRequestApprovalConfigKeys(value any) any { diff --git a/config/assignment_test.go b/config/assignment_test.go index ae55a37..586387c 100644 --- a/config/assignment_test.go +++ b/config/assignment_test.go @@ -114,7 +114,7 @@ func TestGetAssignmentConfig(t *testing.T) { viper.Set("course.a1.release", map[string]any{"dockerImages": []string{"registry/app"}}) viper.Set("course.a1.release.dockerImages", []string{"registry/app"}) - cfg := GetAssignmentConfig("course", "a1") + cfg := mustAssignmentConfig(t, "course", "a1") if cfg.Path != "mpd/ss26/blatt-01" { t.Fatalf("Path = %q", cfg.Path) @@ -154,7 +154,7 @@ func TestGetAssignmentConfig_DefaultMergeRequest(t *testing.T) { viper.Set("course.a1", true) viper.Set("course.a1.assignmentpath", "blatt-01") - cfg := GetAssignmentConfig("course", "a1") + cfg := mustAssignmentConfig(t, "course", "a1") if cfg.MergeRequest == nil { t.Fatal("MergeRequest should be initialized with defaults") @@ -199,7 +199,7 @@ func TestGetAssignmentConfig_SquashOption(t *testing.T) { viper.Set("course.a1.assignmentpath", "blatt-01") viper.Set("course.a1.mergeRequest.squashOption", tc.value) - cfg := GetAssignmentConfig("course", "a1") + cfg := mustAssignmentConfig(t, "course", "a1") if cfg.MergeRequest == nil { t.Fatal("MergeRequest should not be nil") @@ -224,7 +224,7 @@ func TestGetAssignmentConfig_MergeRequestChecks(t *testing.T) { viper.Set("course.a1.mergeRequest.allThreadsMustBeResolved", true) viper.Set("course.a1.mergeRequest.statusChecksMustSucceed", true) - cfg := GetAssignmentConfig("course", "a1") + cfg := mustAssignmentConfig(t, "course", "a1") if cfg.MergeRequest == nil { t.Fatal("MergeRequest should not be nil") @@ -277,7 +277,7 @@ func TestGetAssignmentConfig_MergeRequestApprovals(t *testing.T) { }, }) - cfg := GetAssignmentConfig("course", "a1") + cfg := mustAssignmentConfig(t, "course", "a1") if cfg.MergeRequest == nil { t.Fatal("MergeRequest should not be nil") } diff --git a/config/course.go b/config/course.go index d8dcf51..482fecd 100644 --- a/config/course.go +++ b/config/course.go @@ -3,7 +3,6 @@ package config import ( "fmt" - "github.com/rs/zerolog/log" "github.com/spf13/viper" ) @@ -13,19 +12,16 @@ type CourseConfig struct { Groups []*Group } -func GetCourseConfig(course string) *CourseConfig { +func GetCourseConfig(course string) (*CourseConfig, error) { if !viper.IsSet(course) { - log.Fatal(). - Str("course", course). - Msg("configuration for course not found") - return nil + return nil, fmt.Errorf("configuration for course %s not found", course) } return &CourseConfig{ Course: course, Students: students(PerStudent, course, ""), Groups: groups(PerGroup, course, ""), - } + }, nil } // CourseExists checks if a course configuration exists diff --git a/config/course_test.go b/config/course_test.go index 3da6d73..ab95085 100644 --- a/config/course_test.go +++ b/config/course_test.go @@ -10,7 +10,7 @@ func TestGetCourseConfig_HappyPath_Students(t *testing.T) { resetViper(t) viper.Set("mpd.students", []string{"alice", "bob"}) - cc := GetCourseConfig("mpd") + cc := mustCourseConfig(t, "mpd") if cc == nil { t.Fatal("GetCourseConfig() returned nil") } @@ -27,7 +27,7 @@ func TestGetCourseConfig_HappyPath_Groups(t *testing.T) { viper.Set("mpd.groups.team1.members", []string{"alice"}) viper.Set("mpd.groups.team2.members", []string{"bob", "carol"}) - cc := GetCourseConfig("mpd") + cc := mustCourseConfig(t, "mpd") if cc == nil { t.Fatal("GetCourseConfig() returned nil") } @@ -41,7 +41,7 @@ func TestGetCourseConfig_NoStudentsNoGroups(t *testing.T) { // Just set the top-level key so IsSet("mpd") returns true viper.Set("mpd.semesterpath", "ss26") - cc := GetCourseConfig("mpd") + cc := mustCourseConfig(t, "mpd") if cc == nil { t.Fatal("GetCourseConfig() returned nil") } diff --git a/config/errors_test.go b/config/errors_test.go new file mode 100644 index 0000000..c2798fc --- /dev/null +++ b/config/errors_test.go @@ -0,0 +1,160 @@ +package config + +import ( + "strings" + "testing" + + "github.com/spf13/viper" +) + +// These tests were impossible before config loading returned errors: every case +// below used to call log.Fatal (= os.Exit(1)) and would have taken the test +// binary down with it. + +func loadErrorFixture(t *testing.T) { + t.Helper() + + resetViper(t) + viper.SetConfigFile("testdata/errors/edge.yaml") + if err := viper.ReadInConfig(); err != nil { + t.Fatalf("reading error fixture: %v", err) + } + viper.Set("gitlab.host", "https://gitlab.lrz.de") +} + +func TestGetAssignmentConfigErrors(t *testing.T) { + tests := []struct { + name string + course string + assignment string + wantErr string + }{ + { + name: "unknown course", + course: "nosuchcourse", + assignment: "fine", + wantErr: "configuration for course nosuchcourse not found", + }, + { + name: "unknown assignment", + course: "edge", + assignment: "nosuchassignment", + wantErr: "configuration for assignment nosuchassignment not found", + }, + { + name: "abstract assignment used directly", + course: "edge", + assignment: "base", + wantErr: "is abstract", + }, + { + name: "cyclic extends across two assignments", + course: "edge", + assignment: "cyclea", + wantErr: "cyclic 'extends' inheritance detected", + }, + { + name: "assignment extending itself", + course: "edge", + assignment: "selfcycle", + wantErr: "cyclic 'extends' inheritance detected", + }, + { + name: "extends is not a string", + course: "edge", + assignment: "extendsnotstring", + wantErr: "'extends' must be the name of another assignment", + }, + { + name: "extends is blank", + course: "edge", + assignment: "extendsempty", + wantErr: "'extends' must be the name of another assignment", + }, + { + name: "extends names a missing parent", + course: "edge", + assignment: "extendsmissing", + wantErr: `assignment "doesnotexist" referenced by 'extends' not found`, + }, + { + name: "startercode without url", + course: "edge", + assignment: "starterwithouturl", + wantErr: "startercode provided without url", + }, + { + name: "seeder without cmd", + course: "edge", + assignment: "seederwithoutcmd", + wantErr: "seeder provided without cmd", + }, + { + name: "seeder signKey is not an armored key ring", + course: "edge", + assignment: "seederbadsignkey", + wantErr: "cannot read seeder.signKey as an armored PGP key ring", + }, + { + name: "legacy approvals users key", + course: "edge", + assignment: "approvalsusers", + wantErr: "no longer supported; use usernames", + }, + { + name: "invalid whenCommitAdded", + course: "edge", + assignment: "badwhencommitadded", + wantErr: "invalid mergeRequest.approvals.settings.whenCommitAdded", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + loadErrorFixture(t) + + cfg, err := GetAssignmentConfig(tt.course, tt.assignment) + if err == nil { + t.Fatalf("GetAssignmentConfig(%q, %q) = %+v, want error containing %q", + tt.course, tt.assignment, cfg, tt.wantErr) + } + if cfg != nil { + t.Errorf("GetAssignmentConfig(%q, %q) returned a config alongside the error: %+v", + tt.course, tt.assignment, cfg) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("GetAssignmentConfig(%q, %q) error = %q, want it to contain %q", + tt.course, tt.assignment, err, tt.wantErr) + } + }) + } +} + +// The error fixture must itself be loadable, so a failure above is about the +// assignment under test and not a broken file. +func TestErrorFixtureResolvesValidAssignment(t *testing.T) { + loadErrorFixture(t) + + cfg, err := GetAssignmentConfig("edge", "fine") + if err != nil { + t.Fatalf("GetAssignmentConfig(edge, fine): unexpected error: %v", err) + } + if cfg.Per != PerStudent { + t.Errorf("Per = %q, want %q (inherited from the abstract base)", cfg.Per, PerStudent) + } +} + +func TestGetCourseConfigUnknownCourse(t *testing.T) { + loadErrorFixture(t) + + cfg, err := GetCourseConfig("nosuchcourse") + if err == nil { + t.Fatalf("GetCourseConfig(nosuchcourse) = %+v, want error", cfg) + } + if cfg != nil { + t.Errorf("GetCourseConfig(nosuchcourse) returned a config alongside the error: %+v", cfg) + } + if want := "configuration for course nosuchcourse not found"; !strings.Contains(err.Error(), want) { + t.Errorf("error = %q, want it to contain %q", err, want) + } +} diff --git a/config/golden_test.go b/config/golden_test.go index c2658cc..9b4a332 100644 --- a/config/golden_test.go +++ b/config/golden_test.go @@ -226,10 +226,7 @@ func TestGolden(t *testing.T) { // Reload per assignment: GetAssignmentConfig mutates viper. loadCourseFixture(t, path) - cfg := GetAssignmentConfig(course, assignment) - if cfg == nil { - t.Fatalf("GetAssignmentConfig(%q, %q) returned nil", course, assignment) - } + cfg := mustAssignmentConfig(t, course, assignment) got, err := json.MarshalIndent(goldenView(cfg), "", " ") if err != nil { diff --git a/config/inheritance.go b/config/inheritance.go index 5f6fe1a..2f46bdf 100644 --- a/config/inheritance.go +++ b/config/inheritance.go @@ -4,7 +4,6 @@ import ( "fmt" "strings" - "github.com/rs/zerolog/log" "github.com/spf13/viper" ) @@ -40,28 +39,35 @@ func assignmentIsAbstract(course, assignment string) bool { // - scalars and slices (e.g. branches, issueNumbers) are replaced wholesale. // // Parents may themselves extend other assignments; chains are resolved -// recursively. Cycles and missing parents are fatal. -func resolveAssignmentInheritance(course, assignment string) { +// recursively. Cycles and missing parents are errors. +// +// The write-back mutates global viper state from what is nominally a read path, +// so resolving two assignments concurrently is not safe. This disappears once +// config loading moves to a typed source schema and resolution becomes a pure +// function over it; until then callers must resolve one assignment at a time. +func resolveAssignmentInheritance(course, assignment string) error { assignmentKey := course + "." + assignment if !viper.IsSet(assignmentKey + "." + inheritKey) { - return + return nil } - merged := mergedAssignmentMap(course, assignment, map[string]bool{}) + merged, err := mergedAssignmentMap(course, assignment, map[string]bool{}) + if err != nil { + return err + } // Meta keys must never leak into the effective config or be inherited. delete(merged, inheritKey) delete(merged, abstractKey) viper.Set(assignmentKey, merged) + return nil } // mergedAssignmentMap returns the assignment's configuration map with all parent // configuration (via `extends`) merged in. The child's own values win. -func mergedAssignmentMap(course, assignment string, seen map[string]bool) map[string]any { +func mergedAssignmentMap(course, assignment string, seen map[string]bool) (map[string]any, error) { if seen[assignment] { - log.Fatal(). - Str("course", course). - Str("assignment", assignment). - Msg("cyclic 'extends' inheritance detected in assignment configuration") + return nil, fmt.Errorf("course %s, assignment %s: cyclic 'extends' inheritance detected in assignment configuration", + course, assignment) } seen[assignment] = true @@ -69,29 +75,27 @@ func mergedAssignmentMap(course, assignment string, seen map[string]bool) map[st parentRaw, ok := own[inheritKey] if !ok { - return own + return own, nil } parent, ok := parentRaw.(string) if !ok || strings.TrimSpace(parent) == "" { - log.Fatal(). - Str("course", course). - Str("assignment", assignment). - Msg("'extends' must be the name of another assignment in the same course") + return nil, fmt.Errorf("course %s, assignment %s: 'extends' must be the name of another assignment in the same course", + course, assignment) } parent = strings.TrimSpace(parent) if !viper.IsSet(course + "." + parent) { - log.Fatal(). - Str("course", course). - Str("assignment", assignment). - Str("extends", parent). - Msg("assignment referenced by 'extends' not found") + return nil, fmt.Errorf("course %s, assignment %s: assignment %q referenced by 'extends' not found", + course, assignment, parent) } - parentMap := mergedAssignmentMap(course, parent, seen) + parentMap, err := mergedAssignmentMap(course, parent, seen) + if err != nil { + return nil, err + } - return deepMerge(parentMap, own) + return deepMerge(parentMap, own), nil } // deepMerge returns a new map with child merged onto parent. Nested maps are diff --git a/config/inheritance_test.go b/config/inheritance_test.go index c59fc1d..886e158 100644 --- a/config/inheritance_test.go +++ b/config/inheritance_test.go @@ -63,7 +63,7 @@ func TestInheritance_OverridesAndInherits(t *testing.T) { "url": "git@gitlab.lrz.de:mpd/labs/blatt-10.git", }) - cfg := GetAssignmentConfig("mpd", "blatt10") + cfg := mustAssignmentConfig(t, "mpd", "blatt10") // overridden scalars if cfg.Path != "mpd/ss26/blatt-10" { @@ -122,7 +122,7 @@ func TestInheritance_DeepMergeNestedDeferredBranch(t *testing.T) { }, }) - cfg := GetAssignmentConfig("mpd", "blatt10") + cfg := mustAssignmentConfig(t, "mpd", "blatt10") if len(cfg.DeferredBranches) != 2 { t.Fatalf("DeferredBranches len = %d, want 2 (devcontainer kept)", len(cfg.DeferredBranches)) @@ -153,7 +153,7 @@ func TestInheritance_MultiLevelChain(t *testing.T) { viper.Set("mpd.blatt11.extends", "blatt10") viper.Set("mpd.blatt11.assignmentpath", "blatt-11") - cfg := GetAssignmentConfig("mpd", "blatt11") + cfg := mustAssignmentConfig(t, "mpd", "blatt11") if cfg.Path != "mpd/ss26/blatt-11" { t.Fatalf("Path = %q, want mpd/ss26/blatt-11", cfg.Path) @@ -170,7 +170,7 @@ func TestInheritance_MultiLevelChain(t *testing.T) { func TestInheritance_NoExtendsIsUnaffected(t *testing.T) { baseAssignment(t) - cfg := GetAssignmentConfig("mpd", "blatt09") + cfg := mustAssignmentConfig(t, "mpd", "blatt09") if cfg.Path != "mpd/ss26/blatt-09" { t.Fatalf("Path = %q", cfg.Path) @@ -207,7 +207,7 @@ func TestAssignmentIsAbstract(t *testing.T) { // And after full resolution the abstract flag must not leak into the // effective config. - cfg := GetAssignmentConfig("mpd", "blatt10") + cfg := mustAssignmentConfig(t, "mpd", "blatt10") if cfg.Per != PerStudent { t.Fatalf("Per = %q, want inherited student", cfg.Per) } diff --git a/config/repo.go b/config/repo.go index dd59e9b..93daa1b 100644 --- a/config/repo.go +++ b/config/repo.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "strings" "github.com/go-viper/mapstructure/v2" @@ -8,18 +9,17 @@ import ( "github.com/spf13/viper" ) -func startercode(assignmentKey string) *Startercode { +func startercode(assignmentKey string) (*Startercode, error) { startercodeMap := viper.GetStringMapString(assignmentKey + ".startercode") if len(startercodeMap) == 0 { log.Debug().Str("assignmemtKey", assignmentKey).Msg("no startercode provided") - return nil + return nil, nil } url, ok := startercodeMap["url"] if !ok { - log.Fatal().Str("assignmemtKey", assignmentKey).Msg("startercode provided without url") - return nil + return nil, fmt.Errorf("%s: startercode provided without url", assignmentKey) } fromBranch := "main" @@ -54,10 +54,10 @@ func startercode(assignmentKey string) *Startercode { TemplateMessage: templateMessage, ToBranch: toBranch, AdditionalBranches: additionalBranches, - } + }, nil } -func branches(assignmentKey string, starter *Startercode) []BranchRule { +func branches(assignmentKey string, starter *Startercode) ([]BranchRule, error) { var configured []BranchRule raw := normalizeBranchRuleConfigKeys(viper.Get(assignmentKey + ".branches")) decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ @@ -66,10 +66,10 @@ func branches(assignmentKey string, starter *Startercode) []BranchRule { WeaklyTypedInput: true, }) if err != nil { - log.Fatal().Err(err).Str("assignmentKey", assignmentKey).Msg("cannot create branches decoder") + return nil, fmt.Errorf("%s: cannot create branches decoder: %w", assignmentKey, err) } if err := decoder.Decode(raw); err != nil { - log.Fatal().Err(err).Str("assignmentKey", assignmentKey).Msg("cannot parse branches config") + return nil, fmt.Errorf("%s: cannot parse branches config: %w", assignmentKey, err) } rules := make([]BranchRule, 0) @@ -135,7 +135,7 @@ func branches(assignmentKey string, starter *Startercode) []BranchRule { rules[0].Default = true } - return rules + return rules, nil } func normalizeBranchRuleConfigKeys(value any) any { diff --git a/config/repo_test.go b/config/repo_test.go index 8ceccdc..822b368 100644 --- a/config/repo_test.go +++ b/config/repo_test.go @@ -12,7 +12,7 @@ func TestStartercodeDefaultsAndReplication(t *testing.T) { viper.Set("course.a1.startercode", map[string]string{"url": "git@example.org:starter.git"}) viper.Set("course.a1.startercode.url", "git@example.org:starter.git") - s := startercode("course.a1") + s := mustStartercode(t, "course.a1") if s == nil { t.Fatal("startercode should not be nil") } @@ -33,7 +33,7 @@ func TestStartercodeOverrides(t *testing.T) { viper.Set("course.a1.startercode.toBranch", "submission") viper.Set("course.a1.startercode.additionalBranches", []string{"release", "demo"}) - s := startercode("course.a1") + s := mustStartercode(t, "course.a1") if s.FromBranch != "template" || s.ToBranch != "submission" { t.Fatalf("startercode branches = %#v", s) } @@ -55,7 +55,7 @@ func TestBranches_DefaultsAndLegacyFallback(t *testing.T) { viper.Set("course.a1.startercode.protectToBranch", true) viper.Set("course.a1.startercode.protectDevBranchMergeOnly", true) - b := branches("course.a1", startercode("course.a1")) + b := mustBranches(t, "course.a1", mustStartercode(t, "course.a1")) if len(b) != 3 { t.Fatalf("len(branches) = %d, want 3", len(b)) } @@ -77,7 +77,7 @@ func TestBranches_ExplicitConfig(t *testing.T) { {"name": "dev", "default": true, "mergeOnly": true, "codeOwnerApprovalRequired": true}, }) - b := branches("course.a1", nil) + b := mustBranches(t, "course.a1", nil) if len(b) != 2 { t.Fatalf("len(branches) = %d, want 2", len(b)) } @@ -97,7 +97,7 @@ func TestBranches_MergesAdditionalBranchFlags(t *testing.T) { {"name": "main", "codeOwnerApprovalRequired": true}, }) - b := branches("course.a1", nil) + b := mustBranches(t, "course.a1", nil) if len(b) != 1 { t.Fatalf("len(branches) = %d, want 1", len(b)) } @@ -113,7 +113,7 @@ func TestBranches_LegacySnakeCaseAdditionalBranchFlags(t *testing.T) { {"name": "main", "code_owner_approval_required": true}, }) - b := branches("course.a1", nil) + b := mustBranches(t, "course.a1", nil) if len(b) != 1 { t.Fatalf("len(branches) = %d, want 1", len(b)) } diff --git a/config/seeder.go b/config/seeder.go index ab986d7..423c4b6 100644 --- a/config/seeder.go +++ b/config/seeder.go @@ -12,18 +12,17 @@ import ( "golang.org/x/term" ) -func seeder(assignmentKey string) *Seeder { +func seeder(assignmentKey string) (*Seeder, error) { seederMap := viper.GetStringMapString(assignmentKey + ".seeder") if len(seederMap) == 0 { log.Debug().Str("assignmemtKey", assignmentKey).Msg("no seeder provided") - return nil + return nil, nil } cmd, ok := seederMap["cmd"] if !ok { - log.Fatal().Str("assignmemtKey", assignmentKey).Msg("seeder provided without cmd") - return nil + return nil, fmt.Errorf("%s: seeder provided without cmd", assignmentKey) } toBranch := "main" @@ -33,22 +32,25 @@ func seeder(assignmentKey string) *Seeder { privKeyString := viper.GetString(assignmentKey + ".seeder.signKey") var entity *openpgp.Entity - entity = nil if privKeyString != "" { entities, err := openpgp.ReadArmoredKeyRing(strings.NewReader(privKeyString)) if err != nil { - log.Fatal() + return nil, fmt.Errorf("%s: cannot read seeder.signKey as an armored PGP key ring: %w", assignmentKey, err) + } + if len(entities) == 0 { + return nil, fmt.Errorf("%s: seeder.signKey contains no PGP key", assignmentKey) + } + if entities[0].PrivateKey == nil { + return nil, fmt.Errorf("%s: seeder.signKey contains no private key", assignmentKey) } if entities[0].PrivateKey.Encrypted { fmt.Println(aurora.Blue("Passphrase for signing key is required. Please enter it now:")) passphrase, _ := term.ReadPassword(int(syscall.Stdin)) - err = entities[0].PrivateKey.Decrypt(passphrase) - if err != nil { - log.Fatal() + if err := entities[0].PrivateKey.Decrypt(passphrase); err != nil { + return nil, fmt.Errorf("%s: cannot decrypt seeder.signKey with the given passphrase: %w", assignmentKey, err) } } entity = entities[0] - } return &Seeder{ @@ -59,5 +61,5 @@ func seeder(assignmentKey string) *Seeder { EMail: viper.GetString(assignmentKey + ".seeder.email"), ToBranch: toBranch, ProtectToBranch: viper.GetBool(assignmentKey + ".seeder.protectToBranch"), - } + }, nil } diff --git a/config/seeder_test.go b/config/seeder_test.go index fec699d..16c6971 100644 --- a/config/seeder_test.go +++ b/config/seeder_test.go @@ -8,7 +8,7 @@ import ( func TestSeeder_NoSeeder_ReturnsNil(t *testing.T) { resetViper(t) - if got := seeder("course.a1"); got != nil { + if got := mustSeeder(t, "course.a1"); got != nil { t.Fatalf("seeder() = %#v, want nil", got) } } @@ -20,7 +20,7 @@ func TestSeeder_HappyPath_Defaults(t *testing.T) { viper.Set("course.a1.seeder.name", "Seed Bot") viper.Set("course.a1.seeder.email", "bot@example.com") - got := seeder("course.a1") + got := mustSeeder(t, "course.a1") if got == nil { t.Fatal("seeder() returned nil, want non-nil") } @@ -46,7 +46,7 @@ func TestSeeder_ToBranchOverride(t *testing.T) { viper.Set("course.a1.seeder.cmd", "make") viper.Set("course.a1.seeder.toBranch", "develop") - got := seeder("course.a1") + got := mustSeeder(t, "course.a1") if got == nil { t.Fatal("seeder() returned nil") } @@ -60,7 +60,7 @@ func TestSeeder_WithArgs(t *testing.T) { viper.Set("course.a1.seeder.cmd", "python") viper.Set("course.a1.seeder.args", []string{"seed.py", "--verbose"}) - got := seeder("course.a1") + got := mustSeeder(t, "course.a1") if got == nil { t.Fatal("seeder() returned nil") } diff --git a/config/testdata/errors/edge.yaml b/config/testdata/errors/edge.yaml new file mode 100644 index 0000000..42442d5 --- /dev/null +++ b/config/testdata/errors/edge.yaml @@ -0,0 +1,89 @@ +# Fixtures for the error paths of config loading. Deliberately NOT in +# testdata/courses/: every assignment here fails to resolve, so the golden +# tests must not pick it up. +# +# These tests exist at all only because config loading returns errors instead of +# calling log.Fatal (= os.Exit), which would have killed the test process. +edge: + coursepath: edge/semester + semesterpath: ss2026 + useCoursenameAsPrefix: true + + # An abstract base is a valid config; using it directly is not. + base: + abstract: true + per: student + + # Resolves fine — proves failures below are about the assignment, not the file. + fine: + extends: base + assignmentpath: fine + + # extends cycle across two assignments: cyclea -> cycleb -> cyclea + cyclea: + extends: cycleb + assignmentpath: cycle-a + cycleb: + extends: cyclea + assignmentpath: cycle-b + + # extends cycle of length one + selfcycle: + extends: selfcycle + assignmentpath: self-cycle + + # extends must be a string naming another assignment + extendsnotstring: + extends: [a, b] + assignmentpath: extends-not-string + + extendsempty: + extends: " " + assignmentpath: extends-empty + + extendsmissing: + extends: doesnotexist + assignmentpath: extends-missing + + # startercode requires a url + starterwithouturl: + assignmentpath: starter-without-url + startercode: + fromBranch: template + + # seeder requires a cmd + seederwithoutcmd: + assignmentpath: seeder-without-cmd + seeder: + name: bot + + # seeder.signKey must be an armored PGP key ring. Before the error refactor + # this hit a bare `log.Fatal()` with no .Msg(), which in zerolog neither logs + # nor exits — execution fell through to entities[0] and panicked with index + # out of range. + seederbadsignkey: + assignmentpath: seeder-bad-signkey + seeder: + cmd: /bin/true + signKey: "definitely not an armored pgp key" + + # `users` was renamed to `usernames` + approvalsusers: + assignmentpath: approvals-users + mergeRequest: + approvals: + rules: + - name: review + branches: [main] + users: [obcode] + + # whenCommitAdded is an enum + badwhencommitadded: + assignmentpath: bad-when-commit-added + mergeRequest: + approvals: + settings: + whenCommitAdded: nonsense + + students: + - a@example.edu diff --git a/config/testhelpers_test.go b/config/testhelpers_test.go index 43cf5a8..31eaf43 100644 --- a/config/testhelpers_test.go +++ b/config/testhelpers_test.go @@ -11,3 +11,53 @@ func resetViper(t *testing.T) { viper.Reset() t.Cleanup(viper.Reset) } + +// The must* helpers keep the existing happy-path tests readable now that config +// loading returns errors. Error paths are asserted explicitly in +// errors_test.go, which is only possible at all because these no longer exit +// the process. + +func mustAssignmentConfig(t *testing.T, course, assignment string, onlyFor ...string) *AssignmentConfig { + t.Helper() + cfg, err := GetAssignmentConfig(course, assignment, onlyFor...) + if err != nil { + t.Fatalf("GetAssignmentConfig(%q, %q): unexpected error: %v", course, assignment, err) + } + return cfg +} + +func mustCourseConfig(t *testing.T, course string) *CourseConfig { + t.Helper() + cfg, err := GetCourseConfig(course) + if err != nil { + t.Fatalf("GetCourseConfig(%q): unexpected error: %v", course, err) + } + return cfg +} + +func mustStartercode(t *testing.T, assignmentKey string) *Startercode { + t.Helper() + s, err := startercode(assignmentKey) + if err != nil { + t.Fatalf("startercode(%q): unexpected error: %v", assignmentKey, err) + } + return s +} + +func mustBranches(t *testing.T, assignmentKey string, starter *Startercode) []BranchRule { + t.Helper() + b, err := branches(assignmentKey, starter) + if err != nil { + t.Fatalf("branches(%q): unexpected error: %v", assignmentKey, err) + } + return b +} + +func mustSeeder(t *testing.T, assignmentKey string) *Seeder { + t.Helper() + s, err := seeder(assignmentKey) + if err != nil { + t.Fatalf("seeder(%q): unexpected error: %v", assignmentKey, err) + } + return s +} diff --git a/gitlab/groups.go b/gitlab/groups.go index d2f182d..effcc73 100644 --- a/gitlab/groups.go +++ b/gitlab/groups.go @@ -86,7 +86,10 @@ func (c *Client) createGroup(assignmentCfg *config.AssignmentConfig) (int64, err // AddGroupGuests adds all students from an assignment config as guests to the course subgroup // (coursepath/semesterpath). This enables students to use the Dependency-Proxy. func (c *Client) AddGroupGuests(courseName string) error { - courseConfig := config.GetCourseConfig(courseName) + courseConfig, err := config.GetCourseConfig(courseName) + if err != nil { + return err + } subgroupPath := config.GetCourseSubgroupPath(courseName) log.Info().