diff --git a/pkg/webhook/plan_drift.go b/pkg/webhook/plan_drift.go index 66b31bc1f..19c8df196 100644 --- a/pkg/webhook/plan_drift.go +++ b/pkg/webhook/plan_drift.go @@ -64,7 +64,7 @@ func (h *Handler) reviewTimeDrift(ctx context.Context, planReq api.PlanRequest, summary: "drift check failed; see logs", }, &templates.DeploymentDriftData{Computed: false} } - preview := deploymentDriftPreview(rollup) + preview := deploymentDriftPreview(rollup, primaryPlan.GetPlanId()) if rollup.Clean { return reviewDriftOutcome{state: driftClean}, preview } @@ -83,7 +83,7 @@ const erroredDriftDetail = "diff failed; see server logs" // deploymentDriftPreview turns a computed rollup into the PR-preview rendering // data. It returns nil for a single-deployment database: with one deployment // there is nothing to compare, so a "same plan everywhere" line would be noise. -func deploymentDriftPreview(rollup api.PlanRollup) *templates.DeploymentDriftData { +func deploymentDriftPreview(rollup api.PlanRollup, reviewedPlanID string) *templates.DeploymentDriftData { if len(rollup.Entries) <= 1 { return nil } @@ -124,7 +124,7 @@ func deploymentDriftPreview(rollup api.PlanRollup) *templates.DeploymentDriftDat // vocabulary of a fleet that may diverge, would suggest the agreement was an // outcome rather than the requirement that let the check pass. if rollup.Clean && independent { - data.Plans = deploymentPlanGroups(rollup) + data.Plans = deploymentPlanGroups(rollup, reviewedPlanID) } return data } @@ -139,16 +139,23 @@ func deploymentDriftPreview(rollup api.PlanRollup) *templates.DeploymentDriftDat // the reviewed plan is the one an operator has already seen, and a fixed order // keeps a comment that is re-rendered on a later push from reshuffling under a // reader who is looking for what changed. -func deploymentPlanGroups(rollup api.PlanRollup) []templates.DeploymentPlanGroup { +func deploymentPlanGroups(rollup api.PlanRollup, reviewedPlanID string) []templates.DeploymentPlanGroup { names := rollupMemberNames(rollup) var groups []templates.DeploymentPlanGroup byPlan := make(map[string]int, len(rollup.Entries)) for i, e := range rollup.Entries { at, ok := byPlan[e.PlanFingerprint] if !ok { + // The primary runs the reviewed plan itself, so it has no member plan + // row of its own and its identifier is the reviewed plan's. + planID := e.PlanIdentifier + if i == 0 { + planID = reviewedPlanID + } groups = append(groups, templates.DeploymentPlanGroup{ Primary: i == 0, Changes: memberPlanChanges(e.ChangeSet), + PlanID: planID, }) at = len(groups) - 1 byPlan[e.PlanFingerprint] = at diff --git a/pkg/webhook/plan_drift_test.go b/pkg/webhook/plan_drift_test.go index 868dceab1..9b13a3306 100644 --- a/pkg/webhook/plan_drift_test.go +++ b/pkg/webhook/plan_drift_test.go @@ -128,7 +128,7 @@ func TestDeploymentDriftPreview_NilForSingleDeployment(t *testing.T) { Entries: []api.DeploymentRollupEntry{{Deployment: "primary", Class: api.DeploymentMatch}}, Clean: true, } - assert.Nil(t, deploymentDriftPreview(rollup)) + assert.Nil(t, deploymentDriftPreview(rollup, "plan_reviewed")) } // A clean multi-deployment rollup becomes preview data flagged clean and @@ -141,7 +141,7 @@ func TestDeploymentDriftPreview_CleanMultiDeployment(t *testing.T) { }, Clean: true, } - preview := deploymentDriftPreview(rollup) + preview := deploymentDriftPreview(rollup, "plan_reviewed") assert.NotNil(t, preview) assert.True(t, preview.Computed) assert.True(t, preview.Clean) @@ -166,7 +166,7 @@ func TestDeploymentDriftPreview_DivergedAndErroredDetails(t *testing.T) { }, Clean: false, } - preview := deploymentDriftPreview(rollup) + preview := deploymentDriftPreview(rollup, "plan_reviewed") assert.False(t, preview.Clean) assert.Equal(t, "diverged", preview.Deployments[1].Class) assert.Contains(t, preview.Deployments[1].Detail, "1 unexpected") @@ -250,13 +250,37 @@ func TestDeploymentPlanGroups_SameWorkGroupsTogether(t *testing.T) { }, } - groups := deploymentPlanGroups(rollup) + groups := deploymentPlanGroups(rollup, "plan_reviewed") assert.Equal(t, [][]string{{"primary/testapp_1", "primary/testapp_2", "primary/testapp_3"}}, groupMembers(groups)) assert.True(t, groups[0].Primary) assert.Equal(t, []string{email}, groups[0].Changes[0].Statements) assert.False(t, groups[0].Empty()) } +// A withheld plan is only reachable if the comment can name it. The primary +// runs the reviewed plan itself and has no member plan row of its own, so its +// group carries the reviewed plan's identifier; every other group carries the +// identifier of the member plan its members were stored with. +func TestDeploymentPlanGroups_GroupsCarryThePlanToPrintThem(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + index := "ALTER TABLE users ADD INDEX idx_email (email)" + member := plannedMember("eu", "testapp_2", email, index) + member.PlanIdentifier = "plan_eu" + rollup := api.PlanRollup{ + Clean: true, + Planning: api.PlanIndependent, + Entries: []api.DeploymentRollupEntry{ + plannedMember("primary", "testapp_1", email), + member, + }, + } + + groups := deploymentPlanGroups(rollup, "plan_reviewed") + require.Len(t, groups, 2) + assert.Equal(t, "plan_reviewed", groups[0].PlanID) + assert.Equal(t, "plan_eu", groups[1].PlanID) +} + // Targets that hold their own schemas can need different work. Each distinct // plan is its own group, so the comment describes every plan the apply would // run rather than the reviewed one alone. @@ -273,7 +297,7 @@ func TestDeploymentPlanGroups_DifferentWorkSplits(t *testing.T) { }, } - groups := deploymentPlanGroups(rollup) + groups := deploymentPlanGroups(rollup, "plan_reviewed") assert.Equal(t, [][]string{ {"primary/testapp_1", "primary/testapp_3"}, {"primary/testapp_2"}, @@ -299,7 +323,7 @@ func TestDeploymentPlanGroups_ConvergedTargetsAreTheirOwnGroup(t *testing.T) { }, } - groups := deploymentPlanGroups(rollup) + groups := deploymentPlanGroups(rollup, "plan_reviewed") assert.Equal(t, [][]string{ {"primary/testapp_1", "primary/testapp_3"}, {"primary/testapp_2", "primary/testapp_4"}, @@ -322,7 +346,7 @@ func TestDeploymentPlanGroups_PrimaryGroupComesFirst(t *testing.T) { }, } - groups := deploymentPlanGroups(rollup) + groups := deploymentPlanGroups(rollup, "plan_reviewed") assert.True(t, groups[0].Primary) assert.Equal(t, []string{"primary/testapp_1"}, groups[0].Members) assert.True(t, groups[0].Empty(), "the primary having nothing to apply does not move its group") @@ -346,7 +370,7 @@ func TestDeploymentDriftPreview_BlockedRollupIsNotGrouped(t *testing.T) { }, } - preview := deploymentDriftPreview(rollup) + preview := deploymentDriftPreview(rollup, "plan_reviewed") assert.Empty(t, preview.Plans) assert.Len(t, preview.Deployments, 2) } @@ -367,7 +391,7 @@ func TestDeploymentDriftPreview_MirroredMembersAreNotGrouped(t *testing.T) { Entries: []api.DeploymentRollupEntry{eu, au}, } - preview := deploymentDriftPreview(rollup) + preview := deploymentDriftPreview(rollup, "plan_reviewed") assert.Empty(t, preview.Plans) } @@ -385,7 +409,7 @@ func TestDeploymentDriftPreview_CleanIndependentRollupCarriesGroups(t *testing.T }, } - preview := deploymentDriftPreview(rollup) + preview := deploymentDriftPreview(rollup, "plan_reviewed") assert.Len(t, preview.Plans, 2) } diff --git a/pkg/webhook/templates/plan.go b/pkg/webhook/templates/plan.go index f88cac00e..0727d2c04 100644 --- a/pkg/webhook/templates/plan.go +++ b/pkg/webhook/templates/plan.go @@ -227,6 +227,17 @@ type DeploymentPlanGroup struct { // the comment renders the reviewed plan itself. Empty for a group whose // members are already at the desired schema. Changes []KeyspaceChangeData + // PlanID names the stored plan the group's members would run, so a reader + // can print it in full when the comment has no room to show it. Every member + // of the group has a stored plan of its own and they are identical, so any + // one of them answers the question; this is the first member's. + PlanID string + + // clamped marks a group whose DDL body this comment gave up to stay inside + // GitHub's size cap. It is set while rendering, never by the caller: how much + // room a plan needs is a fact about the comment it lands in, not about the + // group. + clamped bool } // Empty reports that the group's members are already at the desired schema and @@ -316,8 +327,88 @@ type KeyspaceShardChange struct { Satisfied bool } +// planCommentChromeHeadroom reserves room under GitHub's cap for the markup a +// caller adds to the body after this renders — the tracking marker and the +// support-channel footer — plus margin, so an assembled comment never lands +// exactly at the limit. +const planCommentChromeHeadroom = 1024 + +// planCommentBudget is how large a plan comment may render. A comment over +// GitHub's cap is rejected outright, which would leave the PR with no plan at +// all rather than a long one. +const planCommentBudget = GitHubIssueCommentMaxChars - planCommentChromeHeadroom + // RenderPlanComment renders the plan comment markdown. +// +// A rollout of N targets against M tables can render past what GitHub will +// accept, so an oversized comment gives up DDL bodies until it fits, largest +// plan first. What it never gives up is the summary lines: the comment still +// names every target and says how much each one would run, and each withheld +// plan is replaced by the command that prints it in full. A reader who cannot +// see one plan's statements can still see that it exists, which target runs it, +// and where to read it. func RenderPlanComment(data PlanCommentData) string { + out := renderPlanComment(data) + if len(out) <= planCommentBudget || len(data.planGroups()) == 0 { + return out + } + + // Clamping works on a copy so the caller's groups are not rewritten by the + // act of rendering them. + groups := slices.Clone(data.DeploymentDrift.Plans) + drift := *data.DeploymentDrift + drift.Plans = groups + data.DeploymentDrift = &drift + + for range groups { + i := largestUnclampedGroup(groups) + if i < 0 { + break + } + groups[i].clamped = true + out = renderPlanComment(data) + if len(out) <= planCommentBudget { + break + } + } + return out +} + +// largestUnclampedGroup is the group whose DDL body would free the most room, +// or -1 when every group's body has already been given up. Taking the largest +// first withholds the fewest plans for the room it recovers. +func largestUnclampedGroup(groups []DeploymentPlanGroup) int { + best, bestSize := -1, 0 + for i, g := range groups { + if g.clamped || g.Empty() { + continue + } + if size := renderedDDLSize(g.Changes); size > bestSize { + best, bestSize = i, size + } + } + return best +} + +// renderedDDLSize approximates how much of a comment a group's statements +// occupy. It is used only to order the groups against each other, so it counts +// the statements rather than rendering them. +func renderedDDLSize(changes []KeyspaceChangeData) int { + size := 0 + for _, ks := range changes { + for _, s := range ks.Statements { + size += len(s) + } + for _, shard := range ks.Shards { + for _, s := range shard.Statements { + size += len(s) + } + } + } + return size +} + +func renderPlanComment(data PlanCommentData) string { var sb strings.Builder // Header @@ -1294,7 +1385,7 @@ func writePlanGroups(sb *strings.Builder, data PlanCommentData, groups []Deploym if !collapse { fmt.Fprintf(sb, "**%s** — %s\n\n", heading, label) - writeKeyspaceChanges(sb, scoped) + writePlanGroupBody(sb, scoped, g) continue } @@ -1303,7 +1394,7 @@ func writePlanGroups(sb *strings.Builder, data PlanCommentData, groups []Deploym open = " open" } fmt.Fprintf(sb, "\n%s — %s\n\n", open, heading, label) - writeKeyspaceChanges(sb, scoped) + writePlanGroupBody(sb, scoped, g) sb.WriteString("\n\n") } @@ -1315,6 +1406,27 @@ func writePlanGroups(sb *strings.Builder, data PlanCommentData, groups []Deploym } } +// writePlanGroupBody renders a group's statements, or the pointer that replaces +// them when the comment had no room for this plan. +// +// The pointer names the stored plan rather than summarizing it. A summary of +// withheld DDL is the one thing worse than withholding it: a reader would take +// it for the plan and stop looking. +func writePlanGroupBody(sb *strings.Builder, scoped PlanCommentData, g DeploymentPlanGroup) { + if !g.clamped { + writeKeyspaceChanges(sb, scoped) + return + } + sb.WriteString("This plan is too large to render here. To read it in full:\n\n") + if g.PlanID == "" { + // Without an identifier there is nothing to point at, so the comment says + // so plainly rather than printing a command that cannot work. + sb.WriteString("SchemaBot has no stored identifier for this plan. Re-plan the pull request to store one.\n\n") + return + } + fmt.Fprintf(sb, "```\nschemabot list-plans %s\n```\n\n", g.PlanID) +} + // writePlanGroupSummary states what the apply runs across the whole rollout, // standing in for the reviewed plan's own summary. Counting the primary's tables // would understate an apply that runs different work on the other targets, and diff --git a/pkg/webhook/templates/plan_drift_test.go b/pkg/webhook/templates/plan_drift_test.go index 210f9e38d..ec0d476de 100644 --- a/pkg/webhook/templates/plan_drift_test.go +++ b/pkg/webhook/templates/plan_drift_test.go @@ -618,6 +618,80 @@ func TestRenderPlanComment_ClampedGroupHeadingKeepsThePrimary(t *testing.T) { assert.NotContains(t, out, "my_db_4") } +// A rollout of many targets against many tables renders past what GitHub will +// accept, and an oversized comment is rejected outright — which would leave the +// PR with no plan at all. The comment gives up DDL bodies, largest plan first, +// until it fits, and points at the stored plan it withheld. +func TestRenderPlanComment_OversizedRolloutWithholdsTheLargestPlan(t *testing.T) { + out := renderGroupedPlan(planGroupChanges(1), []DeploymentPlanGroup{ + {Members: []string{"primary/a"}, Primary: true, Changes: planGroupChanges(1), PlanID: "plan_reviewed"}, + {Members: []string{"eu/b"}, Changes: planGroupChanges(2000), PlanID: "plan_eu"}, + {Members: []string{"ap/c"}, Changes: planGroupChanges(2500), PlanID: "plan_ap"}, + }) + + assert.LessOrEqual(t, len(out), planCommentBudget) + // The largest plan is the one withheld, and it is still named, still counted, + // and still reachable. + assert.Contains(t, out, "`ap/c` — 2500 DDL statements") + assert.Contains(t, out, "This plan is too large to render here.") + assert.Contains(t, out, "schemabot list-plans plan_ap") + assert.Contains(t, out, "📋 **Plan**: 3 distinct plans on 3 targets") + // The plans that fit are untouched. + assert.Contains(t, out, "`primary/a` (primary) — 1 DDL statement") + assert.NotContains(t, out, "schemabot list-plans plan_eu") + assert.NotContains(t, out, "schemabot list-plans plan_reviewed") +} + +// Withholding stops as soon as the comment fits, so a reader keeps as many +// plans as the budget allows rather than losing all of them to the first one +// that did not fit. +func TestRenderPlanComment_WithholdingStopsWhenTheCommentFits(t *testing.T) { + out := renderGroupedPlan(planGroupChanges(1), []DeploymentPlanGroup{ + {Members: []string{"primary/a"}, Primary: true, Changes: planGroupChanges(2000), PlanID: "plan_reviewed"}, + {Members: []string{"eu/b"}, Changes: planGroupChanges(2100), PlanID: "plan_eu"}, + {Members: []string{"ap/c"}, Changes: planGroupChanges(2200), PlanID: "plan_ap"}, + {Members: []string{"us/d"}, Changes: planGroupChanges(2300), PlanID: "plan_us"}, + }) + + assert.LessOrEqual(t, len(out), planCommentBudget) + assert.Equal(t, 3, strings.Count(out, "This plan is too large to render here."), + "only as many plans as the budget needs are withheld") + // Every group is still named and counted, whether or not its DDL survived. + for _, heading := range []string{ + "`primary/a` (primary) — 2000 DDL statements", + "`eu/b` — 2100 DDL statements", + "`ap/c` — 2200 DDL statements", + "`us/d` — 2300 DDL statements", + } { + assert.Contains(t, out, heading) + } +} + +// A plan the comment cannot show and cannot point at is said to be missing +// rather than pointed at with a command that would not resolve. +func TestRenderPlanComment_WithheldPlanWithoutAnIdentifierSaysSo(t *testing.T) { + out := renderGroupedPlan(planGroupChanges(1), []DeploymentPlanGroup{ + {Members: []string{"primary/a"}, Primary: true, Changes: planGroupChanges(1), PlanID: "plan_reviewed"}, + {Members: []string{"eu/b"}, Changes: planGroupChanges(2000), PlanID: "plan_eu"}, + {Members: []string{"ap/c"}, Changes: planGroupChanges(2500)}, + }) + + assert.LessOrEqual(t, len(out), planCommentBudget) + assert.Contains(t, out, "SchemaBot has no stored identifier for this plan.") + assert.NotContains(t, out, "schemabot list-plans\n") +} + +// A comment that fits withholds nothing. +func TestRenderPlanComment_ComfortableRolloutShowsEveryPlan(t *testing.T) { + out := renderGroupedPlan(planGroupChanges(1), []DeploymentPlanGroup{ + {Members: []string{"primary/a"}, Primary: true, Changes: planGroupChanges(1), PlanID: "plan_reviewed"}, + {Members: []string{"eu/b"}, Changes: planGroupChanges(2), PlanID: "plan_eu"}, + }) + + assert.NotContains(t, out, "too large to render here") + assert.Contains(t, out, "ALTER TABLE `t1` ADD COLUMN `c` int") +} + // The clamp keeps a list readable without costing a reader a name it would have // been just as short to state. func TestClampNameList(t *testing.T) {