fix(project): drop three fields the GitLab API does not accept

[markussiebert]

Description of your changes

Three Project spec fields were sent to GitLab although the API neither accepts nor
returns them. GitLab discards undeclared parameters without an error, so setting them
never had any effect.

Two of them were also part of isProjectUpToDate, compared against an observation that
can never be populated. A spec that set either one therefore stayed permanently out of
date: an update on every reconcile, a PUT returning 200 because GitLab dropped the
parameter, and the same difference again next time — while the resource reported
Synced=True and Ready=True. Same failure shape as #416.

buildCoverageRegex was removed from the Projects API in GitLab 15.0 and the identifier
does not occur anywhere in gitlab-org/gitlab at master. operationsAccessLevel was
deprecated in 15.8 and removed in 16.0; GitLab lists it under unexposed_attributes in
its own request spec. The third is containerExpirationPolicyAttributes.name_regex, the
snake_case field this spec carried alongside nameRegexDelete: GitLab declares the
deletion pattern as name_regex and nothing else, but client-go overwrites
ContainerExpirationPolicyAttributes.NameRegex with NameRegexDelete before encoding the
request, so a value placed in the snake_case field was cleared and never left the process.

All three are removed rather than marked deprecated — a field that cannot take effect
should not be part of the API.

The deletion pattern is now a single field named after the API parameter. The first
version of this PR had this backwards and removed nameRegexDelete, the spelling that
does work, which would have stopped sending the pattern altogether. Thanks to
@henrysachs for catching it. Request bodies captured through the real client:

• NameRegexDelete set → {"name_regex":"delete-me-.*","name_regex_delete":"delete-me-.*"}
• NameRegex set → container_expiration_policy_attributes empty, no pattern sent

nameRegexDelete is therefore renamed to nameRegex, matching both the GitLab parameter
and the observation field, and the converter writes it to client-go's NameRegexDelete so
the library forwards it. That removes the last snake_case key from this CRD, and it also
removes an SA1019 suppression on the write path that was telling the truth.

Breaking change

The Project spec no longer accepts buildCoverageRegex, operationsAccessLevel or
containerExpirationPolicyAttributes.name_regex, and
containerExpirationPolicyAttributes.nameRegexDelete is renamed to nameRegex.

Remove the dropped fields from manifests and Composition output before upgrading. Strict
kubectl apply and server-side apply reject unknown fields rather than pruning them, and
because Crossplane applies composed resources via SSA, a Composition that still emits one
of them stops reconciling the whole claim.

The rename of nameRegexDelete is mechanical, the behaviour is unchanged. Moving a value
from name_regex to nameRegex is not: that pattern never took effect, and under the new
name it will start deleting matching tags.

The two removed parameters were still accepted on older GitLab — operations_access_level
up to 15.x, build_coverage_regex in 14.x. The provider does not document a minimum
GitLab version; on those releases these fields stop being sent.

Follow-up, not in this PR

GitLab split operations_access_level into releases, environments, feature_flags,
infrastructure and monitor access levels. This API exposes none of them, so those
features currently cannot be controlled at all.

How has this code been tested

make generate is reproducible, and go build ./..., go test ./... and golangci-lint
are clean.

The new containerexpirationpolicy_wire_test.go drives the real CreateProject and
EditProject against an httptest server and asserts name_regex in the encoded request
body. The existing tests compare Generate*Options output, which is the state before
client-go rewrites the field, and cannot catch this class of bug. Reverting the converter
to NameRegex makes both new tests fail with an empty attributes object.

The removals were checked against GitLab source at a current tag — REST params, entity,
model and GraphQL type — and against a live self-managed EE instance, where
name_regex_delete is discarded while name_regex is applied.

I have:

• Read and followed Crossplane's contribution process.
• Run make generate and golangci-lint to ensure this PR is ready for review.

[henrysachs]

Hi Markus, thank you so much for this, and for the depth of research behind it. Tracing build_coverage_regex and operations_access_level through GitLab's own params, entities and unexposed_attributes, and spotting that name_regex_delete only exists in the docs, is exactly the kind of digging this provider needs. Your caution about not activating a dormant deletion pattern is the right instinct, too.

While reviewing I ran into one thing in client-go that I think flips the conclusion for the third field, plus a couple of smaller points. I'd love to sort these out before we merge.

Before merging

1. client-go rewrites the regex before sending, so nameRegexDelete is the field that actually works

• client-go v2.64.0 overwrites ContainerExpirationPolicyAttributes.NameRegex with NameRegexDelete right before encoding.
  • This happens in CreateProject (projects.go:1053) and in EditProject (projects.go:1224).
  • The field doc says it is "internally hardwired" to NameRegexDelete.
• On master, nameRegexDelete therefore goes out as name_regex and takes effect.
• The snake_case name_regex spec field is the one that never did anything, because client-go replaces it with nil.
• With this PR only NameRegex is mapped (pkg/namespaced/clients/gitlab.go:141). client-go clears it, and no regex is sent at all. Request bodies captured with httptest and the real client-go:
  • master, nameRegexDelete set: {"name_regex_delete":"…","name_regex":"…"}
  • this PR, name_regex set: no regex in container_expiration_policy_attributes
• The regression would be silent:
  • The attributes aren't part of isProjectUpToDate, so the resource stays Ready.
  • The unit tests compare Generate*Options output, which is built before client-go rewrites it.
• Your dormant-pattern concern still applies, just to the other field. Wiring up name_regex would activate values that never took effect.
• Suggestion:
  • Keep nameRegexDelete and map it to both NameRegexDelete and NameRegex, so it keeps working if client-go ever drops the copy.
  • Remove the snake_case name_regex instead, alongside buildCoverageRegex and operationsAccessLevel. That keeps the PR's intent of dropping what can't take effect, and existing cleanup configs keep working.
  • Add a small test that runs the real CreateProject/EditProject against httptest and asserts name_regex in the body. That would also guard against client-go changes.
• The comments at project_types.go:148-154 and gitlab.go:137-140, and the description and commit body, would need to follow.

2. Upgrade note: removed fields are rejected, not just pruned

I checked this on envtest and kind:

How the manifest is applied	Result
Stored objects, read back	Removed fields pruned
PUT/patch with default validation	Pruned, with a warning
kubectl apply (default --validate=strict)	Fails: strict decoding error: unknown field "spec.forProvider.buildCoverageRegex"
Server-side apply	Fails with HTTP 500 field not declared in schema, regardless of fieldValidation, even Ignore

Crossplane applies composed resources via SSA and only tolerates IsInvalid errors (crossplane v2.4.0 composition_functions.go:689-716). A Composition that still emits one of these fields would stop reconciling the whole XR.

I don't think we need a deprecation period for a v1alpha1 API, as long as the fields we drop are ones that never took effect. A clear note in the description and commit body should cover it, for example:

buildCoverageRegex, operationsAccessLevel and containerExpirationPolicyAttributes.name_regex are removed. Remove them from manifests and Composition output before upgrading; strict kubectl apply and server-side apply (including Crossplane, Flux and Argo CD in SSA mode) reject unknown fields. name_regex never took effect: if you move its value to nameRegexDelete, it will start deleting matching tags.

Smaller things

3. Older GitLab. operations_access_level was accepted up to 15.x, and build_coverage_regex was still declared in 14.10. Since we don't document a minimum GitLab version, a sentence in the description saying this drops them for those versions would help.

4. Doc line. The ProjectParameters.ContainerExpirationPolicyAttributes comment says "Accepts: … nameRegex (string)". That was already off before this PR, but since the PR touches that line, it would be nice to fix it there.

5. PR template. The PR body is missing the template's "How has this code been tested" section.

What I checked and found correct

• GitLab v19.4.1 declares none of build_coverage_regex, operations_access_level or name_regex_delete.
• Renaming the observation field to nameRegex is right: GitLab returns name_regex, and the old status field was always empty.
• Codegen is in sync, lint and tests are clean, and it merges cleanly with master, with no interaction with fix(project): stop stamping mirror_user on non-mirrored projects #415.

Thanks again for taking this on. Happy to help with any of it.

[markussiebert]

Thank you — point 1 is correct and I had it backwards. I reproduced every step before changing anything.

The rewrite is unconditional in client-go v2.64.0 (CreateProject, EditProject) and in v3.15.0 it happens in three places, CreateProjectForUser as well. Driving the real client against an httptest server:

• NameRegexDelete set → {"name_regex":"delete-me-.*","name_regex_delete":"delete-me-.*"}
• NameRegex set → container_expiration_policy_attributes empty, nothing sent

So nameRegexDelete was the working spelling and the snake_case name_regex field was the dead one, exactly as you describe. My mistake was carrying an observation from the raw REST API — GitLab discards name_regex_delete — over to the client-go field, which the library translates.

I fixed it slightly differently from your suggestion, on two sub-points.

The surviving field is named nameRegex, not nameRegexDelete. GitLab's parameter is name_regex, the observation field this PR introduces is already nameRegex, and name_regex was the only snake_case key in the CRD. Keeping nameRegexDelete would carry a name into our public API that no GitLab version implements, and leave spec and status disagreeing about the same value. The rename is mechanical for existing users and the behaviour is identical; the upgrade note calls it out separately from the name_regex removal, because moving a value out of name_regex does change behaviour and starts deleting tags.

The converter writes only NameRegexDelete, not both fields. Writing both would not actually protect us against the change you have in mind: the comment in client-go says the copy should be deleted when NameRegex is removed, so the plausible future is that both the copy and the field disappear together. At that point assigning NameRegex would not compile, while NameRegexDelete alone would quietly go out as name_regex_delete and be discarded by GitLab. Only a wire-level assertion catches that, so I took the test you suggested as the guard instead of the second assignment. containerexpirationpolicy_wire_test.go runs the real CreateProject and EditProject and asserts name_regex in the encoded body; reverting the converter makes both tests fail with an empty attributes object.

Happy to switch to nameRegexDelete or add the second assignment if you still prefer either — neither is a hill I want to die on.

Points 2 to 5 are in: the upgrade note is in the description and the commit body, including that strict apply and SSA reject rather than prune and what that means for Compositions; the sentence about older GitLab is there; the ContainerExpirationPolicyAttributes doc comment is accurate now that the field really is nameRegex; and the missing template section is filled in.

One side effect worth noting: the SA1019 suppression on that converter line is gone, because NameRegexDelete carries no deprecation marker. That warning was a true positive and I had silenced it — your review found the bug that it was pointing at.

I also need to correct my upstream report. client-go#2303 claims that setting NameRegexDelete is a silent no-op, which is wrong for the write struct. The read-side part stands — GitLab never returns name_regex_delete, so ContainerExpirationPolicy.NameRegexDelete is always empty and the deprecation on the read field points at nothing.

I have not verified your envtest and kind results on SSA myself; I have no cluster set up for it at the moment. Everything else in your checklist matches what I measured.

[markussiebert]

Update on the upstream side: client-go!3080 landed and closed #2303, released in v3.16.2. It swaps the annotations — NameRegex is no longer deprecated, NameRegexDelete is — and replaces the unconditional overwrite with a fallback that only copies NameRegexDelete into NameRegex when NameRegex is unset.

That does not change anything in this PR. The fix is v3-only, the v2 line is frozen at v2.64.0 (tagged a month earlier), and this branch builds against v2.64.0, where the overwrite still applies. Verified against v3.17.0 that the mapping chosen here keeps working on both lines:

• NameRegexDelete only → {"name_regex":"regex-from-delete","name_regex_delete":"regex-from-delete"}
• NameRegex only → {"name_regex":"regex-from-name-regex"} on v3.16.2+, dropped entirely on v2.64.0

So writing NameRegexDelete is the portable choice and stays correct after the v3 bump. What changes there is cosmetic: NameRegexDelete becomes the deprecated field, so the mapping moves to NameRegex to avoid a new suppression, and the read-side suppression this PR adds is dropped because that annotation is gone. Both are part of the client-go v3 PR, and the wire test added here covers either mapping, so it needs no change.

One thing worth recording: the SA1019 on the converter line was a true positive, and the first version of this PR carried the suppression forward while changing the field underneath it. That is how the regression got in. Thanks again for catching it.

[henrysachs]

Thanks, Markus, great follow-up, and nice work getting the fix landed upstream. I checked v3.17.0 and the wire test, both behave as you describe. Looks good to me!