Skip to content

feat: support merging multiple NotificationsConfiguration CRs per namespace - #1488

Open
Mangaal wants to merge 7 commits into
redhat-developer:masterfrom
Mangaal:notification-controller
Open

Mangaal wants to merge 7 commits into
redhat-developer:masterfrom
Mangaal:notification-controller

Conversation

@Mangaal

@Mangaal Mangaal commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Previously, each CR reconciliation could replace the entire ConfigMap, overwriting configuration contributed by other CRs. This PR merges multiple NotificationsConfiguration CRs in the same namespace into ArgoCD argocd-notifications-cm configmap.

Merge behavior:

  • Triggers, templates, services, and additional subscription configuration are merged by key.
  • Context entries are merged into one context YAML value.
  • Subscription lists are concatenated, preserving their order and duplicates.
  • Invalid key entries and malformed subscriptions are skipped and logged. Other valid entries from the same CR are retained.

Conflict resolution:

  • When CRs define different values for the same key, the oldest CR wins.
  • If the oldest CRs have equal timestamps and conflicting values, retain the existing ConfigMap value only if it matches one of those CRs. Otherwise, omit the key.
  • Equal-timestamp conflicts are logged as errors without exposing configuration values.
  • Identical values are recorded as duplicates and are not treated as errors.

What type of PR is this?

/kind enhancement

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
@openshift-ci

openshift-ci Bot commented Nov 2, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci Bot added kind/bug Something isn't working kind/cleanup Categorizes issue or PR as related to cleaning up code, process. labels Nov 2, 2026
@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review.

Summary

Summary by CodeRabbit

  • New Features
    • Multiple notifications configurations in a namespace are now combined into a shared notifications configuration. Older configurations take precedence when values conflict, while subscription lists are combined in order.
    • Changes to configurations are reflected in the shared configuration, and it is removed when no active configurations remain.
  • Bug Fixes
    • Invalid or conflicting entries are handled without preventing valid settings from being applied. Existing configuration data and unrelated metadata are preserved when no update is needed.

The notifications-configuration controller now merges active namespace-scoped resources into one shared ConfigMap. It applies precedence rules and manages ConfigMap creation, updates, migration, and deletion. Argo CD reconciliation adds the operator tracking label to notifications ConfigMaps in the Argo CD and source namespaces.

Changes: Notifications configuration aggregation

Layer / File(s) Summary
Merge configuration data
argocd-operator/controllers/notificationsconfiguration/merge.go, argocd-operator/controllers/notificationsconfiguration/merge_test.go
The merge combines configuration entries by precedence and records conflicts, duplicates, and skipped entries. It also handles context serialization and subscription-list validation. Tests cover precedence, ties, reserved keys, and invalid entries.
Reconcile the shared ConfigMap
argocd-operator/controllers/notificationsconfiguration/notificationsconfiguration_controller.go, argocd-operator/controllers/notificationsconfiguration/configmap.go, argocd-operator/controllers/notificationsconfiguration/reconcile.go, argocd-operator/controllers/notificationsconfiguration/configmap_test.go
The controller maps resource and ConfigMap events to namespace reconciliation requests. It merges active resources and creates, updates, or deletes the shared ConfigMap. Helpers enforce tracking-label and owner-reference checks. Unit tests cover reconciliation, lifecycle, and ownership cases.
Validate controller behavior in envtest
argocd-operator/controllers/notificationsconfiguration/integration_test.go
The integration test covers startup recovery, manager downtime, live resource events, ConfigMap correction and recreation, and deletion.
Track ConfigMaps during Argo CD reconciliation
argocd-operator/controllers/argocd/notifications.go, argocd-operator/controllers/argocd/notifications_test.go
Argo CD reconciliation adds the operator tracking label to notifications ConfigMaps in the Argo CD and source namespaces. Tests check label preservation and repeated reconciliation.

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Events as NotificationsConfiguration events
  participant Reconciler as NotificationsConfigurationReconciler
  participant Merger as MergeNotificationsConfigurations
  participant API as Kubernetes API
  Events->>Reconciler: enqueue namespace reconcile key
  Reconciler->>API: list namespace CRs and get shared ConfigMap
  Reconciler->>Merger: merge CRs with existing data
  Merger-->>Reconciler: merged data and diagnostics
  Reconciler->>API: create, update, or delete shared ConfigMap
Loading

Suggested reviewers: olivergondza, jgwest, anandrkskd

Merge Risk: Minimal · previous review a69bb

This change merges multiple NotificationsConfiguration resources in a namespace into one shared notifications ConfigMap. The supplied summaries show no concrete merge-blocking risk, and the new unit and integration tests cover the main lifecycle and migration paths.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 54.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the main change: support for merging multiple NotificationsConfiguration CRs within one namespace.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description directly explains the multi-CR ConfigMap merge behavior, conflict resolution, context merging, subscription handling, and invalid-entry handling implemented by the changeset.
Full details: Docstring Coverage

Explanation

Docstring coverage is 54.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 24 functions across 8 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between c687c9a and a69bb0d.

Files selected for processing: 9
  • argocd-operator/controllers/argocd/notifications.go
  • argocd-operator/controllers/argocd/notifications_test.go
  • argocd-operator/controllers/notificationsconfiguration/configmap.go
  • argocd-operator/controllers/notificationsconfiguration/configmap_test.go
  • argocd-operator/controllers/notificationsconfiguration/integration_test.go
  • argocd-operator/controllers/notificationsconfiguration/merge.go
  • argocd-operator/controllers/notificationsconfiguration/merge_test.go
  • argocd-operator/controllers/notificationsconfiguration/notificationsconfiguration_controller.go
  • argocd-operator/controllers/notificationsconfiguration/reconcile.go
Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

Files with no reviewable changes: 1
  • argocd-operator/controllers/notificationsconfiguration/reconcile.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

@openshift-ci openshift-ci Bot added the kind/failing-ci OpenShift CI failure label Nov 2, 2026
@openshift-ci

openshift-ci Bot commented Nov 2, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jannfis for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added kind/failing-test Categorizes issue or PR as related to a frequently failing test. kind/enhancement New feature or request kind/documentation Improvements or additions to documentation labels Nov 2, 2026
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
@Mangaal
Mangaal marked this pull request as ready for review November 3, 2026 06:56
@Mangaal Mangaal changed the title Multi Notifcation CR support Support merging multiple NotificationsConfiguration CRs per namespace Nov 3, 2026
@Mangaal Mangaal changed the title Support merging multiple NotificationsConfiguration CRs per namespace feat: support merging multiple NotificationsConfiguration CRs per namespace Nov 3, 2026
@Mangaal

Mangaal commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

/test

@Mangaal

Mangaal commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

/test all

@openshift-ci

openshift-ci Bot commented Nov 4, 2026

Copy link
Copy Markdown

@Mangaal: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v4.14-kuttl-parallel 684e0d6 link false /test v4.14-kuttl-parallel
ci/prow/v4.14-kuttl-sequential 684e0d6 link false /test v4.14-kuttl-sequential
ci/prow/v4.14-e2e 684e0d6 link false /test v4.14-e2e
ci/prow/v4.19-e2e 684e0d6 link true /test v4.19-e2e
ci/prow/v4.19-kuttl-parallel 684e0d6 link true /test v4.19-kuttl-parallel
ci/prow/v4.19-kuttl-sequential 684e0d6 link true /test v4.19-kuttl-sequential

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@jgwest jgwest removed kind/bug Something isn't working kind/documentation Improvements or additions to documentation kind/failing-ci OpenShift CI failure kind/failing-test Categorizes issue or PR as related to a frequently failing test. labels Nov 4, 2026
@jgwest jgwest removed the kind/cleanup Categorizes issue or PR as related to cleaning up code, process. label Nov 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants