Added command check when namespace-scoped Argo CD instance in testing

[nmirasch]

What type of PR is this?

/kind enhancement

What does this PR do / why we need it:
Added Controller command check at ginkgo testing of notifications in any namespace

Summary by CodeRabbit

• Tests

  • Strengthened validation tests for notification controller deployments to assert the absence of an unintended command flag, with robust polling/timeouts.

• Style

  • Minor comment grammar correction in notification controller code (non-functional).

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR fixes a comment typo and replaces placeholder/no-op assertions with real test checks that verify the notifications controller deployment command does not include the --application-namespaces flag.

Changes

Cohort / File(s)	Summary
Grammar fix controllers/argocd/notifications.go	Corrected comment wording from "cleansup resources" to "cleanup resources".
Test assertions tests/ginkgo/parallel/1-055_validate_notification_controller_test.go	Added strings import and replaced placeholder assertions with active checks that re-fetch the notifications controller Deployment, join container Command slices, and assert the absence of --application-namespaces (2-minute timeout, 5s polling) in both relevant test cases.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–12 minutes

• Review focus: validate test logic, ensure the Deployment re-fetch and command-joining are correct and resilient to nil containers.

Possibly related PRs

• fix: namespace validation for notification #1955 — Related namespace-scoped behavior preventing --application-namespaces flag in notifications controller.
• fix: Add rbac & tests for notifications in any namespace #1940 — Related modifications to notifications controller/deployment args validated by these tests.

Suggested reviewers

• svghadi
• jgwest

Poem

🐰 I hopped through comments, fixed a small typo,
I bounced into tests to make assertions fly,
No stray flags in sight, I inspect with delight,
Containers and commands all tidy and tight,
A tiny rabbit clap—code ready for flight! 🎉

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title accurately describes the main change: adding a command check to the notification controller tests for namespace-scoped Argo CD instances.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d498b8a and 5a8e196.

📒 Files selected for processing (2)

• controllers/argocd/notifications.go (1 hunks)
• tests/ginkgo/parallel/1-055_validate_notification_controller_test.go (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: build
• GitHub Check: build-operator
• GitHub Check: Run end-to-end tests (v1.27.1)
• GitHub Check: Run golangci-lint and gosec

🔇 Additional comments (2)

tests/ginkgo/parallel/1-055_validate_notification_controller_test.go (2)

21-21: LGTM!

The strings import is correctly added to support the command verification logic below.

---

213-224: LGTM! Test correctly validates namespace-scoped behavior.

The test appropriately verifies that the --application-namespaces flag is absent from the notifications controller deployment command for namespace-scoped Argo CD instances. The implementation:

• Properly handles errors and container existence checks
• Correctly constructs the command string for verification
• Uses appropriate timeout and polling intervals

This aligns with the controller logic in getNotificationsCommand which only adds the flag when IsNamespaceClusterConfigNamespace returns true.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix incomplete grammar in the comment.

The comment uses "cleanup resources" which is grammatically incorrect. It should be "cleans up resources" to match standard Go documentation style where the comment completes the sentence starting with the function name.

Apply this diff:

-// removeUnmanagedNotificationsSourceNamespaceResources cleanup resources from NotificationsSourceNamespaces if namespace is not managed by argocd instance.
+// removeUnmanagedNotificationsSourceNamespaceResources cleans up resources from NotificationsSourceNamespaces if namespace is not managed by argocd instance.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In controllers/argocd/notifications.go around line 917, update the function
comment that currently reads
"removeUnmanagedNotificationsSourceNamespaceResources cleanup resources from
NotificationsSourceNamespaces if namespace is not managed by argocd instance."
to correct grammar and follow Go doc style by changing it to
"removeUnmanagedNotificationsSourceNamespaceResources cleans up resources from
NotificationsSourceNamespaces if the namespace is not managed by the Argo CD
instance."

[svghadi]

We need to skip this check. The application controller doesn't come up due to rbac issues when sourceNamespaces is set for namespaced scope instance.

[nmirasch]

after the fix for the namespaces validation it works as expected and comes up, that's the reason why I restored this line.

[svghadi]

oh okay. Good to know.