-
Notifications
You must be signed in to change notification settings - Fork 302
feat(fleetctl): list machines in consistent/sorted order #473
Conversation
sure |
@@ -52,8 +52,6 @@ func runListUnits(args []string) (exit int) { | |||
|
|||
func findAllUnits() (jobs map[string]job.Job, sortable sort.StringSlice, err error) { | |||
jobs = make(map[string]job.Job, 0) | |||
sortable = make(sort.StringSlice, 0) |
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.
Can we leave this change out of this PR?
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.
Do you really feel strongly about this? It makes it consistent with findAllMachines
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.
(by which I mean I feel it is relevant enough to be part of this change)
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.
I am always a proponent of making a commit change one thing, and that makes it easier to follow the history. The consistencty that we get between a new method and this existing method isn't a great argument to me, as you could make findAllMachines
match what already exists, then change both at once in a secondary commit. I won't block this PR on something this small, but I want you to understand where I'm coming from.
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.
Noted.
Added docstrings. |
lgtm |
feat(fleetctl): list machines in consistent/sorted order
I propose sorting by machineid.