Add error condition when notification replica is greater than 1 #1642
Conversation
Signed-off-by: Rizwana777 <[email protected]>
| if argocd.Spec.Notifications.Replicas != nil && *argocd.Spec.Notifications.Replicas > 1 { | ||
| message := "Argo CD Notification controller does not support multiple replicas. Notification replicas cannot be greater than 1." | ||
| reqLogger.Info(message) | ||
| return reconcile.Result{}, argocd, fmt.Errorf("%s", message) |
There was a problem hiding this comment.
Will this stop the reconciliation completely if there is problem with notifications configurations?
There was a problem hiding this comment.
What do you think about ignoring the replica field value for notifications and throwing a warning message instead of stopping the entire reconciliation process due to a single unsupported configuration? Since this is not yet supported upstream, we could inform users through a warning message and maybe also update our documentation accordingly to reflect this.
There was a problem hiding this comment.
i agree, but in ticket they mentioned to report an error and stop the reconciliation
There was a problem hiding this comment.
Oh, I see. I think we have the flexibility to decide the behavior here. Personally, I don't think it’s appropriate to stop reconciliation entirely for a minor non-critical misconfiguration. Ignoring the unsupported config and notifying the user should be enough, I feel.
Additionally, if upstream has no plans to support replicas, we could also consider deprecating the field in the future.
There was a problem hiding this comment.
yes, then I will just log the error and move forward
There was a problem hiding this comment.
Cool. Lets also ignore the value if set.
There was a problem hiding this comment.
if we are just giving the warning , then there is no need to update the condition field of ArgoCD with error type ErrorOccurred as we will not return any error here https://github.com/argoproj-labs/argocd-operator/pull/1642/files#diff-2fe0a2bb5baaf6170a23a7c08adc70117adfa25b7d481923aa8626a38cefd447R137?
Signed-off-by: Rizwana777 <[email protected]>
|
I think we also need to ignore the value set in replica field here argocd-operator/controllers/argocd/notifications.go Lines 341 to 343 in 5cce9ce Maybe, log the message in this if condition and remove the assignment statement to ignore the value. WDYT? |
I think we can remove or ignore this function itself getArgoCDNotificationsControllerReplicas if we are removing assignment statement to ignore the value, it will be of no use calling this function if we are not using the value returned by it right? and I see this function not used in any other place as well? |
|
Yes. Sounds good. |
JIRA - https://issues.redhat.com/browse/GITOPS-5967