cert-manager-ctl CLI with version #2725
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoshVanL The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cmd/ctl/main.go
Outdated
| func SetupSignalHandler() (stopCh <-chan struct{}) { | ||
| stop := make(chan struct{}) | ||
| c := make(chan os.Signal, 2) | ||
| signal.Notify(c, []os.Signal{os.Interrupt, syscall.SIGTERM}...) |
There was a problem hiding this comment.
Would signal.Notify(c, os.Interrupt, syscall.SIGTERM) not be simpler here?
cmd/ctl/pkg/version/version.go
Outdated
|
|
||
| // Run executes version command | ||
| func (o *Options) Run() error { | ||
| var err error |
There was a problem hiding this comment.
We never seem to use this error
| = vendor/github.com/fatih/color/LICENSE.md 316e6d590bdcde7993fb175662c0dd5a | ||
| ================================================================================ | ||
|
|
||
| = vendor/github.com/docker/docker licensed under: = |
There was a problem hiding this comment.
🙄 what caused us to start depending on docker/docker?! 😬
cmd/ctl/cmd.go
Outdated
| @@ -0,0 +1,45 @@ | |||
| /* | |||
| Copyright 2019 The Jetstack cert-manager contributors. | |||
cmd/ctl/main.go
Outdated
| @@ -0,0 +1,53 @@ | |||
| /* | |||
| Copyright 2019 The Jetstack cert-manager contributors. | |||
cmd/ctl/main.go
Outdated
| // SetupSignalHandler registered for SIGTERM and SIGINT. A stop channel is returned | ||
| // which is closed on one of these signals. If a second signal is caught, the program | ||
| // is terminated with exit code 1. | ||
| func SetupSignalHandler() (stopCh <-chan struct{}) { |
There was a problem hiding this comment.
nit: doesn't this exist somewhere already? If not, can we un-export this? It's not importable anyway as this is a main package so 🤷♂
cmd/ctl/pkg/version/version.go
Outdated
| @@ -0,0 +1,105 @@ | |||
| /* | |||
| Copyright 2019 The Jetstack cert-manager contributors. | |||
cmd/ctl/pkg/version/version.go
Outdated
| // Options is a struct to support version command | ||
| type Options struct { | ||
| Output string | ||
| Short bool |
There was a problem hiding this comment.
nit: comments here would be great - could just be copied from the flag description
cmd/ctl/pkg/version/version.go
Outdated
| }, | ||
| } | ||
|
|
||
| cmd.Flags().StringVarP(&o.Output, "output", "o", o.Output, "One of 'yaml' or 'json'.") |
There was a problem hiding this comment.
You're setting the default value here to o.Output - shouldn't this be yaml or json? It'll be `` (empty) otherwise
There was a problem hiding this comment.
Ahh, I see, 'empty' means 'human readable'. Maybe just change this to "" then instead of o.Short? :)
cmd/ctl/pkg/version/version.go
Outdated
| } | ||
|
|
||
| cmd.Flags().StringVarP(&o.Output, "output", "o", o.Output, "One of 'yaml' or 'json'.") | ||
| cmd.Flags().BoolVar(&o.Short, "short", o.Short, "If true, print just the version number.") |
There was a problem hiding this comment.
Again, not sure why defaulting to o.Short - can you change this to be false?
There was a problem hiding this comment.
Taken from kubectl code. I agree, hate that
| Platform: fmt.Sprintf("%s/%s", runtime.GOOS, runtime.GOARCH), | ||
| } | ||
| } | ||
|
|
||
| func version() string { |
There was a problem hiding this comment.
May be one for a follow up, but can we remove this function now? canary should never be set as the version tag anymore since the build infra changes. May be easiest to follow up though 😄
|
Also, this pulls in |
|
Hm, it also appears that the dependency has changed to use |
cmd/ctl/main.go
Outdated
| ) | ||
|
|
||
| func main() { | ||
| stopCh := genericapiserver.SetupSignalHandler() |
There was a problem hiding this comment.
nit (one to follow up on as there's a few places that call this) - given the generic nature of the pkg/server package, I'm concerned that this probably adds some extra bloat to all our binaries when it is pulled in. This will be especially relevant with a CLI tool as users will be downloading this to their own machines. Perhaps we should have our own version of this in the repo 😄
cmd/ctl/cmd.go
Outdated
| Use: "cert-manager-ctl", | ||
| Short: "cert-manager CLI tool to manage and configure cert-manager resources", | ||
| Long: ` | ||
| cert-manager-ctl is a CLI tool managage and confiure cert-manager resources for Kubernetes`, |
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
82187b9 to
2ed9ec4
Compare
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
|
/lgtm |
This PR adds the cert-manager-ctl CLI which has a
versionsub command.Currently also gets built to a binary in the host arch.
TODO: fix version injection
/assign @munnerz