Document and deprecate cancel_merge_when_pipeline_succeeds

What does this MR do and why?

POST /projects/:id/merge_requests/:merge_request_iid/cancel_merge_when_pipeline_succeeds publishes a contract it does not honour. !255239 (closed) proposed moving the endpoint to match what is written about it; that is a breaking change, so @phikai and @marc_shaw settled on the opposite split: the correct contract arrives on a new cancel_auto_merge endpoint, and this endpoint keeps its behaviour and gets documentation that describes it honestly. This merge request is that second half.

The page and the desc block said What the endpoint sends
Success a full merge request {"status":"success"}
Cannot cancel 406, message Can't cancel the automatic merge 201 carrying {"message":"Can't cancel the automatic merge","status":"error","http_status":406}

The refusal row is worth reading twice. AutoMergeService#cancel returns error("Can't cancel the automatic merge", 406), and BaseServiceUtility#error builds a plain hash. A plain hash carries no status, so Grape applies its own 201 for a POST and the service's http_status: 406 arrives inside the body instead. A refused cancellation is therefore reported with a success status code, and only the body says otherwise. That is now on the page, because a client branching on the status code gets this wrong today and nothing published warns it.

What changed

doc/api/merge_requests.md describes both paths as they behave: one 201 for both outcomes, the status field as the thing to read, an example response for each, and an explicit note that the http_status field is not the status of the response. The example that printed a whole merge request is gone, as is the pointer to the single merge request response notes, because no merge request is returned. The section is marked deprecated with a warning alert, following deprecate a page or topic, which is where REST API deprecations sends you for an endpoint.

The desc block in lib/api/merge_requests.rb loses success Entities::MergeRequest and gains deprecated true, because it is the third place the old claim was published: doc/api/openapi/openapi_v3.yaml is generated from it and gave the 201 a APIEntitiesMergeRequest schema. A Grape desc block is documentation metadata that nothing reads at request time, so this is documentation rather than behaviour. The regenerated OpenAPI document is committed with it, and bundle exec rake gitlab:openapi:v3:check_docs passes.

I also dropped { code: 406, message: 'Not acceptable' } from that block's failure list. not_acceptable! is never called on this route, and 406 is listed on no other endpoint in this file, so it reads as the same mistaken belief that the service's http_status became the response status. Say the word if you would rather keep it and I will put it back.

The endpoint's behaviour is untouched. That is the whole point of this merge request.

Specs that pin the published contract

Two request specs now assert the two response bodies the page documents, so a later change cannot quietly turn the refusal into a real 406 without a test saying so.

Writing them turned up something worth knowing about the specs that were already there: the before block arms the auto-merge with AutoMergeService#execute(merge_request, STRATEGY_MERGE_WHEN_CHECKS_PASS), and on this fixture that call is a no-op. MergeWhenChecksPassService#availability_details refuses a merge request that is already mergeable with no pipeline in progress, which is exactly what the factory builds, so auto_merge_enabled is never set and every example in the block takes the refusal branch. Nothing could see it, because this endpoint answers 201 either way and the existing example asserts only the status code. The new success example arms the auto-merge with the :merge_when_checks_pass factory trait instead, which is what the rest of the file uses for this, and asserts both the body and auto_merge_enabled afterwards.

Judgement calls I would rather state than bury

The first commit carries Changelog: deprecated. I had left it off at first on the grounds that this is a documentation-only change, and that was wrong: it edits lib/api/merge_requests.rb and flips deprecated: true on an OpenAPI operation, so the changelog guidelines' "any client-facing change to our REST and GraphQL APIs must have a changelog entry" applies. Danger said so on this merge request and I read it as noise. The trailer is on the first commit so it survives a squash.

I did not add an entry to doc/api/rest/deprecations.md, on the grounds that the styleguide makes it optional: "To widely announce a deprecation, update the REST API deprecations page." I had first argued that every entry there is a breaking change promising a v5 removal, and that is not true. require_password_to_approve carries no breaking-change label, and restrict_user_defined_variables names no removal at all, so an entry need not promise one. The pull mirroring entry is this merge request's exact shape: an endpoint deprecated, replaced by a new one, with its heading suffixed -deprecated. So the page would accept an entry for this; I just do not think one endpoint gaining a better-named sibling needs a wide announcement. Say the word and I will add it.

The [Deprecated](...) link in the warning points at this merge request, the one that performs the deprecation. The first commit had it citing !255239 (closed); that merge request has since been closed without merging, so it deprecates nothing and a reader following the link would land on a withdrawn proposal. This is what the rest of the page already does: the wip filter parameter and reference each cite the merge request that deprecated them.

The old page said 201 meant "Success, or the merge request has already merged". I did not carry that forward: the condition the code actually tests is auto_merge_enabled?, and I could not confirm from the source what that flag holds on an already-merged merge request. The new wording says the merge request is not set to auto-merge, which is exactly what the branch tests.

Merging this after !255702

!255702 should merge first, and this merge request is written to be rebased onto it. Both reviewers asked for the deprecation notice to link to #cancel-auto-merge rather than name it, and I agree; the link cannot go in yet because that section does not exist on this branch and docs-lint links runs lychee --include-fragments, so it would fail the pipeline. The warning names the endpoint in code font today, and the link goes in on the rebase, along with two conflict resolutions:

In doc/api/merge_requests.md, the deprecated section's closing example and its http_status note come first, and !255702's new ## Cancel auto merge section after them.

In doc/api/merge_trains.md, !255702's version of line 493 is the one to keep. It repoints that link at #cancel-auto-merge, which is the correct end state: a reader who wants to take a merge request off a merge train should be sent to the endpoint we recommend, not the deprecated one. This branch only repoints it at the renamed #cancel-merge-when-pipeline-succeeds-deprecated anchor so its own link check passes while cancel_auto_merge does not exist, so that change drops out at rebase.

Renaming the topic to Cancel merge when pipeline succeeds (deprecated) is what moves the anchor. lychee --offline --include-fragments doc tooling/docs/api/tags reports 0 errors over the whole tree.

References

How to set up and validate locally

  1. Set a merge request to auto-merge, then call the endpoint and observe 201 with {"status":"success"}.
  2. Call it a second time, with no auto-merge set, and observe 201 with {"message":"Can't cancel the automatic merge","status":"error","http_status":406} rather than a 406 response.
  3. Compare both against the rewritten section of doc/api/merge_requests.md.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by José M. Requena Plens

Merge request reports

Loading
Loading