fix(platform): correct what disabledPackages does to ouroboros - #4098

Verdict

LGTM with non-blocking notes

The comments now say what the code does. disabledPackages gates nothing except the {{- if not (has $name $disabled) }} around the Package document in _helpers.tpl, helm.sh/resource-policy: keep is unconditional on both emitters, and nothing between the platform chart and helm-controller uninstalls a release when the document stops rendering. I checked the reverse reading too, that the code drifted and the old prose was the intended contract. It is the other way round. The keep annotation went in deliberately in 1.1.5 and 1.2.1 (#2273, #2297/#2298), and that changelog entry says in as many words that the deletion behaviour was what contradicted the documented contract. This restores it.

Three MINOR notes, none blocking.

Findings

• [MINOR] packages/core/platform/tests/bundles_proxy_protocol_lookup_test.yaml:55, the absence asserts in this file and its siblings pass with the forbidden document present
• [MINOR] packages/core/platform/values.yaml:271, deleting the Package CR is not the only route to the pre-delete hook
• [MINOR] packages/core/platform/values.yaml:257, the vendored ouroboros version in these comments is a release behind

Still open

The test you added is not vacuous: it asserts failedTemplate with an error pattern, and it goes red when the guard stops firing. Its neighbours are another matter. The sweep in the first finding is worth doing on its own account: right now no assert anywhere in this chart would notice bundles.disabledPackages regressing.

One corner the guard does not cover, and your new comments describe it correctly. Leave proxyProtocol at true and add cozystack.ouroboros to disabledPackages: the document stops rendering, keep retains the live Package, ouroboros keeps running, and the guard never fires because it keys on not .Values.publishing.proxyProtocol. Benign in itself, since the ingress side stays configured for PROXY protocol, but it is a second route to "the operator thinks the package is gone and it is not" with no acknowledgement gate on it.

Left unverified

The uninstall chain the comments assert is intact on a static reading only. internal/operator/package_reconciler.go:290 puts a controller ownerReference from the Package onto the HelmRelease, and nothing under internal/operator/, api/ or packages/core/platform/sources/ sets uninstall or DisableHooks, so Flux's hooks-enabled default applies and the chart's helm.sh/hook: pre-delete Job should fire. I did not run that against a cluster.

Findings not anchored to changed lines

These reference code outside this PR's diff (unchanged files, or lines outside a hunk), so GitHub cannot render them inline.

[MINOR] packages/core/platform/tests/bundles_proxy_protocol_lookup_test.yaml:55 the absence asserts in this file and its siblings pass with the forbidden document present

The new case you added is real coverage. The ones next to it are not. containsDocument with not: true scoped by documentIndex: -1 only searches the last rendered document, and the sibling asserts that omit any: entirely search only the first, so a cozystack.ouroboros (or cozystack.velero) Package rendered anywhere in the middle satisfies the negation. Five asserts across bundles_proxy_protocol_lookup_test.yaml:55-60, bundles_proxy_protocol_lookup_nil_test.yaml:31, bundles_proxy_protocol_test.yaml:21,153,170 and bundles_velero_default_test.yaml:36-45 have that shape. Concretely: the test titled "bundles.disabledPackages suppresses cozystack.ouroboros even when proxyProtocol is true" cannot fail if suppression stops working, which is the exact mechanism this PR's new comments assert. I made disabledPackages inert in the emitter and the whole suite stayed green:

$ review-helper mutate . --mutations mut-disabled-inert-full.json --test 'cd packages/core/platform && helm unittest .'
  make bundles.disabledPackages inert in the Package emitter GAP: the suite stayed green without the fix
      file: packages/core/platform/templates/_helpers.tpl
      | Charts:      1 passed, 1 total
      | Test Suites: 34 passed, 34 total
      | Tests:       156 passed, 156 total

(mutation: {{- if not (has $name $disabled) -}} becomes {{- if true -}} in templates/_helpers.tpl:10)

Control, same mutation with any: true moved inside the matcher on the velero assert:

  CONTROL: disabledPackages inert AND the velero absence assert scoped with any:true covered: the suite went red
      | 			DocumentIndex:	28
      | 			Expected NOT to contain document:
      | 				Kind = Package, apiVersion = cozystack.io/v1alpha1, Name = cozystack.velero
      | Tests:       1 failed, 1 passed, 2 total

And the same control on the file you touched, injecting the forbidden Package by flipping that case to proxyProtocol: true: the current documentIndex: -1 shape stays green (Tests: 4 passed), the any: true shape goes red at DocumentIndex: 15. Fix is mechanical, drop documentIndex: -1 and put any: true inside the containsDocument: map as a sibling of apiVersion/kind/name:

      - containsDocument:
          apiVersion: cozystack.io/v1alpha1
          kind: Package
          name: cozystack.ouroboros
          any: true
        not: true

Kept at MINOR because this PR's own required regression test is non-vacuous and the behaviour these asserts fail to pin is unchanged here. It rises if you consider the suppression assertion load-bearing for the comments this PR just wrote.