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(events): Adds config flag. Reduce number of dupe events emitted. #3205
Conversation
@@ -0,0 +1,9 @@ | |||
package config | |||
|
|||
type NodeEvents struct { |
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 might allow for more complex config later on.
@@ -18,6 +18,14 @@ data: | |||
# (available since Argo v2.3) | |||
parallelism: 10 | |||
|
|||
# Whether or not to emit events on node completion. These can take a up a lot of space in | |||
# k8s (typically etcd) resulting in errors when trying to create new events: | |||
# "Unable to create audit event: etcdserver: mvcc: database space exceeded" |
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 include the specific error message so people Googling for it may find it
command: [sh, -c, sleep 10] | ||
` | ||
|
||
func TestEventTimeout(t *testing.T) { |
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 test was garbage - deleted
@sarabala1979 do you think you might be able to review this in the next day or two please? |
return | ||
} | ||
woc.auditLogger.LogWorkflowEvent(woc.wf, argo.EventInfo{Type: apiv1.EventTypeNormal, Reason: argo.EventReasonWorkflowRunning}, "Workflow Running") | ||
woc.eventRecorder.Event(woc.wf, apiv1.EventTypeNormal, "WorkflowRunning", "Workflow Running") |
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.
can you define the constant for EventName/EventType instead of hardcoded string "WorkflowRunning"?
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 actually inlined the constant as it was only used in one place - no need to be a constant
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
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.Changes
EventRecorder
to batch events.Testing
I've not added an e2e test for the flag, because this is not default behaviour. So this must be tested manually.