diff --git a/config/repo.go b/config/repo.go index bd7c86e..db64e63 100644 --- a/config/repo.go +++ b/config/repo.go @@ -3,6 +3,7 @@ package config import ( "strings" + "github.com/go-viper/mapstructure/v2" "github.com/rs/zerolog/log" "github.com/spf13/viper" ) @@ -43,7 +44,16 @@ func startercode(assignmentKey string) *Startercode { func branches(assignmentKey string, starter *Startercode) []BranchRule { var configured []BranchRule - if err := viper.UnmarshalKey(assignmentKey+".branches", &configured); err != nil { + raw := normalizeBranchRuleConfigKeys(viper.Get(assignmentKey + ".branches")) + decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + Result: &configured, + TagName: "mapstructure", + WeaklyTypedInput: true, + }) + if err != nil { + log.Fatal().Err(err).Str("assignmentKey", assignmentKey).Msg("cannot create branches decoder") + } + if err := decoder.Decode(raw); err != nil { log.Fatal().Err(err).Str("assignmentKey", assignmentKey).Msg("cannot parse branches config") } @@ -58,6 +68,8 @@ func branches(assignmentKey string, starter *Startercode) []BranchRule { rules[idx].Protect = rules[idx].Protect || rule.Protect rules[idx].MergeOnly = rules[idx].MergeOnly || rule.MergeOnly rules[idx].Default = rules[idx].Default || rule.Default + rules[idx].AllowForcePush = rules[idx].AllowForcePush || rule.AllowForcePush + rules[idx].CodeOwnerApprovalRequired = rules[idx].CodeOwnerApprovalRequired || rule.CodeOwnerApprovalRequired return } seen[rule.Name] = len(rules) @@ -111,6 +123,37 @@ func branches(assignmentKey string, starter *Startercode) []BranchRule { return rules } +func normalizeBranchRuleConfigKeys(value any) any { + switch typed := value.(type) { + case []any: + normalized := make([]any, len(typed)) + for i, item := range typed { + normalized[i] = normalizeBranchRuleConfigKeys(item) + } + return normalized + case []map[string]any: + normalized := make([]any, len(typed)) + for i, item := range typed { + normalized[i] = normalizeBranchRuleConfigKeys(item) + } + return normalized + case map[string]any: + normalized := make(map[string]any, len(typed)) + for key, item := range typed { + switch key { + case "allow_force_push": + key = "allowForcePush" + case "code_owner_approval_required": + key = "codeOwnerApprovalRequired" + } + normalized[key] = normalizeBranchRuleConfigKeys(item) + } + return normalized + default: + return value + } +} + func defaultBranch(rules []BranchRule, fallback string) string { for _, rule := range rules { if rule.Default && rule.Name != "" { diff --git a/config/repo_test.go b/config/repo_test.go index dc747bc..e5999d1 100644 --- a/config/repo_test.go +++ b/config/repo_test.go @@ -66,22 +66,55 @@ func TestBranches_DefaultsAndLegacyFallback(t *testing.T) { 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}, + {"name": "main", "protect": true, "allowForcePush": true}, + {"name": "dev", "default": true, "mergeOnly": true, "codeOwnerApprovalRequired": 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 { + if b[0].Name != "main" || !b[0].Protect || !b[0].AllowForcePush { t.Fatalf("main branch = %#v", b[0]) } - if b[1].Name != "dev" || !b[1].Default || !b[1].MergeOnly { + if b[1].Name != "dev" || !b[1].Default || !b[1].MergeOnly || !b[1].CodeOwnerApprovalRequired { t.Fatalf("dev branch = %#v", b[1]) } } +func TestBranches_MergesAdditionalBranchFlags(t *testing.T) { + resetViper(t) + viper.Set("course.a1.branches", []map[string]any{ + {"name": "main", "protect": true}, + {"name": "main", "allowForcePush": true}, + {"name": "main", "codeOwnerApprovalRequired": true}, + }) + + b := branches("course.a1", nil) + if len(b) != 1 { + t.Fatalf("len(branches) = %d, want 1", len(b)) + } + if !b[0].Protect || !b[0].AllowForcePush || !b[0].CodeOwnerApprovalRequired { + t.Fatalf("merged branch = %#v", b[0]) + } +} + +func TestBranches_LegacySnakeCaseAdditionalBranchFlags(t *testing.T) { + resetViper(t) + viper.Set("course.a1.branches", []map[string]any{ + {"name": "main", "protect": true, "allow_force_push": true}, + {"name": "main", "code_owner_approval_required": true}, + }) + + b := branches("course.a1", nil) + if len(b) != 1 { + t.Fatalf("len(branches) = %d, want 1", len(b)) + } + if !b[0].Protect || !b[0].AllowForcePush || !b[0].CodeOwnerApprovalRequired { + t.Fatalf("legacy snake case branch = %#v", b[0]) + } +} + func TestIssues_DefaultsAndLegacyFallback(t *testing.T) { resetViper(t) viper.Set("course.a1.startercode.replicateIssue", true) diff --git a/config/show.go b/config/show.go index 0ff7c6c..741a82d 100644 --- a/config/show.go +++ b/config/show.go @@ -85,11 +85,13 @@ func (cfg *AssignmentConfig) Show() { for _, branch := range cfg.Branches { fmt.Fprintf( &out, - " - %-20v (%v=%-5v, %v=%-5v, %v=%-5v)\n", + " - %-20v (%v=%-5v, %v=%-5v, %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), + aurora.Cyan("allowForcePush"), aurora.Yellow(branch.AllowForcePush), + aurora.Cyan("codeOwnerApprovalRequired"), aurora.Yellow(branch.CodeOwnerApprovalRequired), ) } } diff --git a/config/types.go b/config/types.go index 6bd71a6..f017ebc 100644 --- a/config/types.go +++ b/config/types.go @@ -56,10 +56,12 @@ type Startercode struct { } type BranchRule struct { - Name string `mapstructure:"name"` - Protect bool `mapstructure:"protect"` - MergeOnly bool `mapstructure:"mergeOnly"` - Default bool `mapstructure:"default"` + Name string `mapstructure:"name"` + Protect bool `mapstructure:"protect"` + MergeOnly bool `mapstructure:"mergeOnly"` + Default bool `mapstructure:"default"` + AllowForcePush bool `mapstructure:"allowForcePush"` + CodeOwnerApprovalRequired bool `mapstructure:"codeOwnerApprovalRequired"` } type IssueReplication struct { diff --git a/config/urls_show_test.go b/config/urls_show_test.go index 66c422e..b681e06 100644 --- a/config/urls_show_test.go +++ b/config/urls_show_test.go @@ -138,13 +138,13 @@ func TestShow_WithStartercode_WithIssues(t *testing.T) { func TestShow_WithBranches(t *testing.T) { cfg := &AssignmentConfig{ - Branches: []BranchRule{{Name: "main", Protect: true, Default: true}, {Name: "develop", MergeOnly: true}}, + Branches: []BranchRule{{Name: "main", Protect: true, Default: true, AllowForcePush: true}, {Name: "develop", MergeOnly: true, CodeOwnerApprovalRequired: 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") { + if !strings.Contains(out, "mergeOnly") || !strings.Contains(out, "default") || !strings.Contains(out, "allowForcePush") || !strings.Contains(out, "codeOwnerApprovalRequired") { t.Fatalf("Show() output does not contain compact branch flags: %q", out) } } diff --git a/docs/configuration.md b/docs/configuration.md index 4a2e9d4..763bc8b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -206,10 +206,12 @@ Define branch creation, protection and default branch independently from starter branches: - name: main protect: true + allowForcePush: false default: false - name: develop mergeOnly: true + codeOwnerApprovalRequired: true default: true ``` @@ -221,6 +223,8 @@ branches: | `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 | +| `allowForcePush` | Allow force-push on protected branch | `false` | Passed through to GitLab protected branch settings | +| `codeOwnerApprovalRequired` | Require Code Owner approval | `false` | Passed through to GitLab protected branch settings | ### Branch protection notes diff --git a/gitlab/contract_test_helpers_test.go b/gitlab/contract_test_helpers_test.go index 024e041..286bd1e 100644 --- a/gitlab/contract_test_helpers_test.go +++ b/gitlab/contract_test_helpers_test.go @@ -14,7 +14,13 @@ func newContractClient(t *testing.T, handler http.HandlerFunc) *Client { server := httptest.NewServer(handler) t.Cleanup(server.Close) - apiClient, err := gitlabapi.NewClient("token", gitlabapi.WithBaseURL(server.URL+"/api/v4")) + // Use a plain http.Client without the retryable wrapper so that 5xx + // responses from mock servers do not trigger retries and cause test hangs. + apiClient, err := gitlabapi.NewClient("token", + gitlabapi.WithBaseURL(server.URL+"/api/v4"), + gitlabapi.WithHTTPClient(&http.Client{}), + gitlabapi.WithoutRetries(), + ) if err != nil { t.Fatalf("creating gitlab test client failed: %v", err) } diff --git a/gitlab/protect.go b/gitlab/protect.go index 6c6feeb..91522bd 100644 --- a/gitlab/protect.go +++ b/gitlab/protect.go @@ -78,7 +78,7 @@ func (c *Client) protectBranch(assignmentCfg *config.AssignmentConfig, project * mergeLevel = gitlab.DeveloperPermissions } - err := c.protectSingleBranch(project, branch.Name, pushLevel, mergeLevel) + err := c.protectSingleBranch(project, branch, pushLevel, mergeLevel) if err != nil { if spin { err := spinner.StopFail() @@ -113,26 +113,28 @@ func hasProtectedBranches(branches []config.BranchRule) bool { func (c *Client) protectSingleBranch( project *gitlab.Project, - branch string, + branch config.BranchRule, pushAccessLevel gitlab.AccessLevelValue, mergeAccessLevel gitlab.AccessLevelValue, ) error { - existing, _, err := c.ProtectedBranches.GetProtectedBranch(project.ID, branch) + existing, _, err := c.ProtectedBranches.GetProtectedBranch(project.ID, branch.Name) if err == nil { updateOpts := &gitlab.UpdateProtectedBranchOptions{ - AllowedToPush: replaceBranchPermissions(existing.PushAccessLevels, pushAccessLevel), - AllowedToMerge: replaceBranchPermissions(existing.MergeAccessLevels, mergeAccessLevel), - AllowedToUnprotect: replaceBranchPermissions(existing.UnprotectAccessLevels, gitlab.MaintainerPermissions), + AllowedToPush: replaceBranchPermissions(existing.PushAccessLevels, pushAccessLevel), + AllowedToMerge: replaceBranchPermissions(existing.MergeAccessLevels, mergeAccessLevel), + AllowedToUnprotect: replaceBranchPermissions(existing.UnprotectAccessLevels, gitlab.MaintainerPermissions), + AllowForcePush: gitlab.Ptr(branch.AllowForcePush), + CodeOwnerApprovalRequired: gitlab.Ptr(branch.CodeOwnerApprovalRequired), } - _, _, err = c.ProtectedBranches.UpdateProtectedBranch(project.ID, branch, updateOpts) + _, _, err = c.ProtectedBranches.UpdateProtectedBranch(project.ID, branch.Name, updateOpts) if err != nil { log.Debug().Err(err). Str("name", project.Name). Str("toURL", project.SSHURLToRepo). - Str("branch", branch). + Str("branch", branch.Name). Msg("cannot update protected branch") - return fmt.Errorf("error while trying to update protected branch %s: %w", branch, err) + return fmt.Errorf("error while trying to update protected branch %s: %w", branch.Name, err) } return nil @@ -142,16 +144,18 @@ func (c *Client) protectSingleBranch( log.Debug().Err(err). Str("name", project.Name). Str("toURL", project.SSHURLToRepo). - Str("branch", branch). + Str("branch", branch.Name). Msg("cannot read protected branch") - return fmt.Errorf("error while trying to read protected branch %s: %w", branch, err) + return fmt.Errorf("error while trying to read protected branch %s: %w", branch.Name, err) } opts := &gitlab.ProtectRepositoryBranchesOptions{ - Name: gitlab.Ptr(branch), - PushAccessLevel: gitlab.Ptr(pushAccessLevel), - MergeAccessLevel: gitlab.Ptr(mergeAccessLevel), - UnprotectAccessLevel: gitlab.Ptr(gitlab.MaintainerPermissions), + Name: gitlab.Ptr(branch.Name), + PushAccessLevel: gitlab.Ptr(pushAccessLevel), + MergeAccessLevel: gitlab.Ptr(mergeAccessLevel), + UnprotectAccessLevel: gitlab.Ptr(gitlab.MaintainerPermissions), + AllowForcePush: gitlab.Ptr(branch.AllowForcePush), + CodeOwnerApprovalRequired: gitlab.Ptr(branch.CodeOwnerApprovalRequired), } _, _, err = c.ProtectedBranches.ProtectRepositoryBranches(project.ID, opts) @@ -159,9 +163,9 @@ func (c *Client) protectSingleBranch( log.Debug().Err(err). Str("name", project.Name). Str("toURL", project.SSHURLToRepo). - Str("branch", branch). + Str("branch", branch.Name). Msg("error while protecting branch") - return fmt.Errorf("error while trying to protect branch %s: %w", branch, err) + return fmt.Errorf("error while trying to protect branch %s: %w", branch.Name, err) } return nil diff --git a/gitlab/protect_contract_test.go b/gitlab/protect_contract_test.go index c980c7c..2264736 100644 --- a/gitlab/protect_contract_test.go +++ b/gitlab/protect_contract_test.go @@ -1,6 +1,7 @@ package gitlab import ( + "encoding/json" "net/http" "strings" "testing" @@ -254,6 +255,97 @@ func TestProtectBranch_BothSameBranch_Success(t *testing.T) { } } +// TestProtectBranch_SendsAdditionalBranchProtectionFlags verifies that +// AllowForcePush and CodeOwnerApprovalRequired are forwarded to the GitLab API. +// The gitlab client-go library encodes POST/PATCH bodies as JSON +// (Content-Type: application/json), NOT as form-encoded values. +func TestProtectBranch_SendsAdditionalBranchProtectionFlags(t *testing.T) { + var postBody map[string]any + client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { + switch { + 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"): + ct := r.Header.Get("Content-Type") + if !strings.HasPrefix(ct, "application/json") { + t.Errorf("Content-Type = %q, want application/json", ct) + } + if err := json.NewDecoder(r.Body).Decode(&postBody); err != nil { + t.Fatalf("json.Decode() error = %v", err) + } + _, _ = w.Write([]byte(`{"id":1,"name":"main"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + }) + + cfg := &config.AssignmentConfig{ + Branches: []config.BranchRule{{ + Name: "main", + Protect: true, + AllowForcePush: true, + CodeOwnerApprovalRequired: true, + }}, + } + project := &gitlabapi.Project{ID: 1, Name: "myrepo"} + if err := client.protectBranch(cfg, project, false); err != nil { + t.Fatalf("protectBranch() error = %v", err) + } + if postBody["name"] != "main" { + t.Errorf("name = %#v, want \"main\"", postBody["name"]) + } + if postBody["allow_force_push"] != true { + t.Errorf("allow_force_push = %#v, want true", postBody["allow_force_push"]) + } + if postBody["code_owner_approval_required"] != true { + t.Errorf("code_owner_approval_required = %#v, want true", postBody["code_owner_approval_required"]) + } +} + +// TestProtectBranch_UpdateSendsAdditionalBranchProtectionFlags verifies that +// AllowForcePush and CodeOwnerApprovalRequired are also forwarded on the PATCH +// (update) path — i.e. when a protected-branch rule already exists. +func TestProtectBranch_UpdateSendsAdditionalBranchProtectionFlags(t *testing.T) { + var patchBody map[string]any + existingRule := `{"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}]}` + client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): + _, _ = w.Write([]byte(existingRule)) + case r.Method == http.MethodPatch && strings.Contains(r.URL.Path, "protected_branches"): + ct := r.Header.Get("Content-Type") + if !strings.HasPrefix(ct, "application/json") { + t.Errorf("Content-Type = %q, want application/json", ct) + } + if err := json.NewDecoder(r.Body).Decode(&patchBody); err != nil { + t.Fatalf("json.Decode() error = %v", err) + } + _, _ = w.Write([]byte(`{"id":1,"name":"main"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + }) + + cfg := &config.AssignmentConfig{ + Branches: []config.BranchRule{{ + Name: "main", + Protect: true, + AllowForcePush: true, + CodeOwnerApprovalRequired: true, + }}, + } + project := &gitlabapi.Project{ID: 1, Name: "myrepo"} + if err := client.protectBranch(cfg, project, false); err != nil { + t.Fatalf("protectBranch() (update path) error = %v", err) + } + if patchBody["allow_force_push"] != true { + t.Errorf("allow_force_push = %#v, want true", patchBody["allow_force_push"]) + } + if patchBody["code_owner_approval_required"] != true { + t.Errorf("code_owner_approval_required = %#v, want true", patchBody["code_owner_approval_required"]) + } +} + // ---- protectSingleBranch ---------------------------------------------------- func TestProtectSingleBranch_Success(t *testing.T) { @@ -269,7 +361,7 @@ func TestProtectSingleBranch_Success(t *testing.T) { }) project := &gitlabapi.Project{ID: 1, Name: "myrepo"} - err := client.protectSingleBranch(project, "main", gitlabapi.MaintainerPermissions, gitlabapi.MaintainerPermissions) + err := client.protectSingleBranch(project, config.BranchRule{Name: "main"}, gitlabapi.MaintainerPermissions, gitlabapi.MaintainerPermissions) if err != nil { t.Fatalf("protectSingleBranch() error = %v", err) } @@ -291,7 +383,7 @@ func TestProtectSingleBranch_GetNotFound_ProtectStillCalled(t *testing.T) { }) project := &gitlabapi.Project{ID: 1, Name: "myrepo"} - err := client.protectSingleBranch(project, "main", gitlabapi.NoPermissions, gitlabapi.DeveloperPermissions) + err := client.protectSingleBranch(project, config.BranchRule{Name: "main"}, gitlabapi.NoPermissions, gitlabapi.DeveloperPermissions) if err != nil { t.Fatalf("protectSingleBranch() error = %v", err) } @@ -314,7 +406,7 @@ func TestProtectSingleBranch_ProtectFails_ReturnsError(t *testing.T) { }) project := &gitlabapi.Project{ID: 1, Name: "myrepo"} - err := client.protectSingleBranch(project, "main", gitlabapi.MaintainerPermissions, gitlabapi.MaintainerPermissions) + err := client.protectSingleBranch(project, config.BranchRule{Name: "main"}, gitlabapi.MaintainerPermissions, gitlabapi.MaintainerPermissions) if err == nil { t.Fatal("protectSingleBranch() expected error on 403, got nil") }