Skip to content
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

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

jaideepr97
Copy link
Collaborator

@jaideepr97 jaideepr97 commented Sep 27, 2022

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 chore
/kind cleanup
/kind failing-test
/kind enhancement
/kind documentation
/kind code-refactoring

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?

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

Which issue(s) this PR fixes:

Fixes #785
https://issues.redhat.com/browse/GITOPS-2230

How to test changes / Special notes to the reviewer:

  1. Run the operator against a cluster
  2. Create an argo-cd instance and add .spec.dex.config: test-config
  3. run kubectl get events against that namespace
  4. Only single occurrence of deprecation notice event should be present

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
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, thanks

@@ -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 {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

@jopit jopit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jopit jopit merged commit 175fa61 into argoproj-labs:master Sep 28, 2022
@iam-veeramalla iam-veeramalla added this to the 0.5.0 milestone Dec 8, 2022
@jaideepr97 jaideepr97 deleted the update-sso-event branch December 22, 2022 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation warning event spamming and incomplete association with operand
3 participants