diff --git a/TEMPLATES.md b/TEMPLATES.md index 5f202e71d..277220a44 100644 --- a/TEMPLATES.md +++ b/TEMPLATES.md @@ -3751,6 +3751,23 @@ Use `schemabot status -e ` to find the apply ID. +
+Control Command Ambiguous Apply ID + + +## Which Schema Change? + +This PR has more than one schema change in `staging`, so `schemabot cancel` needs to be told which one. + +| Apply | Database | State | +|---|---|---| +| `apply_a1b2c3d4` | `orders` | running | +| `apply_e5f6a7b8` | `customers` | stopped | + +Usage: `schemabot cancel -e staging` + +
+
Volume Changed: Superseded Progress Comment diff --git a/pkg/cmd/internal/templates/preview_comment.go b/pkg/cmd/internal/templates/preview_comment.go index 7a3d2a216..155451c15 100644 --- a/pkg/cmd/internal/templates/preview_comment.go +++ b/pkg/cmd/internal/templates/preview_comment.go @@ -89,6 +89,7 @@ func previewCommentAllOutput() { {"VOLUME COMMAND ACCEPTED", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeCommandAccepted()) }}, {"VOLUME COMMAND INVALID LEVEL", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeInvalidLevel()) }}, {"VOLUME COMMAND MISSING APPLY ID", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeMissingApplyID()) }}, + {"CONTROL COMMAND AMBIGUOUS APPLY ID", func() { fmt.Print(webhooktemplates.PreviewCommentControlAmbiguousApplyID()) }}, {"VOLUME CHANGED: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeSupersededProgress()) }}, {"RESUMED: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentResumeSupersededProgress()) }}, {"REVERT: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentRevertSupersededProgress()) }}, @@ -266,6 +267,7 @@ func previewCommentApplyFlowAllOutput() { {"VOLUME COMMAND ACCEPTED", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeCommandAccepted()) }}, {"VOLUME COMMAND INVALID LEVEL", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeInvalidLevel()) }}, {"VOLUME COMMAND MISSING APPLY ID", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeMissingApplyID()) }}, + {"CONTROL COMMAND AMBIGUOUS APPLY ID", func() { fmt.Print(webhooktemplates.PreviewCommentControlAmbiguousApplyID()) }}, {"VOLUME CHANGED: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeSupersededProgress()) }}, {"RESUMED: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentResumeSupersededProgress()) }}, {"REVERT: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentRevertSupersededProgress()) }}, diff --git a/pkg/webhook/control.go b/pkg/webhook/control.go index a1f5ae31f..9f629f555 100644 --- a/pkg/webhook/control.go +++ b/pkg/webhook/control.go @@ -7,6 +7,7 @@ import ( "github.com/block/schemabot/pkg/api" "github.com/block/schemabot/pkg/apitypes" + "github.com/block/schemabot/pkg/state" "github.com/block/schemabot/pkg/storage" "github.com/block/schemabot/pkg/webhook/action" "github.com/block/schemabot/pkg/webhook/templates" @@ -33,20 +34,130 @@ func (h *Handler) logControlCommandError(command, repo string, pr int, applyID, } } -func (h *Handler) loadApplyForPRControl(ctx context.Context, repo string, pr int, installationID int64, requestedBy string, result CommandResult, command string) (*storage.Apply, bool) { - if result.ApplyID == "" { - if h.silentUsageErrorOnUnscopedFanOut(repo, result.Tenant) { - h.logger.Info("skipping missing-apply-id reply for unscoped fan-out control command; the leader posts it once", - "command", command, - "repo", repo, - "pr", pr, - "environment", result.Environment, - "requested_by", requestedBy) - return nil, false +// controllableApplyState reports whether a control command can still act on an +// apply in this state. Stopped is terminal but not settled — stopping leaves the +// schema change half-applied, and cancel is how an operator finishes it — so a +// stopped apply is still something an operator's command can be about. +func controllableApplyState(applyState string) bool { + return !state.IsTerminalApplyState(applyState) || state.IsState(applyState, state.Apply.Stopped) +} + +// inferApplyForPRControl resolves the apply an operator meant when their control +// command named no apply ID. +// +// `schemabot apply` takes no ID, so operators reach for `schemabot cancel -e +// ` the same way; on a PR carrying a single schema change in that +// environment there is only one apply the command can be about, and demanding +// the ID sends them to `schemabot status` to copy back a value SchemaBot already +// knows. Anything less certain than one candidate is answered, not guessed: no +// candidates gets the usage reply, and several get their identifiers listed so +// the operator names the one they mean. +// +// Inference is scoped to what this deployment stores, so it is only sound when +// this deployment is the one answering. An unscoped command on an aggregate repo +// fans out to deployments that each hold their own slice of the PR's applies, +// where "exactly one here" does not mean "exactly one on this PR" — those keep +// the usage reply and its explicit ID. +func (h *Handler) inferApplyForPRControl( + ctx context.Context, applyStore storage.ApplyStore, + repo string, pr int, installationID int64, requestedBy string, + result CommandResult, command string, +) (*storage.Apply, bool) { + if h.silentOnUnscopedFanOut(repo, result.Tenant) { + h.logger.Info("not inferring an apply id for an unscoped fan-out control command; this deployment holds only part of the PR's applies", + "command", command, "repo", repo, "pr", pr, + "environment", result.Environment, "requested_by", requestedBy) + return nil, false + } + + applies, err := applyStore.GetByPR(ctx, repo, pr) + if err != nil { + h.logger.Error("failed to load the PR's applies to infer the apply id for a control command", + "command", command, "repo", repo, "pr", pr, + "environment", result.Environment, "requested_by", requestedBy, "error", err) + // The lookup failure is logged above with the identifiers that make it + // triageable. The PR comment is public, so it carries the outcome rather + // than raw storage text naming hosts, addresses, or driver internals. + h.postCommandError(repo, pr, installationID, command, result.Environment, requestedBy, + "Failed to look up this PR's schema changes. Retry, and see server logs if it persists.") + return nil, false + } + + var candidates []*storage.Apply + for _, apply := range applies { + if apply.Environment == result.Environment && controllableApplyState(apply.State) { + candidates = append(candidates, apply) } + } + + if len(candidates) == 1 { + h.logger.Info("inferred the apply id for a control command from the PR's only schema change in this environment", + append(candidates[0].LogAttrs(), "command", command, "requested_by", requestedBy)...) + return candidates[0], true + } + h.logger.Info("control command named no apply id and the PR's applies do not resolve it to one", + "command", command, "repo", repo, "pr", pr, + "environment", result.Environment, "requested_by", requestedBy, + "candidates", len(candidates)) + return nil, false +} + +// replyForUnresolvedApplyID answers a control command whose apply the PR could +// not resolve: the usage line when nothing on the PR matches, and the candidates +// themselves when several do. +func (h *Handler) replyForUnresolvedApplyID( + ctx context.Context, applyStore storage.ApplyStore, + repo string, pr int, installationID int64, requestedBy string, + result CommandResult, command string, +) { + if h.silentUsageErrorOnUnscopedFanOut(repo, result.Tenant) { + h.logger.Info("skipping missing-apply-id reply for unscoped fan-out control command; the leader posts it once", + "command", command, + "repo", repo, + "pr", pr, + "environment", result.Environment, + "requested_by", requestedBy) + return + } + + // On an unscoped fan-out this deployment stores only its own slice of the + // PR's applies, which is why the apply id was not inferred. A candidate list + // built from that slice would present a partial view of the PR as the whole + // of it, so the reply stays the usage line the leader posts once. + if h.silentOnUnscopedFanOut(repo, result.Tenant) { h.postComment(repo, pr, installationID, templates.RenderControlMissingApplyID(command)) - return nil, false + return } + + applies, err := applyStore.GetByPR(ctx, repo, pr) + if err != nil { + // The usage reply stands on its own, so a lookup failure here costs the + // operator the candidate list, not the answer. + h.logger.Warn("failed to load the PR's applies to list control command candidates; replying with usage only", + "command", command, "repo", repo, "pr", pr, + "environment", result.Environment, "requested_by", requestedBy, "error", err) + h.postComment(repo, pr, installationID, templates.RenderControlMissingApplyID(command)) + return + } + + var candidates []templates.ControlApplyCandidate + for _, apply := range applies { + if apply.Environment == result.Environment && controllableApplyState(apply.State) { + candidates = append(candidates, templates.ControlApplyCandidate{ + ApplyID: apply.ApplyIdentifier, + Database: apply.Database, + State: apply.State, + }) + } + } + if len(candidates) == 0 { + h.postComment(repo, pr, installationID, templates.RenderControlMissingApplyID(command)) + return + } + h.postComment(repo, pr, installationID, templates.RenderControlAmbiguousApplyID(command, result.Environment, candidates)) +} + +func (h *Handler) loadApplyForPRControl(ctx context.Context, repo string, pr int, installationID int64, requestedBy string, result CommandResult, command string) (*storage.Apply, bool) { if h.service == nil { h.logger.Error("service not configured for PR control command", "command", command, @@ -82,6 +193,19 @@ func (h *Handler) loadApplyForPRControl(ctx context.Context, repo string, pr int h.postCommandError(repo, pr, installationID, command, result.Environment, requestedBy, "SchemaBot apply storage is not configured for "+command+" commands") return nil, false } + // An operator who names no apply is answered from the PR itself: acted on + // when the PR resolves to exactly one candidate, and asked which one when it + // does not. + if result.ApplyID == "" { + inferred, ok := h.inferApplyForPRControl(ctx, applyStore, repo, pr, installationID, requestedBy, result, command) + if !ok { + h.replyForUnresolvedApplyID(ctx, applyStore, repo, pr, installationID, requestedBy, result, command) + return nil, false + } + h.acknowledgeCommandActPoint(repo, pr, installationID, result) + return inferred, true + } + apply, err := applyStore.GetByApplyIdentifier(ctx, result.ApplyID) if err != nil { h.logger.Error("failed to load apply for PR control command", @@ -159,16 +283,22 @@ func runControlCommand[R any]( h *Handler, ctx context.Context, repo string, pr int, installationID int64, requestedBy string, - result CommandResult, + result *CommandResult, actionName string, execute func(ctx context.Context, req apitypes.ControlRequest) (*R, error), accepted func(*R) bool, errorMessage func(*R) string, ) *R { - apply, ok := h.loadApplyForPRControl(ctx, repo, pr, installationID, requestedBy, result, actionName) + apply, ok := h.loadApplyForPRControl(ctx, repo, pr, installationID, requestedBy, *result, actionName) if !ok { return nil } + // The apply the command acts on is settled here, whether the operator named + // it or the PR resolved it. The result is threaded by pointer so everything + // downstream — the request, the logs, and the caller's acknowledgement + // comment — reports that apply, and an inferred command reads back the + // identifier it acted on rather than a blank. + result.ApplyID = apply.ApplyIdentifier client, blocked := h.actorAuthorizationClient(repo, pr, installationID, requestedBy, apply.Database, result.Environment, actionName) if blocked { return nil @@ -227,7 +357,7 @@ func (h *Handler) handleStopCommand(repo string, pr int, installationID int64, r ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Stop, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Stop, h.service.ExecuteStop, func(r *apitypes.StopResponse) bool { return r.Accepted }, func(r *apitypes.StopResponse) string { return r.ErrorMessage }) @@ -260,7 +390,7 @@ func (h *Handler) handleCancelCommand(repo string, pr int, installationID int64, ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Cancel, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Cancel, h.service.ExecuteCancel, func(r *apitypes.CancelResponse) bool { return r.Accepted }, func(r *apitypes.CancelResponse) string { return r.ErrorMessage }) @@ -293,7 +423,7 @@ func (h *Handler) handleStartCommand(repo string, pr int, installationID int64, ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Start, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Start, h.service.ExecuteStart, func(r *apitypes.StartResponse) bool { return r.Accepted }, func(r *apitypes.StartResponse) string { return r.ErrorMessage }) @@ -327,7 +457,7 @@ func (h *Handler) handleReleaseCommand(repo string, pr int, installationID int64 ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Release, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Release, h.service.ExecuteRelease, func(r *apitypes.ReleaseResponse) bool { return r.Accepted }, func(r *apitypes.ReleaseResponse) string { return r.ErrorMessage }) @@ -356,7 +486,7 @@ func (h *Handler) handleCutoverCommand(repo string, pr int, installationID int64 ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Cutover, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Cutover, h.service.ExecuteCutover, func(r *apitypes.ControlResponse) bool { return r.Accepted }, func(r *apitypes.ControlResponse) string { return r.ErrorMessage }) @@ -423,7 +553,7 @@ func (h *Handler) handleVolumeCommand(repo string, pr int, installationID int64, return } - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Volume, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Volume, func(ctx context.Context, req apitypes.ControlRequest) (*apitypes.VolumeResponse, error) { return h.service.ExecuteVolume(ctx, req, result.VolumeLevel) }, @@ -452,7 +582,7 @@ func (h *Handler) handleSkipRevertCommand(repo string, pr int, installationID in ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.SkipRevert, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.SkipRevert, h.service.ExecuteSkipRevert, func(r *apitypes.ControlResponse) bool { return r.Accepted }, func(r *apitypes.ControlResponse) string { return r.ErrorMessage }) @@ -477,7 +607,7 @@ func (h *Handler) handleRevertCommand(repo string, pr int, installationID int64, ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Revert, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Revert, h.service.ExecuteRevert, func(r *apitypes.ControlResponse) bool { return r.Accepted }, func(r *apitypes.ControlResponse) string { return r.ErrorMessage }) diff --git a/pkg/webhook/control_integration_test.go b/pkg/webhook/control_integration_test.go index 3a2de8d4d..ec1c4c673 100644 --- a/pkg/webhook/control_integration_test.go +++ b/pkg/webhook/control_integration_test.go @@ -1214,3 +1214,189 @@ func (c *stopCommandTernClient) SetPendingObserver(tern.ProgressObserver) {} func (c *stopCommandTernClient) SetObserver(int64, tern.ProgressObserver) {} func (c *stopCommandTernClient) Close() error { return nil } + +// postCancelCommandWithoutApplyID issues the command the way an operator reaches +// for it after `schemabot apply`, which takes no apply ID either. +func postCancelCommandWithoutApplyID(t *testing.T, h *Handler, user string) { + t.Helper() + req := buildWebhookRequest(t, webhookPayloadOpts{ + comment: "schemabot cancel -e staging", + userLogin: user, + isPR: true, + }, nil) + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + require.Equal(t, http.StatusOK, rr.Code) +} + +// captureCommentsAndReactions routes the PR's comment and reaction posts into +// channels a control-command test can read. +func captureCommentsAndReactions(t *testing.T, mux *http.ServeMux) (chan string, chan string) { + t.Helper() + comments := make(chan string, 10) + reactions := make(chan string, 10) + mux.HandleFunc("POST /repos/octocat/hello-world/issues/1/comments", func(w http.ResponseWriter, r *http.Request) { + var body struct { + Body string `json:"body"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + comments <- body.Body + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{"id": 99}) + }) + mux.HandleFunc("POST /repos/octocat/hello-world/issues/comments/42/reactions", func(w http.ResponseWriter, r *http.Request) { + var body struct { + Content string `json:"content"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + reactions <- body.Content + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{"id": 1}) + }) + return comments, reactions +} + +// `schemabot apply` takes no apply ID, so operators reach for `schemabot cancel +// -e ` the same way. On a PR carrying a single schema change in that +// environment there is only one apply the command can be about, so it acts on +// that one instead of sending the operator to `schemabot status` to copy back an +// identifier SchemaBot already has. +func TestE2ECancelCommandWithoutApplyIDActsOnThePRsOnlySchemaChange(t *testing.T) { + ctx := t.Context() + schemabotDB, err := sql.Open("mysql", e2eSchemabotDSN) + require.NoError(t, err) + require.NoError(t, schemabotDB.PingContext(ctx)) + + store := mysqlstore.New(schemabotDB) + applyIdentifier := "apply_1e55c0de" + database := "cancel_inferred_only_db" + cleanupStopCommandTestRows(t, schemabotDB, applyIdentifier, database) + t.Cleanup(func() { + cleanupStopCommandTestRows(t, schemabotDB, applyIdentifier, database) + utils.CloseAndLog(schemabotDB) + }) + applyID := createStopCommandApply(t, store, applyIdentifier, database) + + client, mux := setupGitHubServer(t) + comments, reactions := captureCommentsAndReactions(t, mux) + + service := apiServiceForStopCommandTest(t, store, database) + service.RegisterTernClient(database, "staging", &stopCommandTernClient{remote: true}) + h := &Handler{ + service: service, + ghClients: ghclient.NewSingleClientSet(defaultAppName, &fakeClientFactory{client: ghclient.NewInstallationClient(client, testLogger())}), + logger: testLogger(), + } + + postCancelCommandWithoutApplyID(t, h, "alice") + + comment := readComment(t, comments) + assert.Contains(t, comment, "Cancel Request Accepted") + // The reply names the apply it acted on, so an operator who never typed an + // identifier still knows which schema change they just cancelled. + assert.Contains(t, comment, "`"+applyIdentifier+"`") + + controlReq, err := store.ControlRequests().GetPending(ctx, applyID, storage.ControlOperationCancel) + require.NoError(t, err) + require.NotNil(t, controlReq, "the PR's only schema change was not cancelled") + assert.Equal(t, "github:alice@octocat/hello-world#1", controlReq.RequestedBy) + assertReactionEventually(t, reactions) +} + +// With more than one schema change in the environment the command is genuinely +// ambiguous, and guessing would cancel work the operator did not name. The reply +// lists the candidates so they reissue the command against the one they mean, +// and nothing is cancelled in the meantime. +func TestE2ECancelCommandWithoutApplyIDListsCandidatesWhenThePRHasSeveral(t *testing.T) { + ctx := t.Context() + schemabotDB, err := sql.Open("mysql", e2eSchemabotDSN) + require.NoError(t, err) + require.NoError(t, schemabotDB.PingContext(ctx)) + + store := mysqlstore.New(schemabotDB) + firstIdentifier, secondIdentifier := "apply_a3b1c0de", "apply_b7d2e0fa" + firstDatabase, secondDatabase := "cancel_ambiguous_db_a", "cancel_ambiguous_db_b" + cleanup := func() { + cleanupStopCommandTestRows(t, schemabotDB, firstIdentifier, firstDatabase) + cleanupStopCommandTestRows(t, schemabotDB, secondIdentifier, secondDatabase) + } + cleanup() + t.Cleanup(func() { + cleanup() + utils.CloseAndLog(schemabotDB) + }) + firstApplyID := createStopCommandApply(t, store, firstIdentifier, firstDatabase) + secondApplyID := createStopCommandApply(t, store, secondIdentifier, secondDatabase) + + client, mux := setupGitHubServer(t) + comments, _ := captureCommentsAndReactions(t, mux) + + service := apiServiceForStopCommandTest(t, store, firstDatabase) + service.RegisterTernClient(firstDatabase, "staging", &stopCommandTernClient{remote: true}) + h := &Handler{ + service: service, + ghClients: ghclient.NewSingleClientSet(defaultAppName, &fakeClientFactory{client: ghclient.NewInstallationClient(client, testLogger())}), + logger: testLogger(), + } + + postCancelCommandWithoutApplyID(t, h, "alice") + + comment := readComment(t, comments) + assert.Contains(t, comment, "Which Schema Change?") + assert.Contains(t, comment, "`"+firstIdentifier+"`") + assert.Contains(t, comment, "`"+secondIdentifier+"`") + assert.Contains(t, comment, "`schemabot cancel -e staging`") + + for _, applyID := range []int64{firstApplyID, secondApplyID} { + controlReq, err := store.ControlRequests().GetPending(ctx, applyID, storage.ControlOperationCancel) + require.NoError(t, err) + assert.Nil(t, controlReq, "an ambiguous cancel must not settle on one of the candidates") + } +} + +// An unscoped control command on an aggregate repo fans out to deployments that +// each store their own slice of the PR's applies, so "exactly one here" is not a +// statement about the PR. The reply stays the usage line rather than a candidate +// table this deployment cannot see the whole of. +func TestE2ECancelCommandWithoutApplyIDDoesNotListCandidatesOnAnAggregateFanOut(t *testing.T) { + ctx := t.Context() + schemabotDB, err := sql.Open("mysql", e2eSchemabotDSN) + require.NoError(t, err) + require.NoError(t, schemabotDB.PingContext(ctx)) + + store := mysqlstore.New(schemabotDB) + applyIdentifier, database := "apply_c9f4a1bb", "cancel_fanout_db" + cleanup := func() { cleanupStopCommandTestRows(t, schemabotDB, applyIdentifier, database) } + cleanup() + t.Cleanup(func() { + cleanup() + utils.CloseAndLog(schemabotDB) + }) + applyID := createStopCommandApply(t, store, applyIdentifier, database) + + client, mux := setupGitHubServer(t) + comments, _ := captureCommentsAndReactions(t, mux) + + service := apiServiceForStopCommandTest(t, store, database) + service.Config().Repos = map[string]api.RepoConfig{ + "octocat/hello-world": {Aggregate: &api.AggregateConfig{Role: api.AggregateRoleLeader}}, + } + service.RegisterTernClient(database, "staging", &stopCommandTernClient{remote: true}) + h := &Handler{ + service: service, + ghClients: ghclient.NewSingleClientSet(defaultAppName, &fakeClientFactory{client: ghclient.NewInstallationClient(client, testLogger())}), + logger: testLogger(), + } + + postCancelCommandWithoutApplyID(t, h, "alice") + + comment := readComment(t, comments) + assert.Contains(t, comment, "Missing Apply ID") + assert.NotContains(t, comment, "Which Schema Change?") + assert.NotContains(t, comment, applyIdentifier, + "a deployment holding one slice of the PR must not present its slice as the PR's candidates") + + controlReq, err := store.ControlRequests().GetPending(ctx, applyID, storage.ControlOperationCancel) + require.NoError(t, err) + assert.Nil(t, controlReq, "a fan-out cancel with no apply id must not settle on this deployment's apply") +} diff --git a/pkg/webhook/templates/issue_comment.go b/pkg/webhook/templates/issue_comment.go index 01316f9b8..d03ca4104 100644 --- a/pkg/webhook/templates/issue_comment.go +++ b/pkg/webhook/templates/issue_comment.go @@ -88,18 +88,55 @@ type CutoverCommandAcceptedData struct { Status string } -// RenderControlMissingApplyID renders the message posted when an apply-scoped -// control command is invoked without the required apply ID. The usage line -// carries every flag the command requires, so a volume command also shows its -// mandatory `-v` level. -func RenderControlMissingApplyID(command string) string { - usage := fmt.Sprintf("schemabot %s -e ", command) +// controlCommandUsage renders a control command's usage line, carrying every +// flag the command requires — a volume command also shows its mandatory `-v` +// level. +func controlCommandUsage(command, environment string) string { + if environment == "" { + environment = "" + } + usage := fmt.Sprintf("schemabot %s -e %s", command, environment) if command == action.Volume { usage += fmt.Sprintf(" -v <%d-%d>", storage.MinVolume, storage.MaxVolume) } + return usage +} + +// RenderControlMissingApplyID renders the message posted when an apply-scoped +// control command names no apply ID and the PR holds nothing the command could +// have meant. +func RenderControlMissingApplyID(command string) string { return offerSupportChannel(fmt.Sprintf("## Missing Apply ID\n\n"+ "Usage: `%s`\n\n"+ - "Use `schemabot status -e ` to find the apply ID.", usage)) + "Use `schemabot status -e ` to find the apply ID.", controlCommandUsage(command, ""))) +} + +// ControlApplyCandidate is one schema change an apply-scoped control command +// could have meant. +type ControlApplyCandidate struct { + ApplyID string + Database string + State string +} + +// RenderControlAmbiguousApplyID renders the reply to an apply-scoped control +// command that named no apply ID on a PR carrying several the command could +// have meant. +// +// The operator's next step is to reissue the command with one of these +// identifiers, and they are all right here — so unlike the missing-ID reply +// this carries no support-channel link. Sending someone to a support channel to +// read back a list already in front of them is noise. +func RenderControlAmbiguousApplyID(command, environment string, candidates []ControlApplyCandidate) string { + var body strings.Builder + fmt.Fprintf(&body, "## Which Schema Change?\n\n"+ + "This PR has more than one schema change in `%s`, so `schemabot %s` needs to be told which one.\n\n"+ + "| Apply | Database | State |\n|---|---|---|\n", environment, command) + for _, c := range candidates { + fmt.Fprintf(&body, "| `%s` | `%s` | %s |\n", c.ApplyID, c.Database, c.State) + } + fmt.Fprintf(&body, "\nUsage: `%s`\n", controlCommandUsage(command, environment)) + return body.String() } // RenderVolumeInvalidLevel renders the message posted when a volume command @@ -306,6 +343,15 @@ func PreviewCommentVolumeMissingApplyID() string { return RenderControlMissingApplyID(action.Volume) } +// PreviewCommentControlAmbiguousApplyID renders the reply posted when a control +// command names no apply ID on a PR carrying several it could have meant. +func PreviewCommentControlAmbiguousApplyID() string { + return RenderControlAmbiguousApplyID(action.Cancel, "staging", []ControlApplyCandidate{ + {ApplyID: "apply_a1b2c3d4", Database: "orders", State: "running"}, + {ApplyID: "apply_e5f6a7b8", Database: "customers", State: "stopped"}, + }) +} + // VolumeSupersededProgressData contains data for freezing a progress comment // that a volume change has superseded. type VolumeSupersededProgressData struct {