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.
Thanks @cblecker, very nice addition. Just a minor comment about moving the new command to its own package. Once you do that I will merge and make a new release.
cmd/uhc/main.go
Outdated
@@ -63,6 +63,7 @@ func init() { | |||
root.AddCommand(token.Cmd) | |||
root.AddCommand(version.Cmd) | |||
root.AddCommand(cluster.Cmd) | |||
root.AddCommand(completionCmd) |
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.
All the other commands in the CLI live in their own package. Can you do the same for this one? Create a new cmd/uhc/completion
directory and put the code there. Then this will be root.AddCommand(completion.Cmd)
.
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 isn't easily done, as I need to refer back to the top-level command (root
): https://github.com/openshift-online/uhc-cli/pull/9/files#diff-7e01ad4f5f72c98103727d222dba6a64R42
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.
You can do cmd.Root()...
.
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.
Thanks for the tip! Fixed.
cmd/uhc/completion.go
Outdated
@@ -0,0 +1,47 @@ | |||
/* | |||
Copyright (c) 2018 Red Hat, Inc. |
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.
2019
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.
Fixed!
Thanks @cblecker, looks good, I am merging. |
@cblecker your change is included in release 0.1.11. |
Thanks @jhernand! |
No description provided.