From b9a3ab3e304f52a17dc35044c617fbe7fc8b2580 Mon Sep 17 00:00:00 2001 From: Oliver Braun Date: Fri, 17 Jul 2026 14:42:37 +0200 Subject: [PATCH] refactor(config): return errors instead of exiting the process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit config/ called log.Fatal (= os.Exit(1)) on every failure, so it could only ever be used from a CLI: any library or server consumer would be killed mid-request by a config typo. Turn the 17 exit sites into errors and thread them out through GetAssignmentConfig/GetCourseConfig; cmd/ is now the only layer that terminates the process. The goldens from the previous commit pass unchanged, which is the point: this is a signature change, not a behaviour change. Cross-checked independently by diffing `glabs show` output for all 13 assignments in mpd/vss against a binary built before the refactor — identical. (That comparison needs sorted output: config.Show() renders deferredBranches straight from a Go map, so its ordering already varies between runs today. Cosmetic, pre-existing, left alone here.) Error paths are now testable at all, and config/errors_test.go covers all 15 of them against a new testdata/errors/edge.yaml. It lives outside testdata/courses/ so the golden test does not try to resolve fixtures that are meant to fail. Two bare `log.Fatal()` calls in seeder.go turn out to have been worse than they looked. zerolog only exits in Msg()/Send(), so a Fatal() with no message neither logs nor exits: execution fell through to entities[0] on a nil slice and panicked with index out of range. Both are now errors, with the missing len(entities) and PrivateKey == nil guards added. The viper.Set() write-back in the inheritance read path stays for now. Removing it means threading access to the merged map through ~63 readers — exactly the work the typed source schema does next, which would then delete that intermediate again. It disappears for free there, when Resolve() becomes pure by construction. Documented in place until then. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/addgroupguests.go | 7 +- cmd/archive.go | 5 +- cmd/check.go | 5 +- cmd/clone.go | 5 +- cmd/delete.go | 5 +- cmd/generate.go | 5 +- cmd/protect.go | 5 +- cmd/push.go | 7 +- cmd/report.go | 5 +- cmd/setaccess.go | 5 +- cmd/show.go | 5 +- cmd/update.go | 5 +- cmd/urls.go | 5 +- config/assignment.go | 91 +++++++++++------- config/assignment_test.go | 10 +- config/course.go | 10 +- config/course_test.go | 6 +- config/errors_test.go | 160 +++++++++++++++++++++++++++++++ config/golden_test.go | 5 +- config/inheritance.go | 48 +++++----- config/inheritance_test.go | 10 +- config/repo.go | 18 ++-- config/repo_test.go | 12 +-- config/seeder.go | 24 ++--- config/seeder_test.go | 8 +- config/testdata/errors/edge.yaml | 89 +++++++++++++++++ config/testhelpers_test.go | 50 ++++++++++ gitlab/groups.go | 5 +- 28 files changed, 486 insertions(+), 129 deletions(-) create mode 100644 config/errors_test.go create mode 100644 config/testdata/errors/edge.yaml 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().