-
Notifications
You must be signed in to change notification settings - Fork 10
Add filter positional argument to clusters list #52
Add filter positional argument to clusters list #52
Conversation
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.
Looks good. Just a very minor suggestion about comments.
@ALimaRedHat please take a look.
Short: "List clusters", | ||
Long: "List clusters by ID and Name", | ||
Run: run, | ||
Args: cobra.RangeArgs(0, 1), | ||
RunE: run, |
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.
This change to RunE
is nice. Is there any chance that you can do the same in all the other commands?
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.
Absolutely. Would that be better done in a follow up PR though (as to not balloon the size 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.
Yes, sure. A separate PR is better.
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.
Looks good, thanks @cblecker. I will wait a bit for @ALimaRedHat to review.
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.
Looks good to me too
If there is a positional argument, use it as a name or ID filter. Compatible with the
--managed
flag too.