Issue 4185

[anandrkskd]

User description

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug
/kind cleanup
/kind failing-test
/kind enhancement
/kind documentation
/kind code-refactoring

What does this PR do / why we need it:

Have you updated the necessary documentation?

• Documentation update is required by this PR.
• Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #?

Test acceptance criteria:

• Unit Test
• E2E Test

How to test changes / Special notes to the reviewer:

---

PR Type

Bug fix, Tests

---

Description

• Fix bug where ArgoCD nodePlacement stanza was removed unintentionally

  • Update reconciliation logic to handle nodePlacement correctly
  • Ensure removal only when intended, not by default

• Add unit test to verify correct handling of nodePlacement

  • Test updates via GitopsService CR and expected propagation

• Minor refactoring: unify metav1 import usage in tests

---

Changes walkthrough 📝

	Relevant files
Bug fix	gitopsservice_controller.go Fix and clarify ArgoCD nodePlacement reconciliation logic controllers/gitopsservice_controller.go Fix reconciliation logic for nodePlacement in ArgoCD spec Ensure nodePlacement is only removed when intended Add comments to clarify logic +8/-2
Tests	gitopsservice_controller_test.go Add and refactor tests for ArgoCD nodePlacement handling  controllers/gitopsservice_controller_test.go Add unit test for ArgoCD nodePlacement update via GitopsService CR Refactor to use consistent metav1 import (v1.ObjectMeta) Assert correct propagation of nodeSelector to ArgoCD +59/-4    argocd_metrics_controller_test.go Refactor test to use consistent metav1 import                        controllers/argocd_metrics_controller_test.go Refactor to use consistent metav1 import (v1.ObjectMeta) Minor test code cleanup +1/-2

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[anandrkskd]

/review

[qodo-code-review]

PR Reviewer Guide 🔍

(Review updated until commit 7a345e5)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Conditional Logic The fix adds a conditional check for nodePlacement that preserves user-defined values, but there's an edge case in the else branch. The code always sets NodePlacement to nil and marks as changed, even if it's already nil, which could trigger unnecessary updates. } else if existingArgoCD.Spec.NodePlacement != nil { existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement changed = true }

[qodo-code-review]

Persistent review updated to latest commit 7a345e5

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix nil assignment The code incorrectly sets existingArgoCD.Spec.NodePlacement to nil when defaultArgoCDInstance.Spec.NodePlacement is nil. This is redundant and could cause unintended changes. Only set NodePlacement to nil in the else branch. controllers/gitopsservice_controller.go [474-484] // if user is patching nodePlacement through GitopsService CR, then existingArgoCD NodePlacement is updated. if defaultArgoCDInstance.Spec.NodePlacement != nil { if !reflect.DeepEqual(existingArgoCD.Spec.NodePlacement, defaultArgoCDInstance.Spec.NodePlacement) { existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement changed = true } // Handle the case where NodePlacement should be removed } else if existingArgoCD.Spec.NodePlacement != nil { - existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement + existingArgoCD.Spec.NodePlacement = nil changed = true } Apply / Chat Suggestion importance[1-10]: 3 __ Why: The suggestion proposes a minor style improvement by explicitly setting NodePlacement = nil instead of assigning from a nil value, but incorrectly frames it as fixing "incorrect" behavior when both approaches are functionally equivalent.	Low
General	Add test resource cleanup The test doesn't properly clean up after itself. Add a deferred cleanup to remove the created ArgoCD instance to prevent potential interference with other tests. controllers/gitopsservice_controller_test.go [143-148] // verify whether existingArgoCD NodePlacement is updated when user is patching nodePlacement through GitopsService CR err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, existingArgoCD) assertNoError(t, err) assert.Check(t, existingArgoCD.Spec.NodePlacement != nil) assert.DeepEqual(t, existingArgoCD.Spec.NodePlacement.NodeSelector, gitopsService.Spec.NodeSelector) +// Clean up +err = fakeClient.Delete(context.TODO(), existingArgoCD) +assertNoError(t, err) + Apply / Chat Suggestion importance[1-10]: 4 __ Why: While this cleanup suggestion follows good testing practices, it's not strictly necessary since the test uses a fake.NewFakeClient() which provides isolated test environments. The cleanup adds value for explicit resource lifecycle management but has minimal impact on test functionality.	Low
More

[anandrkskd]

/describe

[qodo-code-review]

PR Description updated to latest commit (7a345e5)

• Copy walkthrough table to "Files Changed" Tab

[Copilot]

Pull Request Overview

This PR fixes a bug where the ArgoCD nodePlacement stanza was removed by default and adds tests to ensure correct handling of user-patched node placement.

• Update reconciliation logic to only remove NodePlacement when explicitly intended
• Add a unit test for patching NodePlacement via the GitopsService CR
• Refactor test imports to use a consistent v1 alias

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
controllers/gitopsservice_controller.go	Fix and clarify ArgoCD NodePlacement reconciliation logic (+8/–2)
controllers/gitopsservice_controller_test.go	Add and refactor tests for ArgoCD NodePlacement handling (+59/–4)
controllers/argocd_metrics_controller_test.go	Refactor test to use consistent v1.ObjectMeta import (+1/–2)

Comments suppressed due to low confidence (3)

controllers/gitopsservice_controller_test.go:95

• [nitpick] The test function name has inconsistent capitalization for 'NodePlacement'; consider renaming to 'TestReconcileDefaultForArgoCDNodePlacement' for clarity.

func TestReconcileDefaultForArgoCDNodeplacement(t *testing.T) {

controllers/gitopsservice_controller.go:481

• There’s no unit test covering the removal branch for NodePlacement; add a test where the CR clears NodeSelector to verify NodePlacement is removed from the ArgoCD spec.

} else if existingArgoCD.Spec.NodePlacement != nil {

controllers/gitopsservice_controller.go:475

• The reconciliation logic reads from defaultArgoCDInstance.Spec.NodePlacement, but should instead reconcile based on the GitopsService CR (e.g., instance.Spec.NodePlacement or instance.Spec.NodeSelector).

if defaultArgoCDInstance.Spec.NodePlacement != nil {