feat(managementapi): declare the container tag upsert operation
Stacked on feat(managementapi): declare the artifact delet... (!1445 - merged) • Hayley Swimelar • 19.3, merges after it.
Why
The tag-upsert PUT is the one Phase 4 artifact write that is not a delete, and the only operation that answers limit_exceeded. Declaring it ahead of its handler lets a consumer generate the client now, and the parent MR's pending-sweep set keeps the arms unserved without reddening the sweep. Step 2 of the merged S17 Phase 4 plan, whose Approach slices the phase into four contract MRs.
Nothing serves the operation yet, by design. PUT on the tag path answers the mux's text/plain 405 until the write-route scaffold registers it, which is the state the two pending entries record.
What
- The tag cap binds per manifest, and the spec at this branch's base agrees.
ContainerManifestPersister.moveTagcountscontainer_tagsbycontainer_manifest_idand runs the check on the manifest the digest names for any real pointer change, skipping it only when the tag already points there.docs/specs/S17-rest-management-api.mdcarries that reading onmain, and this branch is rebased past the MR that corrected it, docs(specs): correct four S17 claims the merged... (!1453 - merged) • Hayley Swimelar • 19.3. Creating a tag, or retargeting one, onto a manifest already at the cap answers422 limit_exceeded. Repeating a tag's current digest answers204, because it moves nothing. - The
422is declared inline rather than referencingUnprocessableEntity. Its description is the only place the two codes a422carries are told apart, and the shared prose covers the repository writes instead. Contract tests pin that description clause by clause, not by keyword: an earlier keyword-anchored version passed a rewrite that scoped the count correctly and then called a retarget free, which mutation testing caught in review. - The cap is described without its number.
manifest_max_tagsis operator-configurable, and the repository's own testdata runs it at 500, so the contract says the target manifest "already holds its tag cap" rather than quoting 1,000. The document's numberless convention for configurable caps is what this follows. - No
409, and both successes are empty. A tag upsert refuses nothing, so the cap failure is a422rather than a conflict, asserted negatively. The spec states201on create and204on retarget and is silent on their bodies: a204cannot carry one, and a body on the201alone would make a client branch on status before knowing whether to decode, so the status is the whole signal and the caller re-reads tag detail. NoLocationon the201either, since aPUTcreates at the request URI, and a test now pins that promise. - The shared
tag_nameparameter's404rule is scoped to the read and the delete. It read as a whole-path fact, which the upsert's grammar400contradicts quoted on its own. The parameter now also names who owns the grammar and warns that it is stricter than themaxLength: 255in the same paragraph. 413is declared because the operation carries a body, matchingcreateRepositoryandupdateRepository. The cap itself stays middleware-owned.
Spec coverage
| Spec rule | Test |
|---|---|
| The document defines every Phase 4 endpoint and declares each operation's failure responses (the tag-upsert slice, AC #30) | TestContract_TagUpsert_DeclaresItsResponses |
PUT returns 201 on create and 204 on retarget (AC #52, contract half) |
same, closed status set plus both successes declaring no content and the 201 declaring no Location |
A well-formed digest naming no manifest returns 422, a malformed digest or rejected :tag_name returns 400 (AC #53) |
same, plus TestContract_UpsertContainerTagRequest_ValidatesBodies (eight bodies: wrong algorithm, short hex, uppercase hex, non-string, null, unknown field, missing digest) |
The cap failure is 422 limit_exceeded on the target manifest, a retarget at the cap answers the same 422, and an unchanged digest answers 204 (AC #54) |
same, clause-level description assertions, mutation-proven against the image-scoped rewrite |
| A new envelope code lands as a transport constant and an enum value together | TestContract_ErrorCodeEnum_MatchesTransportCodes in both this package and internal/gitlabapi, whose known list claims the full transport set |
| A declared arm with no handler is exempt only while it answers as unserved, and an entry naming no declared arm fails as orphaned | TestContractOperations_AreRegisteredAndServed, the two upsertContainerTag entries and its closing loop |
| Input validation runs before any artifact access | the request schema's required digest, closed property set, and canonical digest pattern |
| No failure response echoes a submitted digest | the 422 pins to the plain Error envelope, so it declares no details shape a digest could travel back in |
Behavioral halves belong to the handler step. This MR ships no handler, so nothing here creates a tag, moves one, or counts tags on a manifest.
No docs/testing/ e2e scenario is added or affected. This MR declares a contract and registers no route, and the plan's Testing Strategy assigns the management-write journeys in docs/testing/e2e/docker.md and docs/testing/e2e/oci.md to the container handler steps.
Two files outside the step's declared list
docs/dev/api-style.md: its status table read "Successful GET, PATCH, PUT" for200and204for DELETE alone. This MR declares the document's firstPUT, so it is the change that creates the contradiction. Three rows widened, nothing else.internal/gitlabapi/contract_test.go: one line. That test'sknownslice is documented as the full transport constant set, whichCodeLimitExceededwould otherwise falsify.
Noted for follow-up, outside this MR
- The plan's Step 19 carries the stale cap reading in three places:
422 limit_exceeded"on create only", "retarget-at-cap succeeds", and a test bullet reading "cap at the boundary (create rejected, retarget allowed)". A handler built to that text would violate this contract, and nothing here can catch it. Step 19 needs the corrected reading the way this step got it. - Step 19 should derive
201against204from the write, not from a probe. The contract says the two report whether the tag existed. A read-then-write discriminator answers201twice for concurrent same-name creates, whereRETURNING (xmax = 0)on the upsert does not. - The
v1.yamlcomment reading "The Phase 3 artifact resources below leaveadditionalPropertiesopen" now sits above one closed request schema. It is narrowly true, since it speaks of response resources, and the schema explains its own closure. Naming that subject explicitly is a one-word reword on a line the sibling contract MRs also touch, so it waits for them. docs/dev/bruno.mdis not updated here. Its coverage sentence and request counts are shared with the sibling contract MRs adding Bruno files right now, so reconciling per branch would collide three ways and still be wrong once the others land. The plan's Status table row rides separately: the four contract rows fill in one pass through docs(plans): fill the wave-one status rows and ... (!1467 - merged) • Hayley Swimelar • 19.3.
Context for LLM agents
Rationale
- Referencing the shared
UnprocessableEntityresponse for the422. Rejected because that description names the repository-write conditions and nolimit_exceeded, and this operation's422is the one place the two codes are distinguished for a client that has to tell a condition clearing after cleanup from one that never does. - Anchoring the
422prose assertions on keywords ("target manifest", "the same422"). Rejected after a mutation showed a rewrite satisfying both anchors while restoring the per-image reading. The assertions now anchor whole clauses against a whitespace-normalized description, and the mutation fails them in both directions. - Quoting the cap as 1,000 in the contract. Rejected because
manifest_max_tagsis configurable and a client hard-coding the document's number breaks on any deployment that lowers it. ADR-004 stays cited, with attribution, in the Bruno docs and the test rationale. - A resource body on the
201. Rejected because its paired204cannot carry one, so the asymmetry would push a status branch into every client, and the spec's own acceptance criterion has the caller read the newmanifest_digestfrom tag detail. - Putting the Bruno request in the
artifact-deletesfolder. Rejected because that folder'sdocsblock asserts every request in it is destructive and answers202, and an upsert is neither. It gets its owncontainer-tagsfolder atseq: 3, leaving4and up for the sibling bulk folders. - An ADR-004 link in the
422description. Rejected becauseapi/openapi/v1.yamlreferences no ADR anywhere and renders on Pages. The Brunodocsnames ADR-004, where two.brufiles already cite ADR-009. - Squashing the simplifier commit into the test-author commit. Rejected because re-forming that commit re-spends the project's single
--no-verifycarve-out, which covers only the commit the test-first flow produces. The intermediate state never left this branch, and MR-level squash stays available.
Non-goals
- Serving the operation, registering the route, or writing the handler. The write-route scaffold and the container tag upsert handler step own those, and the two pending entries exist precisely so this MR leaves the arms unserved.
- Correcting
docs/specs/S17-rest-management-api.md. That correction merged separately and this branch is rebased past it, so the spec here already reads as the contract does. - Declaring the manifests-per-image cap.
defaultContainerImageMaxManifestsis 25,000, mapped from ADR-004's versions-per-package, and no route in this step exposes it. - Enforcing the cap, the digest resolve, or the tag grammar. All three are handler-side, and the contract only has to make their outcomes expressible.
- Mapping
CodeLimitExceededto a status. No code-to-status map exists ininternal/transport, so the constant cannot fail open ahead of its first emitter. Its only live sibling today is the OCI push path's own400, in a different envelope.
Related to #313 (closed)