fix: honor init container order in spec.deployment.patch [RHDHBUGS-2900]

[rm3l]

Description

spec.deployment.patch did not allow users to control the ordering of init containers. New init containers would always be appended after existing ones (e.g., install-dynamic-plugins always ran first).

This PR switches the kyaml merge strategy so new list items are prepended before existing ones by default. Users who need a custom init container to run after an existing one can still anchor it by referencing the existing container by name first in the patch (this didn't work with the default merge strategy).

Which issue(s) does this PR fix or relate to

• Fixes https://redhat.atlassian.net/browse/RHDHBUGS-2900

PR acceptance criteria

• Tests
• Documentation

How to test changes / Special notes to the reviewer

Note that a two-pass merge is used here to work around kubernetes-sigs/kustomize#6146, where the prepend strategy silently breaks $patch directives. Once that issue is fixed in kyaml, we'll be able to remove this two-pass merge and only use the Prepend strategy.

[rhdh-qodo-merge]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

✅ Tickets: 🎫 `spec.deployment.patch` does not honor the order of init containers; install-dynamic-plugins always runs first

1. Double merge every reconcile 🐞 Bug ➹ Performance

Description

setDeployment now runs two merge2.MergeStrings passes whenever spec.deployment.patch is set,
doubling YAML parse/merge work for every reconcile. This is intentional as a workaround, but it
still adds ongoing CPU/latency overhead for patched deployments.

Code

pkg/model/deployment.go[R238-243]

+			merged, err := merge2.MergeStrings(string(conf.Raw), string(deplStr), false, kyaml.MergeOptions{ListIncreaseDirection: kyaml.MergeOptionsListPrepend})
+			if err != nil {
+				return fmt.Errorf("can not merge spec.deployment: %w", err)
+			}
+
+			merged, err = merge2.MergeStrings(string(conf.Raw), merged, false, kyaml.MergeOptions{})

Evidence

The PR introduces a second merge call in setDeployment; addToModel invokes setDeployment
during object initialization, which is part of the reconcile/model build path.

pkg/model/deployment.go[85-113]
pkg/model/deployment.go[223-246]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pkg/model/deployment.go` always performs a two-pass merge for `spec.deployment.patch`. This doubles YAML parsing/merging cost on every reconcile even when the patch doesn't need the `$patch` workaround.

### Issue Context
The first pass uses `ListPrepend` to get list ordering; the second pass re-applies the same patch with default options to support `$patch` directives.

### Fix Focus Areas
- pkg/model/deployment.go[223-246]

### Suggested approach
- Detect whether the patch actually contains `$patch` directives (e.g., a cheap substring check on `conf.Raw` for `"$patch"` / `$patch:`) and only run the second pass when needed.
- Optionally, also skip the first pass when the patch does not contain any list fields where ordering matters (if a safe/cheap detection is available), otherwise keep pass1.
- Keep behavior identical for patches that do contain `$patch`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Merge pass not labeled 🐞 Bug ◔ Observability

Description

Both merge passes wrap errors with the same message, so logs don’t indicate whether pass 1
(ListPrepend) or pass 2 (default) failed without digging into the underlying error details. This
complicates debugging operational failures from malformed patches.

Code

pkg/model/deployment.go[R238-246]

+			merged, err := merge2.MergeStrings(string(conf.Raw), string(deplStr), false, kyaml.MergeOptions{ListIncreaseDirection: kyaml.MergeOptionsListPrepend})
+			if err != nil {
+				return fmt.Errorf("can not merge spec.deployment: %w", err)
+			}
+
+			merged, err = merge2.MergeStrings(string(conf.Raw), merged, false, kyaml.MergeOptions{})
			if err != nil {
				return fmt.Errorf("can not merge spec.deployment: %w", err)
			}

Evidence

Both merge calls return the same wrapped error string, even though they run with different merge
options and have different failure modes.

pkg/model/deployment.go[238-246]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Two different merge operations can fail, but both return `fmt.Errorf("can not merge spec.deployment: %w", err)`.

### Issue Context
With the two-pass approach, knowing which pass failed is important for quickly identifying whether the failure is tied to the prepend strategy or to `$patch` directive handling.

### Fix Focus Areas
- pkg/model/deployment.go[238-246]

### Suggested approach
- Change the wrapped error messages to include the pass number and merge mode, e.g.:
 - `can not merge spec.deployment (pass 1: ListPrepend): %w`
 - `can not merge spec.deployment (pass 2: default): %w`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Order test hard-codes length 🐞 Bug ⚙ Maintainability

Description

TestInitContainerOrderInSpecDeployment asserts the initContainer list length equals the expected
list, so adding any new default initContainer to the base manifest will fail these cases even if the
relative ordering guarantee still holds. This makes the test unnecessarily sensitive to unrelated
default-manifest evolution.

Code

pkg/model/deployment_test.go[R242-246]

+			initContainers := model.backstageDeployment.podSpec().InitContainers
+			assert.Equal(t, len(tt.expected), len(initContainers))
+			for i, name := range tt.expected {
+				assert.Equal(t, name, initContainers[i].Name)
+			}

Evidence

The test currently enforces len(initContainers) == len(expected) before checking name-by-index,
which ties the test to the exact default initContainer count.

pkg/model/deployment_test.go[161-249]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The initContainer ordering test asserts exact slice length, which will break whenever the default deployment gains additional init containers.

### Issue Context
The intent is to validate relative ordering around `install-dynamic-plugins`, not to freeze the entire initContainer inventory.

### Fix Focus Areas
- pkg/model/deployment_test.go[242-246]

### Suggested approach
- Replace the strict length assertion with checks that:
 - required containers exist (e.g., find indices by name), and
 - their indices satisfy the expected ordering (e.g., `idx("my-init") < idx("install-dynamic-plugins")`).
- Keep a focused assertion that `install-dynamic-plugins` remains present when expected.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[rm3l]

/cc @gazarenkov

[github-actions]

PR images are available:

1. https://quay.io/rhdh-community/operator:0.10.0-pr-2824-be5fc88
2. https://quay.io/rhdh-community/operator-bundle:0.10.0-pr-2824-be5fc88
3. https://quay.io/rhdh-community/operator-catalog:0.10.0-pr-2824-be5fc88

[rhdh-qodo-merge]

Review Summary by Qodo

Honor init container order in spec.deployment.patch with two-pass merge

🐞 Bug fix 🧪 Tests 📝 Documentation

Walkthroughs

Description

• Implement two-pass merge strategy to honor init container ordering in spec.deployment.patch
  - Pass 1 uses ListPrepend to prepend new list items before existing ones
  - Pass 2 applies $patch directives to preserve special merge semantics
• Add comprehensive test coverage for init container ordering scenarios
  - Tests prepending before existing containers
  - Tests anchoring to place containers after existing ones
  - Tests multiple containers with mixed ordering
• Document list ordering semantics and provide usage examples
  - Explain prepend behavior for new list items
  - Show workaround to anchor containers after existing ones by name reference

Diagram

flowchart LR
  A["User Patch"] -->|Pass 1: ListPrepend| B["Correct List Order"]
  B -->|Pass 2: Default Options| C["Final Merged Deployment"]
  C -->|Result| D["New containers before existing<br/>$patch directives applied"]

Loading

File Changes

1. pkg/model/deployment.go 🐞 Bug fix +14/-1

Two-pass merge for init container ordering

• Implement two-pass merge strategy for spec.deployment.patch
• First pass uses ListIncreaseDirection: MergeOptionsListPrepend to prepend new list items
• Second pass uses default merge options to apply $patch directives
• Added TODO comment referencing kyaml issue #6146 for future cleanup

pkg/model/deployment.go

---

2. pkg/model/deployment_test.go 🧪 Tests +96/-7

Add init container ordering tests

• Update existing TestMergeFromSpecDeployment assertions to reflect prepend behavior
• Add new TestInitContainerOrderInSpecDeployment with four test cases
• Test cases cover prepending before existing containers, anchoring by name, and mixed ordering
 scenarios
• Verify correct init container order in resulting deployment

pkg/model/deployment_test.go

---

3. docs/configuration.md 📝 Documentation +39/-0

Document list ordering semantics and workarounds

• Add new "List ordering" section explaining prepend behavior for new list items
• Document that items matching by name are merged in-place and retain position
• Provide example of prepending custom init container before install-dynamic-plugins
• Show workaround to anchor containers after existing ones by referencing them by name first

docs/configuration.md

---