-
Notifications
You must be signed in to change notification settings - Fork 55
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 "current-since" field to services API and CLI #115
Conversation
Shows as follows in the CLI: $ pebble services Service Startup Current Start Time test2 enabled active today at 17:08 NZST $ pebble services --abs-time Service Startup Current Start Time test2 enabled active 2022-04-28T17:08:45+12:00
Per discussion at London mini-sprint, we want to make the API field
|
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.
Very clean PR, thanks Ben.
Name string `json:"name"` | ||
Startup string `json:"startup"` | ||
Current string `json:"current"` | ||
CurrentSince *time.Time `json:"current-since,omitempty"` // pointer as omitempty doesn't work with time.Time directly |
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.
That's awkward, considering times have IsZero which fits properly with the notion of omitempty.
This is an internal detail, so I'm not blocking on it, but it's curious that the standard json package is misbehaving there.
oldStatus := stateToStatus(s.state) | ||
newStatus := stateToStatus(state) | ||
if oldStatus != newStatus { | ||
s.currentSince = time.Now() |
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.
Nice, very clean logic.
This PR adds a
current-since
field to the API, meaning the time at which the "current" field last changed, for example started (current changed frominactive
toactive
), and so on. It's related to #104 (though in a previous iteration it was "start time", not "curren since").Here's how it looks like the CLI:
This PR also removes the "restarts" field which isn't being used and was internal-only. Per #104, we're going to use changes/tasks or events instead.