Rename list instances to get instances #258
Conversation
cmd/kudoctl/cmd/get/instances.go
Outdated
| @@ -28,14 +28,13 @@ const ( | |||
| defaultConfigPath = ".kube/config" | |||
| ) | |||
|
|
|||
| // NewListInstancesCmd creates a command that lists the instances in the cluster | |||
| func NewListInstancesCmd() *cobra.Command { | |||
There was a problem hiding this comment.
Change the Command object's name as well
| ) | ||
|
|
||
| // NewGetInstancesCmd creates a command that lists the instances in the cluster | ||
| func NewGetInstancesCmd() *cobra.Command { |
There was a problem hiding this comment.
I don't quite understand why there are two commands, one in pkg and one in cmd. Which one is actually run? What's the purpose of the other one. This all isn't necessary for this PR though
There was a problem hiding this comment.
I think Intellij must have done this somehow during refactoring. I definitely did not move any code around by myself... let me retry once more :)
There was a problem hiding this comment.
Think you're ok, the one under pkg/kudoctl/cmd has been there a while. We may want to move the functionality of listing instances out to it's own package and not have cobra details inside of pkg/kudoctl/cmd - but like @runyontr said, that's another PR.
|
@runyontr I think now it should be fine :) |
| ) | ||
|
|
||
| // NewGetInstancesCmd creates a command that lists the instances in the cluster | ||
| func NewGetInstancesCmd() *cobra.Command { |
There was a problem hiding this comment.
Think you're ok, the one under pkg/kudoctl/cmd has been there a while. We may want to move the functionality of listing instances out to it's own package and not have cobra details inside of pkg/kudoctl/cmd - but like @runyontr said, that's another PR.
There was a problem hiding this comment.
Just curious are there any other documentation/demos referencing the old command?
Would it make sense to have an alias with the old command?
Also, this should be mentioned in release notes being a breaking change, or?
Having that said, thanks for this PR :-)!
|
@joerg84 I went over this whole repo, hopefully I fixed all the usages. I am not aware of any other. Also I've filled the release note part of the PR, I hope that is enough to get it propagated |
|
@alenkacz squash+merge when ready. 👍 |
What type of PR is this?
/component kudoctl
What this PR does / why we need it:
Moving away from
kudo list instancesto more kubectl-like feel withkudo get instances. It's just a proposal - we can discuss if everyone feels good about this move. I just talked about it with @fabianbaier and he was not opposed.Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?: