The following discussions from !2857 should be addressed: - [ ] @vespian_gl started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/2857#note_3924880869): > non-blocking: This comment says "The update writes before its one read", but the handler reads first. > > `handleUpdateUpstream` calls `repositionedUpstreamReadable` before `arm.store.Reposition`, and that helper calls `arm.store.Upstreams` first. Then `settledUpstream` reads the list again. So the update reads, writes, and reads again. S17 requires this order: "The `POST` and `PATCH` evaluate the check before they write". > > As a result, a gate that the update skipped shows in `rig.store.Lists()`, which the case asserts on the line above. The `Repositions()` assertion does no harm, but the reason the comment gives for it is false. A reader who trusts the comment can accept a later change that moves the check after `Reposition`. That change breaks the S17 rule that a `503` applies nothing. Guardrail 17 in `AGENTS.md` requires a comment to describe the code as the change leaves it. > > The comment should go, or it should match the code. For example: "The update reads the association before it moves it, so Lists already catches a skipped gate; Repositions pins that no move reached the store either." > > <!-- mergeable:v1 {"at_sha":"50bea54635d7b3f348a763e9a71913bce101e473","finding":"F1","lens":"code","severity":"non-blocking"} --> > _This is a bot message 🤖 — /mergeable_ - [ ] @vespian_gl started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/2857#note_3924881792): > non-blocking: Two assertions in this file check less than their messages say. > > 1. This `require.Equal` on `201` says "the mix must be admitted". But `fakeVirtualUpstreams.Associate` accepts any upstream, and `handleAssociateUpstream` has no format or family gate before `arm.store.Associate`. So this status cannot fail on admission, and a store change that refuses the `docker`/`oci` mix does not fail here. `TestContainerVirtualUpstreamStore_Associate_AdmitsTheDockerOciMixBothWays` in the datastore suite tests the admission. This test pins that the associate renders the upstream's stored format. > 2. In `TestContainerUpstreams_List_VerdictFailure_FailsClosed`, the `unavailable` row (line 284) sets no `wantLog`. So the `assert.Contains` at line 315 becomes `Contains(logs, "")` on that row, which is always true. Its message says that the `500` log line names the lengths, but that row is a `503`. > > The first message should say what the test checks: the associate renders the upstream's stored format. The log assertion should run only when `tc.wantLog != ""`, or the `503` row can get its own expected fragment if that path writes one. > > <!-- mergeable:v1 {"at_sha":"50bea54635d7b3f348a763e9a71913bce101e473","finding":"F2","lens":"code","severity":"non-blocking"} --> > _This is a bot message 🤖 — /mergeable_ - [ ] @vespian_gl started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/2857#note_3924882460): > non-blocking: This test sends the same requests over the same state as the `the sibling container format` row at line 399. > > Under the `oci` arm, that row stores a `docker` virtual repository and sends all five routes under `/oci`. Under the `docker` arm, this test also stores a `docker` virtual repository and sends all five routes under `/oci`. The same pair holds for the other arm. The doc comment says this test reads "the same rule ... from the route rather than the row", but no handler change fails one test and passes the other. > > This test adds a seeded association row, a `Dissociates()` assertion, and `assertNoAuditEventsOnSink`. It does not assert `Lists()` or the `CodeNotFound` envelope, which the table does. So a reader sees two directions of coverage where there is one, and the copy with fewer checks is the one that claims to add a direction. > > One of the two should go. You can remove the sibling row from the table and add `Lists()` and the envelope check here. Or you can remove this test and move its `Dissociates()` and audit assertions into the table. If you keep both, the doc comment should not claim a separate direction. > > <!-- mergeable:v1 {"at_sha":"50bea54635d7b3f348a763e9a71913bce101e473","finding":"F3","lens":"code","severity":"non-blocking"} --> > _This is a bot message 🤖 — /mergeable_ - [ ] @vespian_gl started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/2857#note_3924883006): > non-blocking: Part 11 already defines `containerArm{segment, format}`, `containerArms()`, and `containerArmRow(t, suffix, position, kind, format)` in `container_upstream_arms_test.go`. > > This file adds `containerUpstreamArm`, `containerUpstreamArms()`, and `containerUpstreamRow` with the same signature. It also adds `containerUpstreamTestID`, a copy of `npmUpstreamTestID` with a different prefix, and `containerUpstreamRoute`, which has the same fields as `npmUpstreamRoute`. The Maven suite reuses the npm route type: `mavenUpstreamRoutes` returns `[]npmUpstreamRoute`. Also, the rig's `arm` field (line 82) is set at line 126 and nothing reads it. > > Nothing fails today. But with two arm tables for the same two segments in one package, a change to one table does not reach the other suite, and that suite stops testing the new arm without failing. > > The container suites should share one arm table. You can add the sibling fields to part 11's `containerArm` and build the rows with `containerArmRow`. `containerUpstreamRoutes` can return `[]npmUpstreamRoute`, as the Maven suite does. The unread `arm` field should go, or a test should use it. > > <!-- mergeable:v1 {"at_sha":"50bea54635d7b3f348a763e9a71913bce101e473","finding":"F4","lens":"code","severity":"non-blocking"} --> > _This is a bot message 🤖 — /mergeable_