-
Notifications
You must be signed in to change notification settings - Fork 225
Logging and events #231
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
Logging and events #231
Conversation
cmd/machine-api-operator/start.go
Outdated
@@ -67,7 +67,7 @@ func runStartCmd(cmd *cobra.Command, args []string) { | |||
Callbacks: leaderelection.LeaderCallbacks{ | |||
OnStartedLeading: run, | |||
OnStoppedLeading: func() { | |||
glog.Fatalf("leaderelection lost") | |||
glog.Fatalf("leader election lost") |
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.
uppercase Leader
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.
Awkward one, but I tend to go with: https://github.com/golang/go/wiki/CodeReviewComments#error-strings
dd7ee06
to
143dbc5
Compare
I think the "rules" would be simpler if it was lower always. With that simple rule there's no ambiguity. Code reviews go faster because there's only 1 rule. Error message and Warning messages and Info messages are consistently formatted. It is also idiomatic Go [1]. [1] - https://github.com/golang/go/wiki/CodeReviewComments#error-strings |
/test integration |
/test integration |
/test integration |
fixes integration #233 |
143dbc5
to
10d6daf
Compare
/test integration |
var message string | ||
if !reflect.DeepEqual(desiredVersions, currentVersions) { | ||
glog.V(2).Info("Syncing status: progressing") | ||
message = fmt.Sprintf("Progressing towards %s", optr.printOperandVersions()) | ||
optr.eventRecorder.Eventf(co, v1.EventTypeNormal, "Status upgrade", message) |
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.
Status upgraded
or Status upgraging
maybe?
Does it make sense to report |
10d6daf
to
66e2c50
Compare
cmd/nodelink-controller/main.go
Outdated
modNode.Labels[k] = v | ||
} | ||
|
||
addTaintsToNode(modNode, matchingMachine) | ||
|
||
if !reflect.DeepEqual(node, modNode) { | ||
glog.V(3).Infof("node has changed, updating") | ||
glog.V(3).Infof("Node has changed, updating") |
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.
If we're going to log we might as well indicate which node.
66e2c50
to
a5b6fb5
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund 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 |
Improve logging and add events for the operator cluster status
Conventions: