-
Notifications
You must be signed in to change notification settings - Fork 763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Update sso eventing logic to stop spam #786
Conversation
62294b8
to
4cef41a
Compare
4cef41a
to
87bb20b
Compare
controllers/argocd/sso.go
Outdated
err := argoutil.CreateEvent(r.Client, "Warning", "Deprecated", "`.spec.SSO.Image`, `.spec.SSO.Version`, `.spec.SSO.Resources` and `.spec.SSO.VerifyTLS` are deprecated, and support will be removed in Argo CD Operator v0.6.0/OpenShift GitOps v1.9.0. Keycloak configuration can be managed through `.spec.sso.keycloak`", "DeprecationNotice", cr.ObjectMeta) | ||
if err != nil { | ||
return err | ||
// Emit event warning users about deprecation notice for `.spec.dex` users if not emitted already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should reference .spec.SSO, not .spec.dex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, thanks
controllers/argoutil/resource.go
Outdated
@@ -52,17 +52,23 @@ func CombineImageTag(img string, tag string) string { | |||
} | |||
|
|||
// CreateEvent will create a new Kubernetes Event with the given action, message, reason and involved uid. | |||
func CreateEvent(client client.Client, eventType, action, message, reason string, meta metav1.ObjectMeta) error { | |||
event := newEvent(meta) | |||
func CreateEvent(client client.Client, eventType, action, message, reason string, ObjectMeta metav1.ObjectMeta, typeMeta metav1.TypeMeta) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why the ObjectMeta
parameter name starts with an uppercase letter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no particular reason
changed it, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What type of PR is this?
/kind bug
What does this PR do / why we need it:
This PR updates the logic for SSO related field deprecations in the CR, so that single event is emitted with latest timestamp and CR association so that no. of emitted events is greatly reduced and events have more metadata stored
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #785
https://issues.redhat.com/browse/GITOPS-2230
How to test changes / Special notes to the reviewer:
.spec.dex.config: test-config
kubectl get events
against that namespace