Skip to content

fix(flagger): make strategy post-renderer robust to the missing chart strategy block - #1938

Closed
devantler wants to merge 1 commit into
mainfrom
claude/fix-flagger-strategy-postrender
Closed

devantler wants to merge 1 commit into
mainfrom
claude/fix-flagger-strategy-postrender

Conversation

@devantler

Copy link
Copy Markdown
Contributor

Problem

The System Test fails on every k8s PR (surfaced on #1929, but not caused by it). The flagger HelmRelease install fails at post-render:

Helm install failed for release flagger-system/flagger with chart flagger@1.43.0:
error while running post render on manifests: replace operation does not apply:
doc is missing path: /spec/strategy/type: missing value

…which wedges infrastructure-controllers → infrastructure → apps.

Root cause (regression from #1900)

#1900 enabled flagger leaderElection for HA. The flagger chart only renders strategy.type: Recreate when leaderElection is disabled; with it enabled the chart omits the strategy block entirely. The existing post-renderer used op: replace /spec/strategy/type, and replace fails when the target path doesn't exist — so the install now dies deterministically.

(#1900's own System Test presumably passed before the leaderElection commit landed, or merged via the merge queue, which skips the docker System Test.)

Fix

Replace the replace+add pair with a single op: add /spec/strategy for the whole strategy object. /spec always exists on a Deployment, so add creates-or-replaces deterministically whether or not the chart renders a strategy block — and still pins the explicit RollingUpdate defaults so drift detection (which doesn't ignore /spec/strategy) sees no diff. The comment is updated to reflect the leaderElection interaction.

Validation

  • ksail workload validate: local 0 failures; prod unchanged (only the pre-existing coroot CRD-catalog schema flake). Both provider controller overlays build.
  • The post-render itself is only exercisable in the System Test (it runs at Helm install time) — this PR's System Test is the real confirmation. op: add on /spec cannot raise the "missing path" error by construction.

Impact

Unblocks the System Test for all open k8s PRs (#1927/#1928 already merged; this restores green for #1929, #1931, and the merge queue).

🤖 Generated with Claude Code

… strategy block

#1900 enabled flagger `leaderElection`. The chart only renders
`strategy.type: Recreate` when leaderElection is DISABLED; with it enabled the
chart omits the strategy block entirely. The strategy post-renderer used
`op: replace /spec/strategy/type`, which now fails at Helm install with:

  error while running post render on manifests: replace operation does not
  apply: doc is missing path: /spec/strategy/type: missing value

That fails the flagger HelmRelease install and wedges infrastructure-controllers
(→ infrastructure → apps), failing the System Test on every k8s PR.

Replace the `replace`/`add` pair with a single `op: add /spec/strategy` for the
whole strategy object. `/spec` always exists on a Deployment, so `add`
creates-or-replaces deterministically whether or not the chart renders a strategy
block — and still pins the explicit RollingUpdate defaults so drift detection
(which does not ignore /spec/strategy) sees no diff.

Validated: ksail workload validate passes (local 0 failures; prod's only failure
is the pre-existing coroot CRD-catalog schema flake); both provider controller
overlays build. The post-render itself is exercised by the System Test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-project-automation github-project-automation Bot moved this to 🫴 Ready in 🌊 Project Board Jun 9, 2026
@devantler
devantler marked this pull request as ready for review June 9, 2026 19:51
@devantler
devantler enabled auto-merge June 9, 2026 19:51
@devantler
devantler added this pull request to the merge queue Jun 9, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Jun 9, 2026
@devantler
devantler enabled auto-merge June 9, 2026 20:31
@devantler

Copy link
Copy Markdown
Contributor Author

Closing as redundant — main already has this exact fix.

#1901 (the VPA/HA follow-up) independently caught the same #1900 regression and applied the identical post-renderer change — op: add /spec/strategy with the same {type: RollingUpdate, rollingUpdate: {maxSurge: 25%, maxUnavailable: 25%}} values (verified byte-identical patch ops). The "merge conflict" here is just both branches changing the same lines to the same thing.

Net: nothing to merge from this PR. The flagger install regression is already resolved on main, which should unblock the System Test for the remaining k8s PRs (#1929, #1931).

@devantler devantler closed this Jun 9, 2026
auto-merge was automatically disabled June 9, 2026 20:33

Pull request was closed

@github-project-automation github-project-automation Bot moved this from 🫴 Ready to ✅ Done in 🌊 Project Board Jun 9, 2026
@devantler
devantler deleted the claude/fix-flagger-strategy-postrender branch June 9, 2026 20:33

This branch was previously deployed

1 inactive deployment
ci — e9c8c43b Deployed Jun 9, 2026 by devantler via 🧪 System Test #2412
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

1 participant