diff --git a/pkg/config/prow-config-documented.yaml b/pkg/config/prow-config-documented.yaml index 7dae5e503..8ec02e916 100644 --- a/pkg/config/prow-config-documented.yaml +++ b/pkg/config/prow-config-documented.yaml @@ -1560,6 +1560,15 @@ tide: org: ' ' repos: - "" + # GitHubMergeBlocksPolicyMap configures on org or org/repo level how Tide should handle + # GitHub's mergeStateStatus (BLOCKED state from branch protection rules, rulesets, + # required reviews, etc.). + # Use '*' as key to set this globally. Defaults to "permit" which allows merging but logs warnings. + # Valid values: "ignore", "permit", "block" + # Note: "ignore" may cause issues with repos using GitHub Rulesets that restrict updates + # but have Tide on the bypass list. + github_merge_blocks_policy: + "": "" # A key/value pair of an org/repo as the key and Go template to override # the default merge commit title and/or message. Template is passed the # PullRequest struct (prow/github/types.go#PullRequest) diff --git a/pkg/config/tide.go b/pkg/config/tide.go index d65303167..cb6910fa6 100644 --- a/pkg/config/tide.go +++ b/pkg/config/tide.go @@ -39,6 +39,25 @@ import ( // TideQueries is a TideQuery slice. type TideQueries []TideQuery +// GitHubMergeBlocksPolicy describes how Tide should handle GitHub's merge blocking conditions. +type GitHubMergeBlocksPolicy string + +const ( + // GitHubMergeBlocksIgnore ignores GitHub's BLOCKED status entirely in merge decisions. + // Tide will attempt to merge PRs even if GitHub reports them as blocked. + GitHubMergeBlocksIgnore GitHubMergeBlocksPolicy = "ignore" + + // GitHubMergeBlocksPermit allows merging of BLOCKED PRs but logs warnings and may + // surface the blocked state in PR status messages. This is useful for monitoring + // repos that need branch protection or ruleset fixes. + GitHubMergeBlocksPermit GitHubMergeBlocksPolicy = "permit" + + // GitHubMergeBlocksBlock respects GitHub's BLOCKED status and prevents Tide from + // attempting to merge PRs that are blocked by branch protection rules, rulesets, + // required reviews, or other GitHub-side blocking conditions. + GitHubMergeBlocksBlock GitHubMergeBlocksPolicy = "block" +) + type TideBranchMergeType struct { MergeType types.PullRequestMergeType Regexpr *regexp.Regexp @@ -227,6 +246,14 @@ type Tide struct { // starting a new one requires to start new instances of all tests. // Use '*' as key to set this globally. Defaults to true. PrioritizeExistingBatchesMap map[string]bool `json:"prioritize_existing_batches,omitempty"` + // GitHubMergeBlocksPolicyMap configures on org or org/repo level how Tide should handle + // GitHub's mergeStateStatus (BLOCKED state from branch protection rules, rulesets, + // required reviews, etc.). + // Use '*' as key to set this globally. Defaults to "permit" which allows merging but logs warnings. + // Valid values: "ignore", "permit", "block" + // Note: "ignore" may cause issues with repos using GitHub Rulesets that restrict updates + // but have Tide on the bypass list. + GitHubMergeBlocksPolicyMap map[string]GitHubMergeBlocksPolicy `json:"github_merge_blocks_policy,omitempty"` TideGitHubConfig `json:",inline"` } @@ -370,6 +397,22 @@ func (t *Tide) BatchSizeLimit(repo OrgRepo) int { return t.BatchSizeLimitMap["*"] } +// GitHubMergeBlocksPolicy returns the policy for handling GitHub's merge blocking conditions. +// The default is "permit" which allows merging but logs warnings for monitoring. +func (t *Tide) GitHubMergeBlocksPolicy(repo OrgRepo) GitHubMergeBlocksPolicy { + if val, set := t.GitHubMergeBlocksPolicyMap[repo.String()]; set { + return val + } + if val, set := t.GitHubMergeBlocksPolicyMap[repo.Org]; set { + return val + } + if val, set := t.GitHubMergeBlocksPolicyMap["*"]; set { + return val + } + // Default to "permit" to allow merging while surfacing the blocked state for monitoring + return GitHubMergeBlocksPermit +} + // MergeMethod returns the merge method to use for a repo. The default of merge is // returned when not overridden. func (t *Tide) MergeMethod(repo OrgRepo) types.PullRequestMergeType { diff --git a/pkg/tide/github.go b/pkg/tide/github.go index f5a42997b..3f93c5a7d 100644 --- a/pkg/tide/github.go +++ b/pkg/tide/github.go @@ -632,6 +632,20 @@ func (m *mergeChecker) isAllowedToMerge(crc *CodeReviewCommon) (string, error) { } else if !allowed { return fmt.Sprintf("Merge type %q disallowed by repo settings", *mergeMethod), nil } + // Check GitHub's mergeStateStatus which reflects all GitHub-side merge blocking conditions + // including branch protection rules, rulesets, required reviews, status checks, etc. + // This is controlled by the github_merge_blocks_policy configuration. + if pr.MergeStateStatus == "BLOCKED" { + policy := m.config().Tide.GitHubMergeBlocksPolicy(orgRepo) + switch policy { + case config.GitHubMergeBlocksBlock: + return "PR is blocked from merging by GitHub (check branch protection, required reviews, or rulesets)", nil + case config.GitHubMergeBlocksPermit: + // Allow merge but the warning will be surfaced in PR status by requirementDiff + case config.GitHubMergeBlocksIgnore: + // Ignore BLOCKED status entirely + } + } return "", nil } diff --git a/pkg/tide/status.go b/pkg/tide/status.go index 17901811e..d74afc023 100644 --- a/pkg/tide/status.go +++ b/pkg/tide/status.go @@ -45,8 +45,9 @@ import ( ) const ( - statusContext = "tide" - statusInPool = "In merge pool." + statusContext = "tide" + statusInPool = "In merge pool." + statusInPoolDespiteBlocked = "In merge pool (despite BLOCKED)." // statusNotInPool is a format string used when a PR is not in a tide pool. // The '%s' field is populated with the reason why the PR is not in a // tide pool or the empty string if the reason is unknown. See requirementDiff. @@ -125,7 +126,7 @@ func (sc *statusController) shutdown() { // Note: an empty diff can be returned if the reason that the PR does not match // the TideQuery is unknown. This can happen if this function's logic // does not match GitHub's and does not indicate that the PR matches the query. -func requirementDiff(pr *PullRequest, q *config.TideQuery, cc contextChecker) (string, int) { +func requirementDiff(pr *PullRequest, q *config.TideQuery, cc contextChecker, tide *config.Tide) (string, int) { const maxLabelChars = 50 var desc string var diff int @@ -260,6 +261,27 @@ func requirementDiff(pr *PullRequest, q *config.TideQuery, cc contextChecker) (s desc = " PullRequest is missing sufficient approving GitHub review(s)" } } + // Check GitHub's mergeStateStatus which reflects all GitHub-side blocking conditions + // This is controlled by the github_merge_blocks_policy configuration. + if tide != nil && pr.MergeStateStatus == "BLOCKED" { + orgRepo := config.OrgRepo{ + Org: string(pr.Repository.Owner.Login), + Repo: string(pr.Repository.Name), + } + policy := tide.GitHubMergeBlocksPolicy(orgRepo) + switch policy { + case config.GitHubMergeBlocksBlock: + // Block merge by adding to diff + diff += 100 + if desc == "" { + desc = " Blocked by GitHub (branch rulesets or protection)" + } + case config.GitHubMergeBlocksPermit: + // Allow merge but don't add to diff. Warning will be shown in pool status. + case config.GitHubMergeBlocksIgnore: + // Ignore BLOCKED status entirely + } + } return desc, diff } @@ -315,7 +337,7 @@ func (sc *statusController) expectedStatus(log *logrus.Entry, queryMap *config.Q minDiffCount := -1 var minDiff string for _, q := range queryMap.ForRepo(repo) { - diff, diffCount := requirementDiff(pr, &q, cc) + diff, diffCount := requirementDiff(pr, &q, cc, &sc.config().Tide) if diffCount == 0 { hasFulfilledQuery = true break @@ -347,7 +369,7 @@ func (sc *statusController) expectedStatus(log *logrus.Entry, queryMap *config.Q if err := sc.pjClient.List(context.Background(), passingUpToDatePJs, ctrlruntimeclient.MatchingFields{indexNamePassingJobs: indexKey}); err != nil { // Just log the error and return success, as the PR is in the merge pool log.WithError(err).Error("Failed to list ProwJobs.") - return github.StatusSuccess, statusInPool, nil + return github.StatusSuccess, poolStatus(pr, &sc.config().Tide, log), nil } var passingUpToDateContexts []string @@ -357,7 +379,30 @@ func (sc *statusController) expectedStatus(log *logrus.Entry, queryMap *config.Q if diff := cc.MissingRequiredContexts(passingUpToDateContexts); len(diff) > 0 { return github.StatePending, retestingStatus(diff), nil } - return github.StatusSuccess, statusInPool, nil + return github.StatusSuccess, poolStatus(pr, &sc.config().Tide, log), nil +} + +// poolStatus returns the appropriate status message for a PR that is in the merge pool. +// If the PR has BLOCKED merge state and the policy is "permit", it returns a warning message. +// This also logs a warning for monitoring purposes. +func poolStatus(pr *PullRequest, tide *config.Tide, log *logrus.Entry) string { + if pr.MergeStateStatus == "BLOCKED" && tide != nil { + repo := config.OrgRepo{ + Org: string(pr.Repository.Owner.Login), + Repo: string(pr.Repository.Name), + } + if tide.GitHubMergeBlocksPolicy(repo) == config.GitHubMergeBlocksPermit { + log.WithFields(logrus.Fields{ + "org": repo.Org, + "repo": repo.Repo, + "pr": pr.Number, + "merge_state": pr.MergeStateStatus, + "policy": config.GitHubMergeBlocksPermit, + }).Warning("PR is in merge pool despite GitHub BLOCKED status (policy: permit)") + return statusInPoolDespiteBlocked + } + } + return statusInPool } func retestingStatus(retested []string) string { diff --git a/pkg/tide/status_test.go b/pkg/tide/status_test.go index 082d2f03e..93c47d35b 100644 --- a/pkg/tide/status_test.go +++ b/pkg/tide/status_test.go @@ -870,6 +870,837 @@ func TestExpectedStatus(t *testing.T) { } } +func TestRequirementDiff(t *testing.T) { + testCases := []struct { + name string + prLabels []string + prAuthor string + prMilestone *Milestone + prBaseBranch string + prContexts []Context + prCheckRuns []CheckRun + reviewDecision githubql.PullRequestReviewDecision + mergeStateStatus string + queryLabels []string + queryForbiddenLabels []string + queryAuthor string + queryMilestone string + queryExcludedBranches []string + queryIncludedBranches []string + reviewApprovedRequired bool + expectedDiff int + expectedDescContains string + }{ + // MergeStateStatus tests + { + name: "Blocked merge state with changes requested", + reviewDecision: githubql.PullRequestReviewDecisionChangesRequested, + mergeStateStatus: "BLOCKED", + reviewApprovedRequired: false, + expectedDiff: 100, + expectedDescContains: "Blocked by GitHub (branch rulesets or protection)", + }, + { + name: "Blocked merge state with specific review decision, review required", + reviewDecision: githubql.PullRequestReviewDecisionChangesRequested, + mergeStateStatus: "BLOCKED", + reviewApprovedRequired: true, + expectedDiff: 150, + expectedDescContains: "PullRequest is missing sufficient approving GitHub review(s)", + }, + { + name: "Blocked merge state without specific review decision", + mergeStateStatus: "BLOCKED", + reviewApprovedRequired: false, + expectedDiff: 100, + expectedDescContains: "Blocked by GitHub (branch rulesets or protection)", + }, + { + name: "Clean merge state allows merge", + mergeStateStatus: "CLEAN", + reviewApprovedRequired: false, + expectedDiff: 0, + expectedDescContains: "", + }, + { + name: "Missing approval when required", + reviewDecision: "", + reviewApprovedRequired: true, + expectedDiff: 50, + expectedDescContains: "missing sufficient approving", + }, + { + name: "Approved review passes", + reviewDecision: githubql.PullRequestReviewDecisionApproved, + reviewApprovedRequired: true, + expectedDiff: 0, + expectedDescContains: "", + }, + { + name: "No review decision without requirement passes", + reviewDecision: "", + reviewApprovedRequired: false, + expectedDiff: 0, + expectedDescContains: "", + }, + // Branch filtering tests + { + name: "Branch in excluded list", + prBaseBranch: "release-1.0", + queryExcludedBranches: []string{"release-1.0", "release-2.0"}, + expectedDiff: 2000, + expectedDescContains: "Merging to branch release-1.0 is forbidden", + }, + { + name: "Branch not in excluded list", + prBaseBranch: "main", + queryExcludedBranches: []string{"release-1.0", "release-2.0"}, + expectedDiff: 0, + expectedDescContains: "", + }, + { + name: "Branch not in included list", + prBaseBranch: "feature-branch", + queryIncludedBranches: []string{"main", "develop"}, + expectedDiff: 2000, + expectedDescContains: "Merging to branch feature-branch is forbidden", + }, + { + name: "Branch in included list", + prBaseBranch: "main", + queryIncludedBranches: []string{"main", "develop"}, + expectedDiff: 0, + expectedDescContains: "", + }, + // Author tests + { + name: "Matching author", + prAuthor: "alice", + queryAuthor: "alice", + expectedDiff: 0, + expectedDescContains: "", + }, + { + name: "Non-matching author", + prAuthor: "bob", + queryAuthor: "alice", + expectedDiff: 1000, + expectedDescContains: "Must be by author alice", + }, + { + name: "No author requirement", + prAuthor: "anyone", + queryAuthor: "", + expectedDiff: 0, + expectedDescContains: "", + }, + // Milestone tests + { + name: "Matching milestone", + prMilestone: &Milestone{Title: githubql.String("v1.0")}, + queryMilestone: "v1.0", + expectedDiff: 0, + expectedDescContains: "", + }, + { + name: "Non-matching milestone", + prMilestone: &Milestone{Title: githubql.String("v1.1")}, + queryMilestone: "v1.0", + expectedDiff: 100, + expectedDescContains: "Must be in milestone v1.0", + }, + { + name: "PR has no milestone but query requires one", + prMilestone: nil, + queryMilestone: "v1.0", + expectedDiff: 100, + expectedDescContains: "Must be in milestone v1.0", + }, + { + name: "No milestone requirement", + prMilestone: &Milestone{Title: githubql.String("v1.0")}, + queryMilestone: "", + expectedDiff: 0, + expectedDescContains: "", + }, + // Label tests + { + name: "All required labels present", + prLabels: []string{"lgtm", "approved"}, + queryLabels: []string{"lgtm", "approved"}, + expectedDiff: 0, + expectedDescContains: "", + }, + { + name: "Missing one required label", + prLabels: []string{"lgtm"}, + queryLabels: []string{"lgtm", "approved"}, + expectedDiff: 1, + expectedDescContains: "Needs approved label", + }, + { + name: "Missing multiple required labels", + prLabels: []string{}, + queryLabels: []string{"lgtm", "approved"}, + expectedDiff: 2, + expectedDescContains: "Needs approved, lgtm labels", + }, + { + name: "Has forbidden label", + prLabels: []string{"lgtm", "do-not-merge"}, + queryForbiddenLabels: []string{"do-not-merge"}, + expectedDiff: 1, + expectedDescContains: "Should not have do-not-merge label", + }, + { + name: "Has multiple forbidden labels", + prLabels: []string{"lgtm", "do-not-merge", "hold"}, + queryForbiddenLabels: []string{"do-not-merge", "hold"}, + expectedDiff: 2, + expectedDescContains: "Should not have do-not-merge, hold labels", + }, + { + name: "Alternative labels (one of several)", + prLabels: []string{"approved"}, + queryLabels: []string{"lgtm,approved"}, + expectedDiff: 0, + expectedDescContains: "", + }, + { + name: "Alternative labels with none present", + prLabels: []string{}, + queryLabels: []string{"lgtm,approved"}, + expectedDiff: 1, + expectedDescContains: "Needs lgtm or approved label", + }, + // Context tests + { + name: "All contexts successful", + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateSuccess}, + {Context: githubql.String("ci/build"), State: githubql.StatusStateSuccess}, + }, + expectedDiff: 0, + expectedDescContains: "", + }, + { + name: "One failed context", + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateFailure}, + }, + expectedDiff: 1, + expectedDescContains: "Job ci/test has not succeeded", + }, + { + name: "Multiple failed contexts", + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateFailure}, + {Context: githubql.String("ci/build"), State: githubql.StatusStateError}, + }, + expectedDiff: 2, + expectedDescContains: "Jobs ci/build, ci/test have not succeeded", + }, + { + name: "Pending context counts as failed", + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStatePending}, + }, + expectedDiff: 1, + expectedDescContains: "Job ci/test has not succeeded", + }, + { + name: "Successful checkrun", + prCheckRuns: []CheckRun{ + {Name: githubql.String("test-job"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateSuccess)}, + }, + expectedDiff: 0, + expectedDescContains: "", + }, + { + name: "Failed checkrun", + prCheckRuns: []CheckRun{ + {Name: githubql.String("test-job"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateFailure)}, + }, + expectedDiff: 1, + expectedDescContains: "Job test-job has not succeeded", + }, + // Priority ordering test: branch takes precedence + { + name: "Branch mismatch takes precedence over other issues", + prBaseBranch: "forbidden-branch", + queryExcludedBranches: []string{"forbidden-branch"}, + prAuthor: "wrong-author", + queryAuthor: "correct-author", + prLabels: []string{}, + queryLabels: []string{"lgtm"}, + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateFailure}, + }, + expectedDiff: 3002, // 2000 (branch) + 1000 (author) + 1 (label) + 1 (context) + expectedDescContains: "Merging to branch forbidden-branch is forbidden", + }, + // Author takes precedence over labels and contexts + { + name: "Author mismatch takes precedence over labels and contexts", + prAuthor: "wrong-author", + queryAuthor: "correct-author", + prLabels: []string{}, + queryLabels: []string{"lgtm"}, + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateFailure}, + }, + expectedDiff: 1002, // 1000 (author) + 1 (label) + 1 (context) + expectedDescContains: "Must be by author correct-author", + }, + // Milestone takes precedence over labels and contexts + { + name: "Milestone mismatch takes precedence over labels and contexts", + prMilestone: &Milestone{Title: githubql.String("v1.1")}, + queryMilestone: "v1.0", + prLabels: []string{}, + queryLabels: []string{"lgtm"}, + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateFailure}, + }, + expectedDiff: 102, // 100 (milestone) + 1 (label) + 1 (context) + expectedDescContains: "Must be in milestone v1.0", + }, + // Labels take precedence over contexts + { + name: "Missing labels take precedence over failed contexts", + prLabels: []string{}, + queryLabels: []string{"lgtm"}, + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateFailure}, + }, + expectedDiff: 2, // 1 (label) + 1 (context) + expectedDescContains: "Needs lgtm label", + }, + // Forbidden labels take precedence over contexts + { + name: "Forbidden labels take precedence over failed contexts", + prLabels: []string{"do-not-merge"}, + queryForbiddenLabels: []string{"do-not-merge"}, + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateFailure}, + }, + expectedDiff: 2, // 1 (forbidden label) + 1 (context) + expectedDescContains: "Should not have do-not-merge label", + }, + // All aspects satisfied - perfect PR + { + name: "Complex: All requirements satisfied across all areas", + prBaseBranch: "main", + queryIncludedBranches: []string{"main", "develop"}, + prAuthor: "approved-contributor", + queryAuthor: "approved-contributor", + prMilestone: &Milestone{Title: githubql.String("v1.0")}, + queryMilestone: "v1.0", + prLabels: []string{"lgtm", "approved", "size/small"}, + queryLabels: []string{"lgtm", "approved"}, + queryForbiddenLabels: []string{"do-not-merge", "hold"}, + reviewDecision: githubql.PullRequestReviewDecisionApproved, + reviewApprovedRequired: true, + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateSuccess}, + {Context: githubql.String("ci/lint"), State: githubql.StatusStateSuccess}, + }, + prCheckRuns: []CheckRun{ + {Name: githubql.String("security-scan"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateSuccess)}, + {Name: githubql.String("e2e-tests"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateSuccess)}, + }, + expectedDiff: 0, + expectedDescContains: "", + }, + // All aspects with branch issue + { + name: "Complex: Multiple issues across all areas with branch taking precedence", + prBaseBranch: "release-1.0", + queryExcludedBranches: []string{"release-1.0"}, + prAuthor: "unauthorized-user", + queryAuthor: "authorized-user", + prMilestone: &Milestone{Title: githubql.String("v2.0")}, + queryMilestone: "v1.0", + prLabels: []string{"do-not-merge"}, + queryLabels: []string{"lgtm", "approved"}, + queryForbiddenLabels: []string{"do-not-merge"}, + reviewDecision: githubql.PullRequestReviewDecisionChangesRequested, + reviewApprovedRequired: true, + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateFailure}, + {Context: githubql.String("ci/lint"), State: githubql.StatusStateError}, + }, + prCheckRuns: []CheckRun{ + {Name: githubql.String("security-scan"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateFailure)}, + }, + mergeStateStatus: "BLOCKED", + // 2000 (branch) + 1000 (author) + 100 (milestone) + 2 (missing labels) + 1 (forbidden label) + 100 (blocked) + 50 (review) + 3 (contexts: 2 status + 1 checkrun) + expectedDiff: 3256, + expectedDescContains: "Merging to branch release-1.0 is forbidden", + }, + // All aspects with author issue + { + name: "Complex: Multiple issues across all areas with author taking precedence", + prBaseBranch: "main", + queryIncludedBranches: []string{"main", "develop"}, + prAuthor: "external-contributor", + queryAuthor: "core-team", + prMilestone: nil, + queryMilestone: "v1.5", + prLabels: []string{"help-wanted", "needs-rebase", "wip"}, + queryLabels: []string{"lgtm", "approved", "size/small"}, + queryForbiddenLabels: []string{"wip", "needs-rebase"}, + reviewDecision: githubql.PullRequestReviewDecisionApproved, + reviewApprovedRequired: true, + prContexts: []Context{ + {Context: githubql.String("ci/build"), State: githubql.StatusStatePending}, + {Context: githubql.String("ci/unit-tests"), State: githubql.StatusStateSuccess}, + }, + prCheckRuns: []CheckRun{ + {Name: githubql.String("integration-tests"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateSuccess)}, + {Name: githubql.String("e2e-tests"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateFailure)}, + }, + mergeStateStatus: "CLEAN", + // 0 (branch OK) + 1000 (author) + 100 (milestone) + 3 (missing labels) + 2 (forbidden labels) + 0 (not blocked) + 2 (failed contexts: ci/build pending + e2e-tests failed) + expectedDiff: 1107, + expectedDescContains: "Must be by author core-team", + }, + // All aspects with milestone issue + { + name: "Complex: Branch and author OK, other issues present with milestone precedence", + prBaseBranch: "develop", + queryIncludedBranches: []string{"main", "develop"}, + prAuthor: "approved-dev", + queryAuthor: "approved-dev", + prMilestone: &Milestone{Title: githubql.String("backlog")}, + queryMilestone: "sprint-23", + prLabels: []string{"lgtm", "hold"}, + queryLabels: []string{"lgtm", "approved"}, + queryForbiddenLabels: []string{"hold", "blocked"}, + reviewDecision: githubql.PullRequestReviewDecisionChangesRequested, + reviewApprovedRequired: false, + prContexts: []Context{ + {Context: githubql.String("ci/verify"), State: githubql.StatusStateSuccess}, + }, + prCheckRuns: []CheckRun{ + {Name: githubql.String("code-coverage"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateFailure)}, + }, + mergeStateStatus: "BLOCKED", + // 0 (branch OK) + 0 (author OK) + 100 (milestone) + 1 (missing approved label) + 1 (forbidden hold label) + 100 (blocked) + 1 (failed checkrun) + expectedDiff: 203, + expectedDescContains: "Must be in milestone sprint-23", + }, + // All aspects with changes requested issue + { + name: "Complex: All requirements met but changes requested blocks merge", + prBaseBranch: "main", + queryIncludedBranches: []string{"main", "develop"}, + prAuthor: "core-maintainer", + queryAuthor: "core-maintainer", + prMilestone: &Milestone{Title: githubql.String("v2.0")}, + queryMilestone: "v2.0", + prLabels: []string{"lgtm", "approved", "ready-to-merge"}, + queryLabels: []string{"lgtm", "approved"}, + queryForbiddenLabels: []string{"do-not-merge", "hold"}, + reviewDecision: githubql.PullRequestReviewDecisionChangesRequested, + reviewApprovedRequired: false, + prContexts: []Context{ + {Context: githubql.String("ci/test"), State: githubql.StatusStateSuccess}, + {Context: githubql.String("ci/lint"), State: githubql.StatusStateSuccess}, + {Context: githubql.String("ci/build"), State: githubql.StatusStateSuccess}, + }, + prCheckRuns: []CheckRun{ + {Name: githubql.String("security-scan"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateSuccess)}, + {Name: githubql.String("e2e-tests"), Status: githubql.String(githubql.CheckStatusStateCompleted), Conclusion: githubql.String(githubql.StatusStateSuccess)}, + }, + mergeStateStatus: "BLOCKED", + // 0 (branch OK) + 0 (author OK) + 0 (milestone OK) + 0 (labels OK) + 100 (blocked) + 50(review) + 0 (contexts OK) + expectedDiff: 100, + expectedDescContains: "Blocked by GitHub (branch rulesets or protection)", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pr := &PullRequest{ + ReviewDecision: tc.reviewDecision, + MergeStateStatus: githubql.String(tc.mergeStateStatus), + HeadRefOID: githubql.String("abc123"), + } + + if tc.prBaseBranch != "" { + pr.BaseRef = struct { + Name githubql.String + Prefix githubql.String + }{ + Name: githubql.String(tc.prBaseBranch), + } + } + + if tc.prAuthor != "" { + pr.Author = struct { + Login githubql.String + }{ + Login: githubql.String(tc.prAuthor), + } + } + + pr.Milestone = tc.prMilestone + + for _, label := range tc.prLabels { + pr.Labels.Nodes = append(pr.Labels.Nodes, struct{ Name githubql.String }{Name: githubql.String(label)}) + } + + if len(tc.prContexts) > 0 || len(tc.prCheckRuns) > 0 { + var checkRunNodes []CheckRunNode + for _, cr := range tc.prCheckRuns { + checkRunNodes = append(checkRunNodes, CheckRunNode{CheckRun: cr}) + } + pr.Commits.Nodes = append(pr.Commits.Nodes, struct{ Commit Commit }{ + Commit: Commit{ + OID: githubql.String("abc123"), + Status: struct{ Contexts []Context }{ + Contexts: tc.prContexts, + }, + StatusCheckRollup: StatusCheckRollup{ + Contexts: StatusCheckRollupContext{ + Nodes: checkRunNodes, + }, + }, + }, + }) + } + + query := &config.TideQuery{ + Labels: tc.queryLabels, + MissingLabels: tc.queryForbiddenLabels, + Author: tc.queryAuthor, + Milestone: tc.queryMilestone, + ExcludedBranches: tc.queryExcludedBranches, + IncludedBranches: tc.queryIncludedBranches, + ReviewApprovedRequired: tc.reviewApprovedRequired, + } + + // Set repository info for the PR (needed for EnforceGitHubMergeBlocks check) + pr.Repository = struct { + Name githubql.String + NameWithOwner githubql.String + Owner struct { + Login githubql.String + } + }{ + Name: githubql.String("test-repo"), + Owner: struct { + Login githubql.String + }{ + Login: githubql.String("test-org"), + }, + } + + cc := &config.TideContextPolicy{} + + // Create tide config with github_merge_blocks_policy set to "block" globally + // This ensures the existing test behavior is preserved + tide := &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksBlock, + }, + } + + desc, diff := requirementDiff(pr, query, cc, tide) + + if diff != tc.expectedDiff { + t.Errorf("Expected diff %d, but got %d", tc.expectedDiff, diff) + } + + if tc.expectedDescContains != "" { + if !strings.Contains(desc, tc.expectedDescContains) { + t.Errorf("Expected description to contain %q, but got %q", tc.expectedDescContains, desc) + } + } else if desc != "" { + t.Errorf("Expected empty description, but got %q", desc) + } + }) + } +} + +func TestGitHubMergeBlocksPolicy(t *testing.T) { + testCases := []struct { + name string + org string + repo string + mergeStateStatus string + tideConfig *config.Tide + expectedDiff int + expectedDescContains string + expectedDescNotContain string + }{ + { + name: "Block policy globally - BLOCKED state should add diff", + org: "test-org", + repo: "test-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksBlock, + }, + }, + expectedDiff: 100, + expectedDescContains: "Blocked by GitHub", + }, + { + name: "Permit policy globally - BLOCKED state should not add diff", + org: "test-org", + repo: "test-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksPermit, + }, + }, + expectedDiff: 0, + expectedDescNotContain: "Blocked by GitHub", + }, + { + name: "Ignore policy globally - BLOCKED state should not add diff", + org: "test-org", + repo: "test-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksIgnore, + }, + }, + expectedDiff: 0, + expectedDescNotContain: "Blocked by GitHub", + }, + { + name: "Policy not configured (default to permit) - BLOCKED state should not add diff", + org: "test-org", + repo: "test-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{}, + expectedDiff: 0, + }, + { + name: "Block policy for specific org - BLOCKED state should add diff", + org: "enforced-org", + repo: "test-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "enforced-org": config.GitHubMergeBlocksBlock, + }, + }, + expectedDiff: 100, + expectedDescContains: "Blocked by GitHub", + }, + { + name: "Block policy for specific org - different org should use default (permit)", + org: "other-org", + repo: "test-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "enforced-org": config.GitHubMergeBlocksBlock, + }, + }, + expectedDiff: 0, + }, + { + name: "Block policy for specific repo - BLOCKED state should add diff", + org: "test-org", + repo: "enforced-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "test-org/enforced-repo": config.GitHubMergeBlocksBlock, + }, + }, + expectedDiff: 100, + expectedDescContains: "Blocked by GitHub", + }, + { + name: "Block policy for specific repo - different repo should use default (permit)", + org: "test-org", + repo: "other-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "test-org/enforced-repo": config.GitHubMergeBlocksBlock, + }, + }, + expectedDiff: 0, + }, + { + name: "Repo config overrides org config - repo uses ignore", + org: "test-org", + repo: "special-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "test-org": config.GitHubMergeBlocksBlock, + "test-org/special-repo": config.GitHubMergeBlocksIgnore, + }, + }, + expectedDiff: 0, + }, + { + name: "Repo config overrides org config - repo uses block", + org: "test-org", + repo: "special-repo", + mergeStateStatus: "BLOCKED", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "test-org": config.GitHubMergeBlocksPermit, + "test-org/special-repo": config.GitHubMergeBlocksBlock, + }, + }, + expectedDiff: 100, + expectedDescContains: "Blocked by GitHub", + }, + { + name: "CLEAN merge state - block policy should not add diff", + org: "test-org", + repo: "test-repo", + mergeStateStatus: "CLEAN", + tideConfig: &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksBlock, + }, + }, + expectedDiff: 0, + }, + { + name: "Nil tide config - should not panic", + org: "test-org", + repo: "test-repo", + mergeStateStatus: "BLOCKED", + tideConfig: nil, + expectedDiff: 0, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pr := &PullRequest{ + MergeStateStatus: githubql.String(tc.mergeStateStatus), + HeadRefOID: githubql.String("abc123"), + Repository: struct { + Name githubql.String + NameWithOwner githubql.String + Owner struct { + Login githubql.String + } + }{ + Name: githubql.String(tc.repo), + Owner: struct { + Login githubql.String + }{ + Login: githubql.String(tc.org), + }, + }, + } + + query := &config.TideQuery{} + cc := &config.TideContextPolicy{} + + desc, diff := requirementDiff(pr, query, cc, tc.tideConfig) + + if diff != tc.expectedDiff { + t.Errorf("Expected diff %d, but got %d", tc.expectedDiff, diff) + } + + if tc.expectedDescContains != "" { + if !strings.Contains(desc, tc.expectedDescContains) { + t.Errorf("Expected description to contain %q, but got %q", tc.expectedDescContains, desc) + } + } + + if tc.expectedDescNotContain != "" { + if strings.Contains(desc, tc.expectedDescNotContain) { + t.Errorf("Expected description NOT to contain %q, but got %q", tc.expectedDescNotContain, desc) + } + } + }) + } +} + +func TestPoolStatus(t *testing.T) { + testCases := []struct { + name string + mergeState string + policy config.GitHubMergeBlocksPolicy + expectedStatus string + }{ + { + name: "BLOCKED with permit policy shows warning", + mergeState: "BLOCKED", + policy: config.GitHubMergeBlocksPermit, + expectedStatus: statusInPoolDespiteBlocked, + }, + { + name: "BLOCKED with block policy shows normal status", + mergeState: "BLOCKED", + policy: config.GitHubMergeBlocksBlock, + expectedStatus: statusInPool, + }, + { + name: "BLOCKED with ignore policy shows normal status", + mergeState: "BLOCKED", + policy: config.GitHubMergeBlocksIgnore, + expectedStatus: statusInPool, + }, + { + name: "CLEAN with permit policy shows normal status", + mergeState: "CLEAN", + policy: config.GitHubMergeBlocksPermit, + expectedStatus: statusInPool, + }, + { + name: "BEHIND with permit policy shows normal status", + mergeState: "BEHIND", + policy: config.GitHubMergeBlocksPermit, + expectedStatus: statusInPool, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pr := &PullRequest{ + MergeStateStatus: githubql.String(tc.mergeState), + Repository: struct { + Name githubql.String + NameWithOwner githubql.String + Owner struct { + Login githubql.String + } + }{ + Name: githubql.String("test-repo"), + Owner: struct { + Login githubql.String + }{ + Login: githubql.String("test-org"), + }, + }, + } + + tide := &config.Tide{ + GitHubMergeBlocksPolicyMap: map[string]config.GitHubMergeBlocksPolicy{ + "*": tc.policy, + }, + } + + log := logrus.NewEntry(logrus.New()) + status := poolStatus(pr, tide, log) + + if status != tc.expectedStatus { + t.Errorf("Expected status %q, but got %q", tc.expectedStatus, status) + } + }) + } +} + func TestSetStatuses(t *testing.T) { statusNotInPoolEmpty := fmt.Sprintf(statusNotInPool, "") testcases := []struct { diff --git a/pkg/tide/tide.go b/pkg/tide/tide.go index c4e974412..53ef77abf 100644 --- a/pkg/tide/tide.go +++ b/pkg/tide/tide.go @@ -1920,11 +1920,12 @@ type PullRequest struct { Name githubql.String Prefix githubql.String } - HeadRefName githubql.String `graphql:"headRefName"` - HeadRefOID githubql.String `graphql:"headRefOid"` - Mergeable githubql.MergeableState - CanBeRebased githubql.Boolean `graphql:"canBeRebased"` - Repository struct { + HeadRefName githubql.String `graphql:"headRefName"` + HeadRefOID githubql.String `graphql:"headRefOid"` + Mergeable githubql.MergeableState + MergeStateStatus githubql.String `graphql:"mergeStateStatus"` + CanBeRebased githubql.Boolean `graphql:"canBeRebased"` + Repository struct { Name githubql.String NameWithOwner githubql.String Owner struct { diff --git a/pkg/tide/tide_test.go b/pkg/tide/tide_test.go index d114dca55..4848f6966 100644 --- a/pkg/tide/tide_test.go +++ b/pkg/tide/tide_test.go @@ -1523,6 +1523,185 @@ func TestRebaseMergeMethodIsAllowed(t *testing.T) { } } +func TestIsAllowedToMerge_ReviewDecision(t *testing.T) { + orgName := "test-org" + repoName := "test-repo" + + testCases := []struct { + name string + mergeStateStatus string + policyConfig map[string]config.GitHubMergeBlocksPolicy + expectedMergeOutput string + expectedMergeAllowed bool + }{ + { + name: "BLOCKED status with block policy globally", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksBlock, + }, + expectedMergeOutput: "PR is blocked from merging by GitHub (check branch protection, required reviews, or rulesets)", + expectedMergeAllowed: false, + }, + { + name: "BLOCKED status with permit policy globally", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksPermit, + }, + expectedMergeOutput: "", + expectedMergeAllowed: true, + }, + { + name: "BLOCKED status with ignore policy globally", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksIgnore, + }, + expectedMergeOutput: "", + expectedMergeAllowed: true, + }, + { + name: "BLOCKED status with policy not configured (default to permit)", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{}, + expectedMergeOutput: "", + expectedMergeAllowed: true, + }, + { + name: "BLOCKED status with block policy for specific org", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + orgName: config.GitHubMergeBlocksBlock, + }, + expectedMergeOutput: "PR is blocked from merging by GitHub (check branch protection, required reviews, or rulesets)", + expectedMergeAllowed: false, + }, + { + name: "BLOCKED status with block policy for different org", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + "other-org": config.GitHubMergeBlocksBlock, + }, + expectedMergeOutput: "", + expectedMergeAllowed: true, + }, + { + name: "BLOCKED status with block policy for specific repo", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + fmt.Sprintf("%s/%s", orgName, repoName): config.GitHubMergeBlocksBlock, + }, + expectedMergeOutput: "PR is blocked from merging by GitHub (check branch protection, required reviews, or rulesets)", + expectedMergeAllowed: false, + }, + { + name: "BLOCKED status with block policy for different repo", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + fmt.Sprintf("%s/other-repo", orgName): config.GitHubMergeBlocksBlock, + }, + expectedMergeOutput: "", + expectedMergeAllowed: true, + }, + { + name: "BLOCKED status - repo config overrides org config (ignore)", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + orgName: config.GitHubMergeBlocksBlock, + fmt.Sprintf("%s/%s", orgName, repoName): config.GitHubMergeBlocksIgnore, + }, + expectedMergeOutput: "", + expectedMergeAllowed: true, + }, + { + name: "BLOCKED status - repo config overrides org config (block)", + mergeStateStatus: "BLOCKED", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + orgName: config.GitHubMergeBlocksPermit, + fmt.Sprintf("%s/%s", orgName, repoName): config.GitHubMergeBlocksBlock, + }, + expectedMergeOutput: "PR is blocked from merging by GitHub (check branch protection, required reviews, or rulesets)", + expectedMergeAllowed: false, + }, + { + name: "CLEAN status with block policy", + mergeStateStatus: "CLEAN", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksBlock, + }, + expectedMergeOutput: "", + expectedMergeAllowed: true, + }, + { + name: "BEHIND status with block policy", + mergeStateStatus: "BEHIND", + policyConfig: map[string]config.GitHubMergeBlocksPolicy{ + "*": config.GitHubMergeBlocksBlock, + }, + expectedMergeOutput: "", + expectedMergeAllowed: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tideConfig := config.Tide{ + TideGitHubConfig: config.TideGitHubConfig{ + MergeType: map[string]config.TideOrgMergeType{ + fmt.Sprintf("%s/%s", orgName, repoName): {MergeType: types.MergeMerge}, + }, + }, + GitHubMergeBlocksPolicyMap: tc.policyConfig, + } + cfg := func() *config.Config { return &config.Config{ProwConfig: config.ProwConfig{Tide: tideConfig}} } + mmc := newMergeChecker(cfg, &fgc{}) + mmc.cache = map[config.OrgRepo]map[types.PullRequestMergeType]bool{ + {Org: orgName, Repo: repoName}: { + types.MergeMerge: true, + }, + } + + pr := &PullRequest{ + Repository: struct { + Name githubql.String + NameWithOwner githubql.String + Owner struct { + Login githubql.String + } + }{ + Name: githubql.String(repoName), + Owner: struct { + Login githubql.String + }{ + Login: githubql.String(orgName), + }, + }, + Labels: struct { + Nodes []struct{ Name githubql.String } + }{ + Nodes: []struct{ Name githubql.String }{}, + }, + MergeStateStatus: githubql.String(tc.mergeStateStatus), + } + + mergeOutput, err := mmc.isAllowedToMerge(CodeReviewCommonFromPullRequest(pr)) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + if mergeOutput != tc.expectedMergeOutput { + t.Errorf("Expected merge output %q but got %q", tc.expectedMergeOutput, mergeOutput) + } + + isAllowed := mergeOutput == "" + if isAllowed != tc.expectedMergeAllowed { + t.Errorf("Expected merge allowed=%v but got %v (output: %q)", tc.expectedMergeAllowed, isAllowed, mergeOutput) + } + }) + } +} + func TestTakeActionV2(t *testing.T) { testTakeAction(localgit.NewV2, t) }