Add initial eventing machinery #56
Conversation
Adds event recorder and emits warning events on errors. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hasheddan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| @@ -133,6 +133,7 @@ github.com/go-logfmt/logfmt v0.4.0/go.mod h1:3RMwSq7FuexP4Kalkev3ejPJsZTpXXBr9+V | |||
| github.com/go-logr/logr v0.1.0 h1:M1Tv3VzNlEHg6uyACnRdtrploV2P7wZqH8BoQMtz0cg= | |||
| github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas= | |||
| github.com/go-logr/zapr v0.1.0/go.mod h1:tabnROwaDl0UNxkVeFRbY8bwB37GwRv0P8lg6aAiEnk= | |||
| github.com/go-logr/zapr v0.1.1 h1:qXBXPDdNncunGs7XeEpsJt8wCjYBygluzfdLO0G5baE= | |||
There was a problem hiding this comment.
hmmm not sure how these go.sum changes happened. make-verify passed locally for me though 🤔
| @@ -116,11 +119,13 @@ func (r *Reconciler) Reconcile(req reconcile.Request) (reconcile.Result, error) | |||
| profilePath, err := getProfilePath(name, profile) | |||
| if err != nil { | |||
| logger.Error(err, "cannot get profile path") | |||
| r.record.Event(profile, event.Warning(event.Reason("cannot get profile path"), err)) | |||
There was a problem hiding this comment.
What's your thoughts on adding this into a new implementation of logger?
There was a problem hiding this comment.
And having a combined logger / eventer? So it would look something like logger.Event()?
There was a problem hiding this comment.
Logging an error automatically throws an event? Sound good! :)
There was a problem hiding this comment.
Hmm I'm not sure we will always want to have our logging and eventing tied together in that manner. @pjbgf did you have a specific use case in mind?
There was a problem hiding this comment.
The idea was just to remove repetition as I can easily imagine those two lines being used quite frequently together.
I thought more like an EventLogger that implemented the same interface as logger but had an extra side effect.
Thinking a bit more about it, this would violate SRP and probably would make things a bit more complex for new contributors to understand.
Let's keep it simple for now, if needs be we can always review and refactor. ;)
/lgtm
k8s-ci-robot
added
release-note
|
/retest |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds event recorder and emits warning events on errors.
Signed-off-by: hasheddan georgedanielmangum@gmail.com
Which issue(s) this PR fixes:
Fixes #41
Special notes for your reviewer:
Does this PR introduce a user-facing change?