-
Notifications
You must be signed in to change notification settings - Fork 152
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
Add actionset and phase name to controller logs #287
Conversation
pkg/controller/controller.go
Outdated
@@ -268,23 +269,25 @@ func (c *Controller) onDeleteBlueprint(bp *crv1alpha1.Blueprint) error { | |||
|
|||
func (c *Controller) initActionSetStatus(as *crv1alpha1.ActionSet) { | |||
if as.Spec == nil { | |||
log.Error("Cannot initialize an ActionSet without a spec.") | |||
log.Error("Cannot initialize an ActionSet %s without a spec.", as.GetName()) |
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.
- Let's not use formatting - we could create the context earlier in the function and add the name there.
- If you had wanted to use formatting, you would need to use
Errof
func (c *Controller) logAndSuccessEvent(msg, reason string, objects ...runtime.Object) { | ||
log.Info(msg) | ||
func (c *Controller) logAndSuccessEvent(ctx context.Context, msg, reason string, objects ...runtime.Object) { | ||
log.WithContext(ctx).Info(msg) |
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.
what happens when context is nil?
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.
func (entry *Entry) WithContext(ctx context.Context) *Entry {
return &Entry{Logger: entry.Logger, Data: entry.Data, Time: entry.Time, err: entry.err, Context: ctx}
}
The Entry.Context will be assigned nil
@@ -4,6 +4,13 @@ import "context" | |||
|
|||
// context.Context support for fields | |||
|
|||
const ( | |||
ActionsetNameKey = "ActionSet" |
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.
Not sure we discussed this in slack. These constants should be defined outside the field
package. Both the interface and the implementation for field
and the replacement log
and errors
packages should not contain kanister specific references or dependencies.
Change Overview
To make the controller logs more descriptive, adding actionset name and phase name to the log context.
Pull request type
Please check the type of change your PR introduces:
Will be testing logs with a small timelog app and will update this section soon.