Log a better message on trying to deregister a nonexistent service #2492
Conversation
| @@ -1074,6 +1074,10 @@ func TestAgent_serviceTokens(t *testing.T) { | |||
| l := new(localState) | |||
| l.Init(config, nil) | |||
|
|
|||
| l.AddService(&structs.NodeService{ | |||
There was a problem hiding this comment.
Is this related to the log change?
There was a problem hiding this comment.
This is because if we don't add the service initially, it'll panic on trying to log the warning with the nil logger we initialize with above
There was a problem hiding this comment.
Oh makes sense - this test was kind of bogus w/o the registration.
| l.serviceStatus[serviceID] = syncStatus{inSync: false} | ||
| l.changeMade() | ||
| } else { | ||
| l.logger.Printf("[WARN] agent: Tried to deregister nonexistent service '%s'", serviceID) |
There was a problem hiding this comment.
I think you'll want to add an error return to this function can compose the warning one level up. Otherwise you'll get this log message followed by "agent: removed service" which'll be confusing. I think up there we can return nil from RemoveService() and add a comment about how we are trying to keep the current idempotent behavior for now (probably a TODO to fix for 0.8).
| l.serviceStatus[serviceID] = syncStatus{inSync: false} | ||
| l.changeMade() | ||
| } else { | ||
| l.logger.Printf("[WARN] agent: Tried to deregister nonexistent service '%s'", serviceID) |
There was a problem hiding this comment.
You can drop the quote marks and use %q here.
|
LGTM |
Improve the log message when attempting to deregister a service that doesn't exist, as shown here: #1188 (comment)