diff --git a/config/assignment.go b/config/assignment.go index fbb7069..609f2ef 100644 --- a/config/assignment.go +++ b/config/assignment.go @@ -47,6 +47,10 @@ func GetAssignmentConfig(course, assignment string, onlyForStudentsOrGroups ...s containerRegistry = true } + starter := startercode(assignmentKey) + branchRules := branches(assignmentKey, starter) + defaultCloneBranch := defaultBranch(branchRules, "main") + assignmentConfig := &AssignmentConfig{ Course: course, Name: assignment, @@ -58,10 +62,12 @@ func GetAssignmentConfig(course, assignment string, onlyForStudentsOrGroups ...s ContainerRegistry: containerRegistry, AccessLevel: accessLevel(assignmentKey), MergeRequest: mergeRequest(assignmentKey), + Branches: branchRules, + Issues: issues(assignmentKey), Students: students(per, course, assignment, onlyForStudentsOrGroups...), Groups: groups(per, course, assignment, onlyForStudentsOrGroups...), - Startercode: startercode(assignmentKey), - Clone: clone(assignmentKey), + Startercode: starter, + Clone: clone(assignmentKey, defaultCloneBranch), Release: release, Seeder: seeder(assignmentKey), } diff --git a/config/repo.go b/config/repo.go index 285e8b8..bd7c86e 100644 --- a/config/repo.go +++ b/config/repo.go @@ -1,6 +1,8 @@ package config import ( + "strings" + "github.com/rs/zerolog/log" "github.com/spf13/viper" ) @@ -29,40 +31,123 @@ func startercode(assignmentKey string) *Startercode { toBranch = tB } - devBranch := toBranch - if dB := viper.GetString(assignmentKey + ".startercode.devBranch"); len(dB) > 0 { - devBranch = dB + additionalBranches := viper.GetStringSlice(assignmentKey + ".startercode.additionalBranches") + + return &Startercode{ + URL: url, + FromBranch: fromBranch, + ToBranch: toBranch, + AdditionalBranches: additionalBranches, } +} - additionalBranches := []string{} - if addB := viper.GetStringSlice(assignmentKey + ".startercode.additionalBranches"); len(addB) > 0 { - additionalBranches = addB +func branches(assignmentKey string, starter *Startercode) []BranchRule { + var configured []BranchRule + if err := viper.UnmarshalKey(assignmentKey+".branches", &configured); err != nil { + log.Fatal().Err(err).Str("assignmentKey", assignmentKey).Msg("cannot parse branches config") } - replicateIssue := viper.GetBool(assignmentKey + ".startercode.replicateIssue") + rules := make([]BranchRule, 0) + seen := make(map[string]int) + appendOrMerge := func(rule BranchRule) { + rule.Name = strings.TrimSpace(rule.Name) + if rule.Name == "" { + return + } + if idx, ok := seen[rule.Name]; ok { + rules[idx].Protect = rules[idx].Protect || rule.Protect + rules[idx].MergeOnly = rules[idx].MergeOnly || rule.MergeOnly + rules[idx].Default = rules[idx].Default || rule.Default + return + } + seen[rule.Name] = len(rules) + rules = append(rules, rule) + } + + for _, rule := range configured { + appendOrMerge(rule) + } + + if len(rules) == 0 { + if starter != nil { + appendOrMerge(BranchRule{Name: starter.ToBranch, Default: true}) + } + + // Legacy compatibility for old startercode-based branch config. + legacyDevBranch := viper.GetString(assignmentKey + ".startercode.devBranch") + if legacyDevBranch != "" { + appendOrMerge(BranchRule{Name: legacyDevBranch, Default: true}) + } + + for _, branchName := range viper.GetStringSlice(assignmentKey + ".startercode.additionalBranches") { + appendOrMerge(BranchRule{Name: branchName}) + } + + if viper.GetBool(assignmentKey+".startercode.protectToBranch") && starter != nil { + appendOrMerge(BranchRule{Name: starter.ToBranch, Protect: true}) + } - var issueNumbers []int - if replicateIssue { - issueNumbers = []int{1} - if issueNums := viper.GetIntSlice(assignmentKey + ".startercode.issueNumbers"); len(issueNums) > 0 { - issueNumbers = issueNums + if viper.GetBool(assignmentKey + ".startercode.protectDevBranchMergeOnly") { + legacyTarget := legacyDevBranch + if legacyTarget == "" && starter != nil { + legacyTarget = starter.ToBranch + } + appendOrMerge(BranchRule{Name: legacyTarget, MergeOnly: true}) } } - return &Startercode{ - URL: url, - FromBranch: fromBranch, - ToBranch: toBranch, - DevBranch: devBranch, - AdditionalBranches: additionalBranches, - ProtectToBranch: viper.GetBool(assignmentKey + ".startercode.protectToBranch"), - ProtectDevBranchMergeOnly: viper.GetBool(assignmentKey + ".startercode.protectDevBranchMergeOnly"), - ReplicateIssue: replicateIssue, - IssueNumbers: issueNumbers, + hasDefault := false + for _, rule := range rules { + if rule.Default { + hasDefault = true + break + } + } + + if !hasDefault && len(rules) > 0 { + rules[0].Default = true + } + + return rules +} + +func defaultBranch(rules []BranchRule, fallback string) string { + for _, rule := range rules { + if rule.Default && rule.Name != "" { + return rule.Name + } + } + if fallback != "" { + return fallback + } + if len(rules) > 0 { + return rules[0].Name + } + return "main" +} + +func issues(assignmentKey string) *IssueReplication { + replicate := viper.GetBool(assignmentKey + ".issues.replicateFromStartercode") + numbers := viper.GetIntSlice(assignmentKey + ".issues.issueNumbers") + + // Legacy compatibility for old startercode issue replication config. + if !replicate && !viper.IsSet(assignmentKey+".issues") { + replicate = viper.GetBool(assignmentKey + ".startercode.replicateIssue") + numbers = viper.GetIntSlice(assignmentKey + ".startercode.issueNumbers") + } + + if !replicate { + return &IssueReplication{ReplicateFromStartercode: false} + } + + if len(numbers) == 0 { + numbers = []int{1} } + + return &IssueReplication{ReplicateFromStartercode: true, IssueNumbers: numbers} } -func clone(assignmentKey string) *Clone { +func clone(assignmentKey, defaultBranch string) *Clone { cloneMap := viper.GetStringMapString(assignmentKey + ".clone") localpath, ok := cloneMap["localpath"] @@ -72,7 +157,7 @@ func clone(assignmentKey string) *Clone { branch, ok := cloneMap["branch"] if !ok { - branch = "main" + branch = defaultBranch } force := viper.GetBool(assignmentKey + ".clone.force") @@ -89,10 +174,21 @@ func (cfg *AssignmentConfig) SetBranch(branch string) { } func (cfg *AssignmentConfig) SetProtectToBranch(branch string) { - if branch != "" { - cfg.Startercode.ToBranch = branch + if branch == "" && len(cfg.Branches) > 0 { + branch = cfg.Branches[0].Name } - cfg.Startercode.ProtectToBranch = true + if branch == "" { + branch = "main" + } + + for i := range cfg.Branches { + if cfg.Branches[i].Name == branch { + cfg.Branches[i].Protect = true + return + } + } + + cfg.Branches = append(cfg.Branches, BranchRule{Name: branch, Protect: true}) } func (cfg *AssignmentConfig) SetLocalpath(localpath string) { diff --git a/config/repo_test.go b/config/repo_test.go index 58ef855..dc747bc 100644 --- a/config/repo_test.go +++ b/config/repo_test.go @@ -11,21 +11,14 @@ func TestStartercodeDefaultsAndReplication(t *testing.T) { resetViper(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") - viper.Set("course.a1.startercode.replicateIssue", true) s := startercode("course.a1") if s == nil { t.Fatal("startercode should not be nil") } - if s.FromBranch != "main" || s.ToBranch != "main" || s.DevBranch != "main" { + if s.FromBranch != "main" || s.ToBranch != "main" { t.Fatalf("unexpected startercode defaults: %#v", s) } - if !s.ReplicateIssue { - t.Fatal("ReplicateIssue should be true") - } - if !reflect.DeepEqual(s.IssueNumbers, []int{1}) { - t.Fatalf("IssueNumbers = %#v, want [1]", s.IssueNumbers) - } } func TestStartercodeOverrides(t *testing.T) { @@ -34,42 +27,101 @@ func TestStartercodeOverrides(t *testing.T) { viper.Set("course.a1.startercode.url", "git@example.org:starter.git") viper.Set("course.a1.startercode.fromBranch", "template") viper.Set("course.a1.startercode.toBranch", "submission") - viper.Set("course.a1.startercode.devBranch", "develop") viper.Set("course.a1.startercode.additionalBranches", []string{"release", "demo"}) - viper.Set("course.a1.startercode.replicateIssue", true) - viper.Set("course.a1.startercode.issueNumbers", []int{4, 7}) - viper.Set("course.a1.startercode.protectToBranch", true) - viper.Set("course.a1.startercode.protectDevBranchMergeOnly", true) s := startercode("course.a1") - if s.FromBranch != "template" || s.ToBranch != "submission" || s.DevBranch != "develop" { + if s.FromBranch != "template" || s.ToBranch != "submission" { t.Fatalf("startercode branches = %#v", s) } if !reflect.DeepEqual(s.AdditionalBranches, []string{"release", "demo"}) { - t.Fatalf("additional branches = %#v", s.AdditionalBranches) + t.Fatalf("startercode additional branches = %#v", s.AdditionalBranches) } - if !reflect.DeepEqual(s.IssueNumbers, []int{4, 7}) { - t.Fatalf("IssueNumbers = %#v", s.IssueNumbers) +} + +func TestBranches_DefaultsAndLegacyFallback(t *testing.T) { + resetViper(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") + viper.Set("course.a1.startercode.toBranch", "main") + viper.Set("course.a1.startercode.devBranch", "develop") + viper.Set("course.a1.startercode.additionalBranches", []string{"release"}) + viper.Set("course.a1.startercode.protectToBranch", true) + viper.Set("course.a1.startercode.protectDevBranchMergeOnly", true) + + b := branches("course.a1", startercode("course.a1")) + if len(b) != 3 { + t.Fatalf("len(branches) = %d, want 3", len(b)) + } + if b[0].Name != "main" { + t.Fatalf("first branch = %#v", b[0]) } - if !s.ProtectToBranch || !s.ProtectDevBranchMergeOnly { - t.Fatalf("protect flags = %#v", s) + if !b[0].Protect { + t.Fatalf("main branch should be protected: %#v", b[0]) + } + if b[1].Name != "develop" || !b[1].Default || !b[1].MergeOnly { + t.Fatalf("develop branch = %#v", b[1]) + } +} + +func TestBranches_ExplicitConfig(t *testing.T) { + resetViper(t) + viper.Set("course.a1.branches", []map[string]any{ + {"name": "main", "protect": true}, + {"name": "dev", "default": true, "mergeOnly": true}, + }) + + b := branches("course.a1", nil) + if len(b) != 2 { + t.Fatalf("len(branches) = %d, want 2", len(b)) + } + if b[0].Name != "main" || !b[0].Protect { + t.Fatalf("main branch = %#v", b[0]) + } + if b[1].Name != "dev" || !b[1].Default || !b[1].MergeOnly { + t.Fatalf("dev branch = %#v", b[1]) + } +} + +func TestIssues_DefaultsAndLegacyFallback(t *testing.T) { + resetViper(t) + viper.Set("course.a1.startercode.replicateIssue", true) + + i := issues("course.a1") + if i == nil || !i.ReplicateFromStartercode { + t.Fatalf("issues = %#v", i) + } + if !reflect.DeepEqual(i.IssueNumbers, []int{1}) { + t.Fatalf("IssueNumbers = %#v, want [1]", i.IssueNumbers) + } + + viper.Set("course.a1.issues.replicateFromStartercode", true) + viper.Set("course.a1.issues.issueNumbers", []int{4, 7}) + i = issues("course.a1") + if !reflect.DeepEqual(i.IssueNumbers, []int{4, 7}) { + t.Fatalf("IssueNumbers = %#v", i.IssueNumbers) } } func TestCloneDefaultsAndOverrides(t *testing.T) { resetViper(t) - c := clone("course.a1") - if c.LocalPath != "." || c.Branch != "main" || c.Force { + c := clone("course.a1", "develop") + if c.LocalPath != "." || c.Branch != "develop" || c.Force { t.Fatalf("clone defaults = %#v", c) } + resetViper(t) + c = clone("course.a1", "develop") + if c.Branch != "develop" { + t.Fatalf("clone default branch = %q, want %q", c.Branch, "develop") + } + viper.Set("course.a1.clone", map[string]string{"localpath": "/tmp/repos", "branch": "dev"}) viper.Set("course.a1.clone.localpath", "/tmp/repos") viper.Set("course.a1.clone.branch", "dev") viper.Set("course.a1.clone.force", true) - c = clone("course.a1") + c = clone("course.a1", "develop") if c.LocalPath != "/tmp/repos" || c.Branch != "dev" || !c.Force { t.Fatalf("clone overrides = %#v", c) } diff --git a/config/setters_test.go b/config/setters_test.go index 969ee00..fa70f0e 100644 --- a/config/setters_test.go +++ b/config/setters_test.go @@ -19,25 +19,24 @@ func TestSetBranch_Empty(t *testing.T) { } func TestSetProtectToBranch_WithBranch(t *testing.T) { - cfg := &AssignmentConfig{Startercode: &Startercode{ToBranch: "main"}} + cfg := &AssignmentConfig{} cfg.SetProtectToBranch("feature") - if cfg.Startercode.ToBranch != "feature" { - t.Fatalf("ToBranch = %q, want %q", cfg.Startercode.ToBranch, "feature") + if len(cfg.Branches) != 1 { + t.Fatalf("len(Branches) = %d, want 1", len(cfg.Branches)) } - if !cfg.Startercode.ProtectToBranch { - t.Fatal("ProtectToBranch should be true") + if cfg.Branches[0].Name != "feature" || !cfg.Branches[0].Protect { + t.Fatalf("branch config = %#v", cfg.Branches[0]) } } func TestSetProtectToBranch_EmptyBranch(t *testing.T) { - cfg := &AssignmentConfig{Startercode: &Startercode{ToBranch: "main"}} + cfg := &AssignmentConfig{Branches: []BranchRule{{Name: "main"}}} cfg.SetProtectToBranch("") - // empty: ToBranch stays unchanged, ProtectToBranch is set to true - if cfg.Startercode.ToBranch != "main" { - t.Fatalf("ToBranch = %q, want %q", cfg.Startercode.ToBranch, "main") + if len(cfg.Branches) != 1 { + t.Fatalf("len(Branches) = %d, want 1", len(cfg.Branches)) } - if !cfg.Startercode.ProtectToBranch { - t.Fatal("ProtectToBranch should be true even with empty branch string") + if cfg.Branches[0].Name != "main" || !cfg.Branches[0].Protect { + t.Fatalf("branch config = %#v", cfg.Branches[0]) } } diff --git a/config/show.go b/config/show.go index 26be505..0ff7c6c 100644 --- a/config/show.go +++ b/config/show.go @@ -8,9 +8,27 @@ import ( ) func (cfg *AssignmentConfig) Show() { - containerRegistry := aurora.Red("disabled") - if cfg.ContainerRegistry { - containerRegistry = aurora.Green("enabled") + var out strings.Builder + + const ( + topLabelWidth = 19 + sectionLabelWidth = 28 + ) + + writeTopField := func(label string, value any) { + fmt.Fprintf(&out, "%-*v %v\n", topLabelWidth, aurora.Cyan(label+":"), aurora.Yellow(value)) + } + + writeSectionHeader := func(name string) { + fmt.Fprintf(&out, "%s\n", aurora.Cyan(name+":")) + } + + writeSectionField := func(label string, value any) { + fmt.Fprintf(&out, " %-*v %v\n", sectionLabelWidth, aurora.Cyan(label+":"), aurora.Yellow(value)) + } + + writeSectionNotDefined := func() { + fmt.Fprintf(&out, " %s\n", aurora.Red("not defined")) } mergeMethod := MergeCommit @@ -27,178 +45,130 @@ func (cfg *AssignmentConfig) Show() { allThreadsMustBeResolved = cfg.MergeRequest.AllThreadsMustBeResolved statusChecksMustSucceed = cfg.MergeRequest.StatusChecksMustSucceed } - mergeRequestCfg := aurora.Sprintf(aurora.Cyan(` - %s %s - %s %s - %s %t - %s %t - %s %t - %s %t`), - aurora.Cyan("MergeMethod:"), - aurora.Yellow(mergeMethod), - aurora.Cyan("SquashOption:"), - aurora.Yellow(squashOption), - aurora.Cyan("PipelineMustSucceed:"), - aurora.Yellow(pipelineMustSucceed), - aurora.Cyan("SkippedPipelinesAreSuccessful:"), - aurora.Yellow(skippedPipelinesAreSuccessful), - aurora.Cyan("AllThreadsMustBeResolved:"), - aurora.Yellow(allThreadsMustBeResolved), - aurora.Cyan("StatusChecksMustSucceed:"), - aurora.Yellow(statusChecksMustSucceed), - ) - startercode := aurora.Sprintf(aurora.Red("not defined")) - if cfg.Startercode != nil { - issueNumbers := aurora.Sprintf(aurora.Red("not defined")) - if cfg.Startercode.ReplicateIssue { - issueNumbers = aurora.Sprintf(aurora.Yellow(cfg.Startercode.IssueNumbers)) - } + containerRegistry := aurora.Red("disabled") + if cfg.ContainerRegistry { + containerRegistry = aurora.Green("enabled") + } + + writeTopField("Course", cfg.Course) + writeTopField("Assignment", cfg.Name) + writeTopField("Coursename-Prefix", cfg.UseCoursenameAsPrefix) + writeTopField("Per", cfg.Per) + writeTopField("Base-URL", cfg.URL) + writeTopField("Description", cfg.Description) + writeTopField("AccessLevel", cfg.AccessLevel.String()) + writeTopField("Container-Registry", containerRegistry) + + writeSectionHeader("MergeRequest") + writeSectionField("MergeMethod", mergeMethod) + writeSectionField("SquashOption", squashOption) + writeSectionField("PipelineMustSucceed", pipelineMustSucceed) + writeSectionField("SkippedPipelinesAreSuccessful", skippedPipelinesAreSuccessful) + writeSectionField("AllThreadsMustBeResolved", allThreadsMustBeResolved) + writeSectionField("StatusChecksMustSucceed", statusChecksMustSucceed) + + writeSectionHeader("Startercode") + if cfg.Startercode == nil { + writeSectionNotDefined() + } else { + writeSectionField("URL", cfg.Startercode.URL) + writeSectionField("FromBranch", cfg.Startercode.FromBranch) + writeSectionField("ToBranch", cfg.Startercode.ToBranch) + writeSectionField("AdditionalBranches", cfg.Startercode.AdditionalBranches) + } - startercode = aurora.Sprintf(aurora.Cyan(` - URL: %s - FromBranch: %s - ToBranch: %s - DevBranch: %s - AdditionalBranches: %s - ProtectToBranch: %t - ProtectDevBranchMergeOnly: %t - ReplicateIssue: %t - IssueNumbers: %s`), - aurora.Yellow(cfg.Startercode.URL), - aurora.Yellow(cfg.Startercode.FromBranch), - aurora.Yellow(cfg.Startercode.ToBranch), - aurora.Yellow(cfg.Startercode.DevBranch), - aurora.Yellow(cfg.Startercode.AdditionalBranches), - aurora.Yellow(cfg.Startercode.ProtectToBranch), - aurora.Yellow(cfg.Startercode.ProtectDevBranchMergeOnly), - aurora.Yellow(cfg.Startercode.ReplicateIssue), - issueNumbers, - ) + writeSectionHeader("Branches") + if len(cfg.Branches) == 0 { + writeSectionNotDefined() + } else { + for _, branch := range cfg.Branches { + fmt.Fprintf( + &out, + " - %-20v (%v=%-5v, %v=%-5v, %v=%-5v)\n", + aurora.Yellow(branch.Name), + aurora.Cyan("protect"), aurora.Yellow(branch.Protect), + aurora.Cyan("mergeOnly"), aurora.Yellow(branch.MergeOnly), + aurora.Cyan("default"), aurora.Yellow(branch.Default), + ) + } } - seeding := aurora.Sprintf(aurora.Red("not defined")) - if cfg.Seeder != nil { - seeding = aurora.Sprintf(aurora.Cyan(` - Command: %s - Args: %v - Author: %s - EMail: %s - ToBranch: %s - ProtectToBranch: %t`), - aurora.Yellow(cfg.Seeder.Command), - aurora.Yellow(cfg.Seeder.Args), - aurora.Yellow(cfg.Seeder.Name), - aurora.Yellow(cfg.Seeder.EMail), - aurora.Yellow(cfg.Seeder.ToBranch), - aurora.Yellow(cfg.Seeder.ProtectToBranch), - ) + + writeSectionHeader("Issues") + if cfg.Issues == nil { + writeSectionNotDefined() + } else { + writeSectionField("ReplicateFromStartercode", cfg.Issues.ReplicateFromStartercode) + if cfg.Issues.ReplicateFromStartercode { + writeSectionField("IssueNumbers", cfg.Issues.IssueNumbers) + } else { + writeSectionField("IssueNumbers", "not used") + } } - clone := aurora.Sprintf(aurora.Red(" not defined")) - if cfg.Clone != nil { - clone = aurora.Sprintf(aurora.Cyan(`Clone: - Localpath: %s - Branch: %s - Force: %t`), - aurora.Yellow(cfg.Clone.LocalPath), - aurora.Yellow(cfg.Clone.Branch), - aurora.Yellow(cfg.Clone.Force), - ) + + writeSectionHeader("Seeding") + if cfg.Seeder == nil { + writeSectionNotDefined() + } else { + writeSectionField("Command", cfg.Seeder.Command) + writeSectionField("Args", cfg.Seeder.Args) + writeSectionField("Author", cfg.Seeder.Name) + writeSectionField("EMail", cfg.Seeder.EMail) + writeSectionField("ToBranch", cfg.Seeder.ToBranch) + writeSectionField("ProtectToBranch", cfg.Seeder.ProtectToBranch) } - release := aurora.Sprintf(aurora.Red("not defined")) - if cfg.Release != nil { - releaseMergeRequest := aurora.Sprintf(aurora.Red("not defined")) - if cfg.Release.MergeRequest != nil { - releaseMergeRequest = aurora.Sprintf(` - %s %s - %s %s - %s %t`, - aurora.Cyan("SourceBranch:"), - aurora.Yellow(cfg.Release.MergeRequest.SourceBranch), - aurora.Cyan("TargetBranch:"), - aurora.Yellow(cfg.Release.MergeRequest.TargetBranch), - aurora.Cyan("Pipeline:"), - aurora.Yellow(cfg.Release.MergeRequest.HasPipeline), - ) + + writeSectionHeader("Release") + if cfg.Release == nil { + writeSectionNotDefined() + } else { + if cfg.Release.MergeRequest == nil { + writeSectionField("MergeRequest", "not defined") + } else { + writeSectionField("MergeRequest.SourceBranch", cfg.Release.MergeRequest.SourceBranch) + writeSectionField("MergeRequest.TargetBranch", cfg.Release.MergeRequest.TargetBranch) + writeSectionField("MergeRequest.Pipeline", cfg.Release.MergeRequest.HasPipeline) } - dockerImages := aurora.Sprintf(aurora.Red("not defined")) - if cfg.Release.DockerImages != nil { - var images strings.Builder - images.WriteString(aurora.Sprintf(aurora.Cyan("\n"))) - for _, image := range cfg.Release.DockerImages { - images.WriteString(aurora.Sprintf(aurora.Cyan(" - "))) - images.WriteString(aurora.Sprintf(aurora.Yellow(image))) - images.WriteString("\n") - } - dockerImages = images.String() + if len(cfg.Release.DockerImages) == 0 { + writeSectionField("DockerImages", "not defined") + } else { + writeSectionField("DockerImages", cfg.Release.DockerImages) } - release = aurora.Sprintf(aurora.Cyan(` - %s %s - %s %s`), - aurora.Cyan("MergeRequest:"), - releaseMergeRequest, - aurora.Cyan("DockerImages:"), - dockerImages, - ) } - var per strings.Builder + writeSectionHeader("Clone") + if cfg.Clone == nil { + writeSectionNotDefined() + } else { + writeSectionField("Localpath", cfg.Clone.LocalPath) + writeSectionField("Branch", cfg.Clone.Branch) + writeSectionField("Force", cfg.Clone.Force) + } + switch cfg.Per { case PerStudent: - per.WriteString(aurora.Sprintf(aurora.Cyan("Students:\n"))) - for _, s := range cfg.Students { - per.WriteString(aurora.Sprintf(aurora.Cyan(" - "))) - per.WriteString(aurora.Sprintf(aurora.Yellow(s.Raw))) - per.WriteString("\n") + writeSectionHeader("Students") + if len(cfg.Students) == 0 { + writeSectionNotDefined() + } else { + for _, s := range cfg.Students { + fmt.Fprintf(&out, " - %s\n", aurora.Yellow(s.Raw)) + } } case PerGroup: - per.WriteString(aurora.Sprintf(aurora.Cyan("Groups:\n"))) - for _, grp := range cfg.Groups { - per.WriteString(aurora.Sprintf(aurora.Cyan(" - "))) - per.WriteString(aurora.Sprintf(aurora.Yellow(grp.Name))) - per.WriteString(aurora.Sprintf(aurora.Cyan(": "))) - for i, m := range grp.Members { - per.WriteString(aurora.Sprintf(aurora.Green(m.Raw))) - if i == len(grp.Members)-1 { - per.WriteString("\n") - } else { - per.WriteString(aurora.Sprintf(aurora.Cyan(", "))) + writeSectionHeader("Groups") + if len(cfg.Groups) == 0 { + writeSectionNotDefined() + } else { + for _, grp := range cfg.Groups { + members := make([]string, 0, len(grp.Members)) + for _, m := range grp.Members { + members = append(members, m.Raw) } + fmt.Fprintf(&out, " - %s: %s\n", aurora.Yellow(grp.Name), aurora.Green(strings.Join(members, ", "))) } } } - groupsOrStudents := per.String() - - fmt.Print(aurora.Sprintf(aurora.Cyan(` -Course: %s -Assignment: %s -Coursename-Prefix: %t -Per: %s -Base-URL: %s -Description: %s -AccessLevel: %s -MergeRequest: %s -%s %s -Startercode: %s -Seeding: %s -Release: %s -%s -%s -`), - aurora.Yellow(cfg.Course), - aurora.Yellow(cfg.Name), - aurora.Yellow(cfg.UseCoursenameAsPrefix), - aurora.Yellow(cfg.Per), - aurora.Yellow(cfg.URL), - aurora.Yellow(cfg.Description), - aurora.Yellow(cfg.AccessLevel.String()), - mergeRequestCfg, - aurora.Cyan("Container-Registry:"), - containerRegistry, - aurora.Yellow(startercode), - aurora.Yellow(seeding), - aurora.Yellow(release), - aurora.Yellow(clone), - aurora.Yellow(groupsOrStudents), - )) + fmt.Println(out.String()) } diff --git a/config/types.go b/config/types.go index 93029c1..6bd71a6 100644 --- a/config/types.go +++ b/config/types.go @@ -20,6 +20,8 @@ type AssignmentConfig struct { ContainerRegistry bool AccessLevel AccessLevel MergeRequest *MergeRequest + Branches []BranchRule + Issues *IssueReplication Students []*Student Groups []*Group Startercode *Startercode @@ -47,15 +49,22 @@ type Seeder struct { } type Startercode struct { - URL string - FromBranch string - ToBranch string - DevBranch string - AdditionalBranches []string - ProtectToBranch bool - ProtectDevBranchMergeOnly bool - ReplicateIssue bool - IssueNumbers []int + URL string + FromBranch string + ToBranch string + AdditionalBranches []string +} + +type BranchRule struct { + Name string `mapstructure:"name"` + Protect bool `mapstructure:"protect"` + MergeOnly bool `mapstructure:"mergeOnly"` + Default bool `mapstructure:"default"` +} + +type IssueReplication struct { + ReplicateFromStartercode bool + IssueNumbers []int } type Clone struct { diff --git a/config/urls_show_test.go b/config/urls_show_test.go index cffe120..66c422e 100644 --- a/config/urls_show_test.go +++ b/config/urls_show_test.go @@ -98,28 +98,57 @@ func TestShow_ContainerRegistryEnabled(t *testing.T) { func TestShow_WithStartercode_NoIssues(t *testing.T) { cfg := &AssignmentConfig{ Startercode: &Startercode{ - URL: "https://gitlab.example.org/starter", - FromBranch: "main", - ToBranch: "main", - ReplicateIssue: false, + URL: "https://gitlab.example.org/starter", + FromBranch: "main", + ToBranch: "main", }, } cfg.Show() } +func TestShow_WithStartercode_AdditionalBranches(t *testing.T) { + cfg := &AssignmentConfig{ + Startercode: &Startercode{ + URL: "https://gitlab.example.org/starter", + FromBranch: "main", + ToBranch: "main", + AdditionalBranches: []string{"solution", "startercode"}, + }, + } + out := captureStdout(t, func() { cfg.Show() }) + if !strings.Contains(out, "AdditionalBranches") || !strings.Contains(out, "solution") { + t.Fatalf("Show() output does not contain startercode additional branches: %q", out) + } +} + func TestShow_WithStartercode_WithIssues(t *testing.T) { cfg := &AssignmentConfig{ Startercode: &Startercode{ - URL: "https://gitlab.example.org/starter", - FromBranch: "main", - ToBranch: "main", - ReplicateIssue: true, - IssueNumbers: []int{1, 2, 3}, + URL: "https://gitlab.example.org/starter", + FromBranch: "main", + ToBranch: "main", + }, + Issues: &IssueReplication{ + ReplicateFromStartercode: true, + IssueNumbers: []int{1, 2, 3}, }, } cfg.Show() } +func TestShow_WithBranches(t *testing.T) { + cfg := &AssignmentConfig{ + Branches: []BranchRule{{Name: "main", Protect: true, Default: true}, {Name: "develop", MergeOnly: true}}, + } + out := captureStdout(t, func() { cfg.Show() }) + if !strings.Contains(out, "Branches:") || !strings.Contains(out, "develop") { + t.Fatalf("Show() output does not contain branches block: %q", out) + } + if !strings.Contains(out, "mergeOnly") || !strings.Contains(out, "default") { + t.Fatalf("Show() output does not contain compact branch flags: %q", out) + } +} + func TestShow_WithSeeder(t *testing.T) { cfg := &AssignmentConfig{ Seeder: &Seeder{ @@ -280,3 +309,29 @@ func TestShow_WithMergeChecks(t *testing.T) { t.Fatalf("Show() output does not contain StatusChecksMustSucceed key: %q", out) } } + +func TestShow_NoFmtArtifacts(t *testing.T) { + cfg := &AssignmentConfig{ + Course: "mpd", + Name: "blatt01", + UseCoursenameAsPrefix: true, + Per: PerStudent, + MergeRequest: &MergeRequest{ + PipelineMustSucceed: true, + SkippedPipelinesAreSuccessful: true, + AllThreadsMustBeResolved: true, + StatusChecksMustSucceed: true, + }, + Branches: []BranchRule{{Name: "main", MergeOnly: true, Default: true}}, + Issues: &IssueReplication{ + ReplicateFromStartercode: true, + IssueNumbers: []int{1}, + }, + Clone: &Clone{LocalPath: ".", Branch: "main", Force: false}, + } + + out := captureStdout(t, func() { cfg.Show() }) + if strings.Contains(out, "%!") { + t.Fatalf("Show() output contains fmt artifacts: %q", out) + } +} diff --git a/docs/advanced.md b/docs/advanced.md index bde540b..efda68f 100644 --- a/docs/advanced.md +++ b/docs/advanced.md @@ -17,9 +17,12 @@ mpd: url: git@gitlab.lrz.de:mpd/starter.git fromBranch: template toBranch: main - devBranch: develop - protectToBranch: true # main is locked - protectDevBranchMergeOnly: false + + branches: + - name: main + protect: true # main is locked + - name: develop + default: true release: mergeRequest: diff --git a/docs/configuration.md b/docs/configuration.md index a457a2f..4a2e9d4 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -7,6 +7,8 @@ glabs uses two configuration layers: Configuration is merged from top to bottom: main config → course config → assignment overrides → defaults. +If you are migrating from the previous startercode structure, use [migration.md](migration.md). + ## Main config Default file: ~/.glabs with any Viper-supported extension (for example .yaml, .yml, .json) @@ -178,14 +180,7 @@ startercode: url: git@gitlab.example.org:mpd/startercode/blatt-01.git fromBranch: template # Clone this branch toBranch: main # Into this branch - devBranch: develop # Development branch (if different) - additionalBranches: [] # Create additional branches - - protectToBranch: true - protectDevBranchMergeOnly: false - - replicateIssue: true - issueNumbers: [1, 3] + additionalBranches: [] # Push starter/ to repo/ ``` ### Startercode keys @@ -195,27 +190,72 @@ startercode: | `url` | SSH URL of starter repo | — | Required if startercode block exists | | `fromBranch` | Source branch in starter | `main` | Must exist in starter repo | | `toBranch` | Target branch in new repos | `main` | Usually production branch | -| `devBranch` | Development branch | value of `toBranch` | For student work | -| `additionalBranches` | Extra branches to create | `[]` | Names only, empty branches | -| `protectToBranch` | Protect `toBranch` | `false` | Maintainer-only push/merge | -| `protectDevBranchMergeOnly` | Merge-only for dev branch | `false` | Devs can merge but not push | -| `replicateIssue` | Copy issues from starter | `false` | For assignment requirements | -| `issueNumbers` | Which issues to copy | `[1]` | Only if `replicateIssue: true` | +| `additionalBranches` | Additional branches mirrored from starter repo | `[]` | Each `x` maps `starter/x -> repo/x` | + +Important behavior: + +- Only branches listed in `startercode.additionalBranches` are mirrored from starter with branch-specific content. +- All other generated branches are created from the state of `startercode.toBranch`. +- This means that branches from the `branches:` block that are not in `additionalBranches` start identical to `toBranch`. + +## Branch options + +Define branch creation, protection and default branch independently from startercode: + +```yaml +branches: + - name: main + protect: true + default: false + + - name: develop + mergeOnly: true + default: true +``` + +### Branch keys + +| Key | Purpose | Default | Notes | +|---|---|---|---| +| `name` | Branch name | — | Required | +| `protect` | Maintainer-only push/merge | `false` | Same semantics as classic protected branch | +| `mergeOnly` | Developers may merge but cannot push | `false` | Sets push=`No one`, merge=`Developers` | +| `default` | Set as project default branch | `false` | If none is set, first branch becomes default | ### Branch protection notes -- **`protectToBranch: true`**: Only Maintainers can push/merge to `toBranch` -- **`protectDevBranchMergeOnly: true`**: Developers can merge but not push to `devBranch` -- If `devBranch == toBranch` and both flags enabled: merge-only rule applies to that branch +- `mergeOnly: true` takes precedence over `protect: true` for that branch +- Branches are created for generated projects based on this list +- Branch creation base is `startercode.toBranch` (or `seeder.toBranch` when seeder is used) +- If a branch name is also listed in `startercode.additionalBranches`, the mirrored starter branch content is used for that branch +- `glabs protect` applies these branch rules to existing repositories -**Example: Merge-only dev branch** +## Issue replication options + +Configure issue replication separately from startercode: ```yaml -startercode: - fromBranch: template - toBranch: main - devBranch: main - protectDevBranchMergeOnly: true +issues: + replicateFromStartercode: true + issueNumbers: [1, 3] +``` + +### Issue keys + +| Key | Purpose | Default | Notes | +|---|---|---|---| +| `replicateFromStartercode` | Copy issues from starter project | `false` | Requires `startercode` | +| `issueNumbers` | Which issue numbers to copy | `[1]` | Used only when replication is enabled | + +**Example: Merge-only development branch** + +```yaml +branches: + - name: main + protect: true + - name: develop + mergeOnly: true + default: true ``` Result in GitLab UI: @@ -411,10 +451,16 @@ mpd: url: git@gitlab.lrz.de:teaching/2024ss/mpd/starter-blatt01.git fromBranch: template toBranch: main - devBranch: develop - protectToBranch: false - protectDevBranchMergeOnly: true - replicateIssue: true + + branches: + - name: main + protect: true + - name: develop + mergeOnly: true + default: true + + issues: + replicateFromStartercode: true issueNumbers: [1] blatt02: diff --git a/docs/getting-started.md b/docs/getting-started.md index e05bc7f..121a879 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -85,11 +85,12 @@ mpd: | Assignment | `mergeRequest.statusChecksMustSucceed` | `false` | | Startercode | `fromBranch` | `main` | | Startercode | `toBranch` | `main` | -| Startercode | `devBranch` | value of `toBranch` | -| Startercode | `protectToBranch` | `false` | -| Startercode | `protectDevBranchMergeOnly` | `false` | -| Startercode | `replicateIssue` | `false` | -| Startercode | `issueNumbers` | `[1]` when `replicateIssue: true` | +| Startercode | `additionalBranches` | `[]` | +| Branches | first `branches[].default` | `true` on first branch | +| Branches | `branches[].protect` | `false` | +| Branches | `branches[].mergeOnly` | `false` | +| Issues | `issues.replicateFromStartercode` | `false` | +| Issues | `issues.issueNumbers` | `[1]` when replication is enabled | | Seeder | `toBranch` | `main` | | Seeder | `protectToBranch` | `false` | | Clone | `localpath` | `.` | @@ -114,6 +115,7 @@ glabs urls mpd blatt01 ## Next steps - Read full config keys in [configuration.md](configuration.md) +- If you use legacy startercode branch/issue keys, migrate with [migration.md](migration.md) - Follow common workflows in [workflows.md](workflows.md) - See all commands in [commands.md](commands.md) - If something fails, check [troubleshooting.md](troubleshooting.md) diff --git a/docs/migration.md b/docs/migration.md new file mode 100644 index 0000000..72f29b1 --- /dev/null +++ b/docs/migration.md @@ -0,0 +1,110 @@ +# Migration Guide: Startercode Refactor + +This guide helps you migrate from legacy assignment config keys under `startercode` to the new independent blocks: + +- `startercode`: only repository source/target for initial code push +- `branches`: branch creation, protection, merge-only behavior, default branch +- `issues`: issue replication from starter project + +## Why this changed + +The previous model mixed unrelated responsibilities in `startercode`: + +- starter code source (`url`, `fromBranch`, `toBranch`) +- branch topology and protection +- issue replication policy + +The new layout separates these concerns and lets you use branch rules and issue replication independently from starter code. + +## 30-second cheat sheet + +```yaml +# keep in startercode: +startercode: + url: ... + fromBranch: main + toBranch: main + additionalBranches: [release] # optional, mirrors starter/release -> repo/release + +# move branch policy here: +branches: + - name: main + protect: true + - name: develop + mergeOnly: true + default: true + +# move issue replication here: +issues: + replicateFromStartercode: true + issueNumbers: [1, 3] +``` + +## Old to new mapping + +| Legacy key | New key | Notes | +|---|---|---| +| `startercode.devBranch` | `branches[].name` + `branches[].default=true` | Dev branch is now just a default branch rule | +| `startercode.additionalBranches` | `startercode.additionalBranches` | Stays in startercode; each `x` mirrors `starter/x -> repo/x` | +| `startercode.protectToBranch` | `branches[]` for `toBranch` with `protect=true` | Maintainer-only push/merge | +| `startercode.protectDevBranchMergeOnly` | `branches[]` for dev branch with `mergeOnly=true` | Developers can merge, cannot push | +| `startercode.replicateIssue` | `issues.replicateFromStartercode` | Moved to issue config | +| `startercode.issueNumbers` | `issues.issueNumbers` | Defaults to `[1]` when replication is enabled | + +## Example migration + +### Before + +```yaml +blatt01: + startercode: + url: git@gitlab.example.org:course/starter.git + fromBranch: template + toBranch: main + additionalBranches: [release] + devBranch: develop + protectToBranch: true + protectDevBranchMergeOnly: true + replicateIssue: true + issueNumbers: [1, 3] +``` + +### After + +```yaml +blatt01: + startercode: + url: git@gitlab.example.org:course/starter.git + fromBranch: template + toBranch: main + additionalBranches: [release] + + branches: + - name: main + protect: true + - name: develop + mergeOnly: true + default: true + + issues: + replicateFromStartercode: true + issueNumbers: [1, 3] +``` + +## Recommended rollout + +1. Move one assignment first (pilot migration). +2. Run `glabs check `. +3. Preview with `glabs show `. +4. For existing repos, apply changed branch protections with `glabs protect `. +5. For new repos, use `glabs generate `. + +## Backward compatibility + +Legacy keys under `startercode` are still read as fallback for compatibility. + +Recommended practice: + +- Migrate all course files to the new keys +- Do not mix old and new keys in the same assignment block +- Keep `startercode` focused on repo source (`url`, `fromBranch`, `toBranch`) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 7cb0f0c..eaf29df 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -159,12 +159,13 @@ Expected: If wrong, check config: ```yaml -startercode: - devBranch: main - protectDevBranchMergeOnly: true # Must be true +branches: + - name: main + mergeOnly: true # Must be true + default: true ``` -Note: Only works if `devBranch` and `toBranch` are the same. +Note: `mergeOnly` applies exactly to the branch specified in `name`. ## Modify existing repositories diff --git a/docs/workflows.md b/docs/workflows.md index a2d8002..8156ded 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -26,12 +26,13 @@ Use this after changing branch protection settings in config. Does not regenerat ## Merge-only development branch -Use this startercode config: +Use this branch config: ```yaml -startercode: - devBranch: main - protectDevBranchMergeOnly: true +branches: + - name: main + mergeOnly: true + default: true ``` Expected behavior in GitLab UI: @@ -233,11 +234,11 @@ This enables a GitLab Flow where development happens on `develop` and production ## Replicate issues from starter repository -When creating new repositories from startercode, automatically copy issues: +When creating new repositories from startercode, configure issue replication with `issues`: ```yaml -startercode: - replicateIssue: true +issues: + replicateFromStartercode: true issueNumbers: [1, 3, 7] ``` @@ -253,7 +254,8 @@ startercode: ```yaml startercode: url: git@gitlab.example.org:course/starter.git - replicateIssue: true +issues: + replicateFromStartercode: true issueNumbers: [1] # Replicate first issue (usually assignment spec) ``` diff --git a/gitlab/branches.go b/gitlab/branches.go new file mode 100644 index 0000000..0b367ef --- /dev/null +++ b/gitlab/branches.go @@ -0,0 +1,70 @@ +package gitlab + +import ( + "fmt" + "strings" + + "github.com/obcode/glabs/config" + "github.com/rs/zerolog/log" + gitlab "gitlab.com/gitlab-org/api/client-go/v2" +) + +func (c *Client) syncConfiguredBranches(assignmentCfg *config.AssignmentConfig, project *gitlab.Project, baseBranch string) error { + if len(assignmentCfg.Branches) == 0 { + return nil + } + + for _, branch := range assignmentCfg.Branches { + if branch.Name == "" { + continue + } + + if branch.Name != baseBranch { + opts := &gitlab.CreateBranchOptions{ + Branch: gitlab.Ptr(branch.Name), + Ref: gitlab.Ptr(baseBranch), + } + + _, _, err := c.Branches.CreateBranch(project.ID, opts) + if err != nil && !isBranchAlreadyExistsError(err) { + return fmt.Errorf("error while creating branch %s from %s: %w", branch.Name, baseBranch, err) + } + } + } + + defaultBranch := defaultBranchName(assignmentCfg.Branches, baseBranch) + projectOpts := &gitlab.EditProjectOptions{DefaultBranch: gitlab.Ptr(defaultBranch)} + _, _, err := c.Projects.EditProject(project.ID, projectOpts) + if err != nil { + return fmt.Errorf("error while switching default branch to %s: %w", defaultBranch, err) + } + + if err := c.protectBranch(assignmentCfg, project, false); err != nil { + log.Debug().Err(err).Str("project", project.Name).Msg("cannot protect configured branches") + } + + return nil +} + +func defaultBranchName(branches []config.BranchRule, fallback string) string { + for _, branch := range branches { + if branch.Default && branch.Name != "" { + return branch.Name + } + } + + if fallback != "" { + return fallback + } + + if len(branches) > 0 && branches[0].Name != "" { + return branches[0].Name + } + + return "main" +} + +func isBranchAlreadyExistsError(err error) bool { + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "already exists") || strings.Contains(msg, "has already been taken") +} diff --git a/gitlab/generate.go b/gitlab/generate.go index 847169d..1491cb0 100644 --- a/gitlab/generate.go +++ b/gitlab/generate.go @@ -161,11 +161,24 @@ func (c *Client) generate(assignmentCfg *config.AssignmentConfig, assignmentGrou } } + if generated && len(assignmentCfg.Branches) > 0 { + baseBranch := defaultBranchName(assignmentCfg.Branches, "main") + if assignmentCfg.Startercode != nil { + baseBranch = assignmentCfg.Startercode.ToBranch + } else if assignmentCfg.Seeder != nil { + baseBranch = assignmentCfg.Seeder.ToBranch + } + + if err := c.syncConfiguredBranches(assignmentCfg, project, baseBranch); err != nil { + log.Debug().Err(err).Str("project", project.Name).Msg("cannot apply configured branch rules") + } + } + // Replicate issues from startercode repo if configured - if generated && assignmentCfg.Startercode != nil && assignmentCfg.Startercode.ReplicateIssue { + if generated && assignmentCfg.Startercode != nil && assignmentCfg.Issues != nil && assignmentCfg.Issues.ReplicateFromStartercode { starterProject, starterProjectErr := c.getStartercodeProject(assignmentCfg) - for _, issueNumber := range assignmentCfg.Startercode.IssueNumbers { + for _, issueNumber := range assignmentCfg.Issues.IssueNumbers { cfg.Suffix = aurora.Sprintf( aurora.Cyan(" ↪ replicating issue #%d from startercode"), aurora.Yellow(issueNumber), diff --git a/gitlab/integration_gitlab_test.go b/gitlab/integration_gitlab_test.go index def0db2..ecb239f 100644 --- a/gitlab/integration_gitlab_test.go +++ b/gitlab/integration_gitlab_test.go @@ -264,10 +264,7 @@ func TestIntegration_GitLab_Operations(t *testing.T) { t.Run("ProtectToBranch", func(t *testing.T) { // withReadme=true so the project has a 'main' branch immediately cfg := createGroupAndProject(t, "protect-a1", "student1", true) - cfg.Startercode = &config.Startercode{ - ToBranch: "main", - ProtectToBranch: true, - } + cfg.Branches = []config.BranchRule{{Name: "main", Protect: true, Default: true}} client.ProtectToBranch(cfg) diff --git a/gitlab/protect.go b/gitlab/protect.go index 2a45ae5..6c6feeb 100644 --- a/gitlab/protect.go +++ b/gitlab/protect.go @@ -2,6 +2,7 @@ package gitlab import ( "fmt" + "strings" "time" "github.com/logrusorgru/aurora" @@ -30,7 +31,7 @@ func (c *Client) ProtectToBranch(assignmentCfg *config.AssignmentConfig) { } func (c *Client) protectBranch(assignmentCfg *config.AssignmentConfig, project *gitlab.Project, spin bool) error { - if assignmentCfg.Startercode.ProtectToBranch || assignmentCfg.Startercode.ProtectDevBranchMergeOnly { + if hasProtectedBranches(assignmentCfg.Branches) { // var cfg yacspin.Config var spinner *yacspin.Spinner if spin { @@ -62,17 +63,22 @@ func (c *Client) protectBranch(assignmentCfg *config.AssignmentConfig, project * log.Debug(). Str("name", project.Name). Str("toURL", project.SSHURLToRepo). - Str("branch", assignmentCfg.Startercode.ToBranch). + Interface("branches", assignmentCfg.Branches). Msg("protecting branch") - if assignmentCfg.Startercode.ProtectDevBranchMergeOnly && - assignmentCfg.Startercode.DevBranch == assignmentCfg.Startercode.ToBranch { - err := c.protectSingleBranch( - project, - assignmentCfg.Startercode.ToBranch, - gitlab.NoPermissions, - gitlab.DeveloperPermissions, - ) + for _, branch := range assignmentCfg.Branches { + if !branch.Protect && !branch.MergeOnly { + continue + } + + pushLevel := gitlab.MaintainerPermissions + mergeLevel := gitlab.MaintainerPermissions + if branch.MergeOnly { + pushLevel = gitlab.NoPermissions + mergeLevel = gitlab.DeveloperPermissions + } + + err := c.protectSingleBranch(project, branch.Name, pushLevel, mergeLevel) if err != nil { if spin { err := spinner.StopFail() @@ -82,42 +88,6 @@ func (c *Client) protectBranch(assignmentCfg *config.AssignmentConfig, project * } return err } - } else { - if assignmentCfg.Startercode.ProtectToBranch { - err := c.protectSingleBranch( - project, - assignmentCfg.Startercode.ToBranch, - gitlab.MaintainerPermissions, - gitlab.MaintainerPermissions, - ) - if err != nil { - if spin { - err := spinner.StopFail() - if err != nil { - log.Debug().Err(err).Msg("cannot stop spinner") - } - } - return err - } - } - - if assignmentCfg.Startercode.ProtectDevBranchMergeOnly { - err := c.protectSingleBranch( - project, - assignmentCfg.Startercode.DevBranch, - gitlab.NoPermissions, - gitlab.DeveloperPermissions, - ) - if err != nil { - if spin { - err := spinner.StopFail() - if err != nil { - log.Debug().Err(err).Msg("cannot stop spinner") - } - } - return err - } - } } if spin { @@ -131,19 +101,50 @@ func (c *Client) protectBranch(assignmentCfg *config.AssignmentConfig, project * return nil } +func hasProtectedBranches(branches []config.BranchRule) bool { + for _, branch := range branches { + if branch.Protect || branch.MergeOnly { + return true + } + } + + return false +} + func (c *Client) protectSingleBranch( project *gitlab.Project, branch string, pushAccessLevel gitlab.AccessLevelValue, mergeAccessLevel gitlab.AccessLevelValue, ) error { - _, err := c.ProtectedBranches.UnprotectRepositoryBranches(project.ID, branch) - if err != nil { + existing, _, err := c.ProtectedBranches.GetProtectedBranch(project.ID, branch) + if err == nil { + updateOpts := &gitlab.UpdateProtectedBranchOptions{ + AllowedToPush: replaceBranchPermissions(existing.PushAccessLevels, pushAccessLevel), + AllowedToMerge: replaceBranchPermissions(existing.MergeAccessLevels, mergeAccessLevel), + AllowedToUnprotect: replaceBranchPermissions(existing.UnprotectAccessLevels, gitlab.MaintainerPermissions), + } + + _, _, err = c.ProtectedBranches.UpdateProtectedBranch(project.ID, branch, updateOpts) + if err != nil { + log.Debug().Err(err). + Str("name", project.Name). + Str("toURL", project.SSHURLToRepo). + Str("branch", branch). + Msg("cannot update protected branch") + return fmt.Errorf("error while trying to update protected branch %s: %w", branch, err) + } + + return nil + } + + if !isProtectedBranchNotFoundError(err) { log.Debug().Err(err). Str("name", project.Name). Str("toURL", project.SSHURLToRepo). Str("branch", branch). - Msg("cannot unprotect branch, but that is okay") + Msg("cannot read protected branch") + return fmt.Errorf("error while trying to read protected branch %s: %w", branch, err) } opts := &gitlab.ProtectRepositoryBranchesOptions{ @@ -166,6 +167,31 @@ func (c *Client) protectSingleBranch( return nil } +func replaceBranchPermissions(existing []*gitlab.BranchAccessDescription, accessLevel gitlab.AccessLevelValue) *[]*gitlab.BranchPermissionOptions { + destroy := true + permissions := make([]*gitlab.BranchPermissionOptions, 0, len(existing)+1) + for _, level := range existing { + if level == nil || level.ID <= 0 { + continue + } + + id := level.ID + permissions = append(permissions, &gitlab.BranchPermissionOptions{ID: &id, Destroy: &destroy}) + } + + permissions = append(permissions, &gitlab.BranchPermissionOptions{AccessLevel: gitlab.Ptr(accessLevel)}) + return &permissions +} + +func isProtectedBranchNotFoundError(err error) bool { + if err == nil { + return false + } + + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "404") || strings.Contains(msg, "not found") +} + func (c *Client) protectToBranchPerStudent(assignmentCfg *config.AssignmentConfig) { if len(assignmentCfg.Students) == 0 { fmt.Println("no students in config for assignment found") diff --git a/gitlab/protect_contract_test.go b/gitlab/protect_contract_test.go index b3e7a5e..c980c7c 100644 --- a/gitlab/protect_contract_test.go +++ b/gitlab/protect_contract_test.go @@ -20,13 +20,10 @@ func TestProtectToBranch_GroupNotFound_Exits(t *testing.T) { }) cfg := &config.AssignmentConfig{ - Course: "mpd", - Path: "mpd/ss26/blatt-01", - Per: config.PerStudent, - Startercode: &config.Startercode{ - ToBranch: "main", - ProtectToBranch: true, - }, + Course: "mpd", + Path: "mpd/ss26/blatt-01", + Per: config.PerStudent, + Branches: []config.BranchRule{{Name: "main", Protect: true}}, } assertExitCode(t, 1, func() { client.ProtectToBranch(cfg) }) } @@ -43,13 +40,10 @@ func TestProtectToBranch_InvalidPer_Exits(t *testing.T) { }) cfg := &config.AssignmentConfig{ - Course: "mpd", - Path: "mpd/ss26/blatt-01", - Per: config.PerFailed, - Startercode: &config.Startercode{ - ToBranch: "main", - ProtectToBranch: true, - }, + Course: "mpd", + Path: "mpd/ss26/blatt-01", + Per: config.PerFailed, + Branches: []config.BranchRule{{Name: "main", Protect: true}}, } assertExitCode(t, 1, func() { client.ProtectToBranch(cfg) }) } @@ -67,10 +61,7 @@ func TestProtectToBranchPerStudent_NoStudents(t *testing.T) { Path: "mpd/ss26/blatt-01", Per: config.PerStudent, Students: []*config.Student{}, - Startercode: &config.Startercode{ - ToBranch: "main", - ProtectToBranch: true, - }, + Branches: []config.BranchRule{{Name: "main", Protect: true}}, } client.protectToBranchPerStudent(cfg) } @@ -89,10 +80,7 @@ func TestProtectToBranchPerStudent_GetProjectFails(t *testing.T) { Per: config.PerStudent, UseCoursenameAsPrefix: true, Students: []*config.Student{{Username: &username, Raw: "alice"}}, - Startercode: &config.Startercode{ - ToBranch: "main", - ProtectToBranch: true, - }, + Branches: []config.BranchRule{{Name: "main", Protect: true}}, } // GetProject fails → prints error and returns client.protectToBranchPerStudent(cfg) @@ -105,8 +93,8 @@ func TestProtectToBranchPerStudent_Success(t *testing.T) { switch { case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "mpd-blatt01-alice"): _, _ = w.Write([]byte(pj)) - case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "protected_branches"): - w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): + w.WriteHeader(http.StatusNotFound) case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "protected_branches"): _, _ = w.Write([]byte(`{"id":1,"name":"main"}`)) default: @@ -122,10 +110,7 @@ func TestProtectToBranchPerStudent_Success(t *testing.T) { Per: config.PerStudent, UseCoursenameAsPrefix: true, Students: []*config.Student{{Username: &username, Raw: "alice"}}, - Startercode: &config.Startercode{ - ToBranch: "main", - ProtectToBranch: true, - }, + Branches: []config.BranchRule{{Name: "main", Protect: true}}, } client.protectToBranchPerStudent(cfg) } @@ -138,15 +123,12 @@ func TestProtectToBranchPerGroup_NoGroups(t *testing.T) { }) cfg := &config.AssignmentConfig{ - Course: "mpd", - Name: "blatt01", - Path: "mpd/ss26/blatt-01", - Per: config.PerGroup, - Groups: []*config.Group{}, - Startercode: &config.Startercode{ - ToBranch: "main", - ProtectToBranch: true, - }, + Course: "mpd", + Name: "blatt01", + Path: "mpd/ss26/blatt-01", + Per: config.PerGroup, + Groups: []*config.Group{}, + Branches: []config.BranchRule{{Name: "main", Protect: true}}, } client.protectToBranchPerGroup(cfg) } @@ -158,8 +140,8 @@ func TestProtectToBranchPerGroup_Success(t *testing.T) { switch { case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "mpd-blatt01-team1"): _, _ = w.Write([]byte(pj)) - case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "protected_branches"): - w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): + w.WriteHeader(http.StatusNotFound) case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "protected_branches"): _, _ = w.Write([]byte(`{"id":1,"name":"main"}`)) default: @@ -177,10 +159,7 @@ func TestProtectToBranchPerGroup_Success(t *testing.T) { Groups: []*config.Group{ {Name: "team1", Members: []*config.Student{{Username: &alice, Raw: "alice"}}}, }, - Startercode: &config.Startercode{ - ToBranch: "main", - ProtectToBranch: true, - }, + Branches: []config.BranchRule{{Name: "main", Protect: true}}, } client.protectToBranchPerGroup(cfg) } @@ -188,7 +167,7 @@ func TestProtectToBranchPerGroup_Success(t *testing.T) { // ---- protectBranch ---------------------------------------------------------- func TestProtectBranch_NoFlags_IsNoOp(t *testing.T) { - // Neither ProtectToBranch nor ProtectDevBranchMergeOnly → nothing happens + // No protected branches configured -> nothing happens called := false client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { called = true @@ -196,11 +175,7 @@ func TestProtectBranch_NoFlags_IsNoOp(t *testing.T) { }) cfg := &config.AssignmentConfig{ - Startercode: &config.Startercode{ - ToBranch: "main", - ProtectToBranch: false, - ProtectDevBranchMergeOnly: false, - }, + Branches: []config.BranchRule{{Name: "main", Protect: false, MergeOnly: false}}, } project := &gitlabapi.Project{ID: 1, Name: "myrepo"} err := client.protectBranch(cfg, project, false) @@ -215,8 +190,8 @@ func TestProtectBranch_NoFlags_IsNoOp(t *testing.T) { func TestProtectBranch_ProtectToBranch_Success(t *testing.T) { client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { switch { - case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "protected_branches"): - w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): + w.WriteHeader(http.StatusNotFound) case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "protected_branches"): _, _ = w.Write([]byte(`{"id":1,"name":"main"}`)) default: @@ -225,11 +200,7 @@ func TestProtectBranch_ProtectToBranch_Success(t *testing.T) { }) cfg := &config.AssignmentConfig{ - Startercode: &config.Startercode{ - ToBranch: "main", - DevBranch: "develop", - ProtectToBranch: true, - }, + Branches: []config.BranchRule{{Name: "main", Protect: true}, {Name: "develop"}}, } project := &gitlabapi.Project{ID: 1, Name: "myrepo"} err := client.protectBranch(cfg, project, false) @@ -241,8 +212,8 @@ func TestProtectBranch_ProtectToBranch_Success(t *testing.T) { func TestProtectBranch_ProtectDevBranchMergeOnly_Success(t *testing.T) { client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { switch { - case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "protected_branches"): - w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): + w.WriteHeader(http.StatusNotFound) case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "protected_branches"): _, _ = w.Write([]byte(`{"id":1,"name":"develop"}`)) default: @@ -251,11 +222,7 @@ func TestProtectBranch_ProtectDevBranchMergeOnly_Success(t *testing.T) { }) cfg := &config.AssignmentConfig{ - Startercode: &config.Startercode{ - ToBranch: "main", - DevBranch: "develop", - ProtectDevBranchMergeOnly: true, - }, + Branches: []config.BranchRule{{Name: "main"}, {Name: "develop", MergeOnly: true}}, } project := &gitlabapi.Project{ID: 1, Name: "myrepo"} err := client.protectBranch(cfg, project, false) @@ -265,11 +232,11 @@ func TestProtectBranch_ProtectDevBranchMergeOnly_Success(t *testing.T) { } func TestProtectBranch_BothSameBranch_Success(t *testing.T) { - // ProtectDevBranchMergeOnly=true AND DevBranch==ToBranch → single protectSingleBranch call + // If one branch is both protected and merge-only, merge-only semantics win. client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { switch { - case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "protected_branches"): - w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): + w.WriteHeader(http.StatusNotFound) case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "protected_branches"): _, _ = w.Write([]byte(`{"id":1,"name":"main"}`)) default: @@ -278,12 +245,7 @@ func TestProtectBranch_BothSameBranch_Success(t *testing.T) { }) cfg := &config.AssignmentConfig{ - Startercode: &config.Startercode{ - ToBranch: "main", - DevBranch: "main", // same as ToBranch - ProtectToBranch: true, - ProtectDevBranchMergeOnly: true, - }, + Branches: []config.BranchRule{{Name: "main", Protect: true, MergeOnly: true}}, } project := &gitlabapi.Project{ID: 1, Name: "myrepo"} err := client.protectBranch(cfg, project, false) @@ -297,9 +259,9 @@ func TestProtectBranch_BothSameBranch_Success(t *testing.T) { func TestProtectSingleBranch_Success(t *testing.T) { client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { switch { - case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "protected_branches"): - w.WriteHeader(http.StatusNoContent) - case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "protected_branches"): + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): + _, _ = w.Write([]byte(`{"id":1,"name":"main","push_access_levels":[{"id":10,"access_level":40}],"merge_access_levels":[{"id":11,"access_level":40}],"unprotect_access_levels":[{"id":12,"access_level":40}]}`)) + case r.Method == http.MethodPatch && strings.Contains(r.URL.Path, "protected_branches"): _, _ = w.Write([]byte(`{"id":1,"name":"main"}`)) default: w.WriteHeader(http.StatusNotFound) @@ -313,12 +275,12 @@ func TestProtectSingleBranch_Success(t *testing.T) { } } -func TestProtectSingleBranch_UnprotectFails_ProtectStillCalled(t *testing.T) { - // Unprotect returns 404 (branch not yet protected) → protectSingleBranch continues +func TestProtectSingleBranch_GetNotFound_ProtectStillCalled(t *testing.T) { + // Get returns 404 (branch not yet protected) -> protectSingleBranch creates the rule. protectCalled := false client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { switch { - case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "protected_branches"): + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): w.WriteHeader(http.StatusNotFound) // not protected yet case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "protected_branches"): protectCalled = true @@ -341,8 +303,8 @@ func TestProtectSingleBranch_UnprotectFails_ProtectStillCalled(t *testing.T) { func TestProtectSingleBranch_ProtectFails_ReturnsError(t *testing.T) { client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { switch { - case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "protected_branches"): - w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): + w.WriteHeader(http.StatusNotFound) case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "protected_branches"): w.WriteHeader(http.StatusForbidden) _, _ = w.Write([]byte(`{"message":"403 Forbidden"}`)) diff --git a/gitlab/runtime_test.go b/gitlab/runtime_test.go index 27af9a4..bd4bfb5 100644 --- a/gitlab/runtime_test.go +++ b/gitlab/runtime_test.go @@ -90,13 +90,11 @@ func TestProtectToBranch_UsesExitSeamForInvalidPer(t *testing.T) { }) cfg := &config.AssignmentConfig{ - Course: "mpd", - Path: "mpd/ss26/blatt-01", - URL: "https://gitlab.example.org/mpd/ss26/blatt-01", - Per: config.PerFailed, - Startercode: &config.Startercode{ - ToBranch: "main", - }, + Course: "mpd", + Path: "mpd/ss26/blatt-01", + URL: "https://gitlab.example.org/mpd/ss26/blatt-01", + Per: config.PerFailed, + Branches: []config.BranchRule{{Name: "main", Protect: true}}, } assertExitCode(t, 1, func() { diff --git a/gitlab/starterrepo.go b/gitlab/starterrepo.go index 7c78631..dded21e 100644 --- a/gitlab/starterrepo.go +++ b/gitlab/starterrepo.go @@ -37,7 +37,6 @@ func (c *Client) pushStartercode(assignmentCfg *cfg.AssignmentConfig, from *g.St Str("toURL", project.SSHURLToRepo). Str("fromBranch", assignmentCfg.Startercode.FromBranch). Str("toBranch", assignmentCfg.Startercode.ToBranch). - Str("devBranch", assignmentCfg.Startercode.DevBranch). Msg("pushing starter code") pushOpts := &git.PushOptions{ @@ -53,40 +52,20 @@ func (c *Client) pushStartercode(assignmentCfg *cfg.AssignmentConfig, from *g.St return fmt.Errorf("cannot push to remote: %w", err) } - if assignmentCfg.Startercode.DevBranch != assignmentCfg.Startercode.ToBranch { - if err := c.devBranch(assignmentCfg, project); err != nil { - log.Debug().Err(err). - Str("name", project.Name). - Msg("cannot set dev branch") - } - } - - if assignmentCfg.Startercode.ProtectToBranch || assignmentCfg.Startercode.ProtectDevBranchMergeOnly { - if err := c.protectBranch(assignmentCfg, project, false); err != nil { - log.Debug().Err(err). - Str("name", project.Name). - Msg("cannot protect to branch") - } - } - for _, additionalBranch := range assignmentCfg.Startercode.AdditionalBranches { - log.Debug().Str("branch", additionalBranch).Msg("pushing additional branch") - - // worktree, err := from.Repo.Worktree() - // if err != nil { - // log.Debug().Err(err). - // Str("branch", additionalBranch). - // Str("name", project.Name).Str("url", project.SSHURLToRepo). - // Msg("cannot get worktree") - // return fmt.Errorf("cannot get worktree: %w", err) - // } - - // worktree.Checkout(&git.CheckoutOptions{ - // Branch: plumbing.ReferenceName(additionalBranch), - // }) + if additionalBranch == "" { + continue + } refSpec := config.RefSpec(fmt.Sprintf("+refs/remotes/origin/%s:refs/heads/%s", additionalBranch, additionalBranch)) + log.Debug(). + Str("refSpec", string(refSpec)). + Str("name", project.Name). + Str("toURL", project.SSHURLToRepo). + Str("branch", additionalBranch). + Msg("pushing additional startercode branch") + pushOpts := &git.PushOptions{ RemoteName: remote.Config().Name, RefSpecs: []config.RefSpec{refSpec}, @@ -94,53 +73,14 @@ func (c *Client) pushStartercode(assignmentCfg *cfg.AssignmentConfig, from *g.St } err = from.Repo.Push(pushOpts) if err != nil { - log.Debug().Err(err). + log.Warn().Err(err). Str("branch", additionalBranch). - Str("refspec", refSpec.String()). - Str("name", project.Name).Str("url", project.SSHURLToRepo). - Msg("cannot push to remote") - return fmt.Errorf("cannot push to remote: %w", err) + Str("refSpec", refSpec.String()). + Str("name", project.Name). + Str("url", project.SSHURLToRepo). + Msg("cannot push additional branch to remote, continuing with other setup steps") + continue } - - } - - return nil -} - -func (c *Client) devBranch(assignmentCfg *cfg.AssignmentConfig, project *gitlab.Project) error { - log.Debug(). - Str("name", project.Name). - Str("toURL", project.SSHURLToRepo). - Str("branch", assignmentCfg.Startercode.DevBranch). - Msg("switching to development branch") - - opts := &gitlab.CreateBranchOptions{ - Branch: gitlab.Ptr(assignmentCfg.Startercode.DevBranch), - Ref: gitlab.Ptr(assignmentCfg.Startercode.ToBranch), - } - - _, _, err := c.Branches.CreateBranch(project.ID, opts) - if err != nil { - log.Debug().Err(err). - Str("name", project.Name). - Str("toURL", project.SSHURLToRepo). - Str("branch", assignmentCfg.Startercode.DevBranch). - Msg("error creating development branch") - return fmt.Errorf("error while trying to create development branch: %w", err) - } - - projectOpts := &gitlab.EditProjectOptions{ - DefaultBranch: gitlab.Ptr(assignmentCfg.Startercode.DevBranch), - } - - _, _, err = c.Projects.EditProject(project.ID, projectOpts) - if err != nil { - log.Debug().Err(err). - Str("name", project.Name). - Str("toURL", project.SSHURLToRepo). - Str("branch", assignmentCfg.Startercode.DevBranch). - Msg("error switching default to development branch") - return fmt.Errorf("error while switching default to development branch: %w", err) } return nil