Added spec support to analyze function in support-bundle
#147
Conversation
cmd/troubleshoot/cli/analyze.go
Outdated
| @@ -59,8 +71,10 @@ func Analyze() *cobra.Command { | |||
| }, | |||
| } | |||
|
|
|||
| cmd.Flags().String("url", "", "URL of the support bundle to analyze") | |||
| cmd.MarkFlagRequired("url") | |||
| cmd.Flags().String("bundle", "", "Filename of the support bundle to analyze") | |||
There was a problem hiding this comment.
maybe "filename or URL of the support bundle to analyze"?
Assuming URLs still work
There was a problem hiding this comment.
Let's make sure to copy the capabilities of preflight and support-bundle here. Take a look at those two and how they set up the CLI.
My instinct is that preflight and support-bundle read their spec from arg[0]. not a flag, and we should copy that logic here for consistency.
There was a problem hiding this comment.
@marccampbell So preflight or support-bundle generation don't require an additional parameter to take a bundle input. So can we nail down what we want here. So it could be either:
kubectl support-bundle analyze <url-or-file-to-analyzer-spec> --bundle=<url-or-file-to-support-bundle>
or
kubectl support-bundle analyze <url-or-file-to-analyzer-spec> <url-or-file-to-support-bundle>
At first glance it seemed confusing to offer the ability to download the support bundle through a URL, as our docs say to use kubectl support-bundle <url-or-file-to-collector-spec> to download a support-bundle to the local file system. So it would seem logical the next step in the workflow is require a support-bundle on the local file system. That being said, it should be one of the required parameters (so not a flag here?). Although I can see people mixing up two positional parameters, which why a flag might be better. This way all of our commands are consistent:
kubectl preflight <url-or-file>
kubectl support-bundle <url-or-file>
kubectl support-bundle analyze <url-or-file> --bundle=<file>
Note: kotsadm currently doesn't expose the URL to download the bundle so currently it seems there's no way for a user to get the URL.
Thoughts?
There was a problem hiding this comment.
kubectl support-bundle analyze <url-or-file-to-analyzer-spec> --bundle=<url-or-file-to-support-bundle> makes sense to me and feels consistent.
There was a problem hiding this comment.
yup, makes sense to me as well. I'll go with that then...
cmd/troubleshoot/cli/analyze.go
Outdated
| filename := v.GetString("spec") | ||
| var analyzersSpec string | ||
| if len(filename) > 0 { | ||
| out, err := ioutil.ReadFile(filename) |
There was a problem hiding this comment.
would it be possible to use the same "read file if local path, get URL if url" logic here as in https://github.com/replicatedhq/troubleshoot/blob/v0.9.25/pkg/analyze/download.go#L73 ?
There was a problem hiding this comment.
The support-bundle URL doesn't make sense (see my comment above about the workflow). But your right, accepting an analyzer URL does make sense here. I'll add it.
cmd/troubleshoot/cli/analyze.go
Outdated
| @@ -59,8 +71,10 @@ func Analyze() *cobra.Command { | |||
| }, | |||
| } | |||
|
|
|||
| cmd.Flags().String("url", "", "URL of the support bundle to analyze") | |||
| cmd.MarkFlagRequired("url") | |||
| cmd.Flags().String("bundle", "", "Filename of the support bundle to analyze") | |||
There was a problem hiding this comment.
Let's make sure to copy the capabilities of preflight and support-bundle here. Take a look at those two and how they set up the CLI.
My instinct is that preflight and support-bundle read their spec from arg[0]. not a flag, and we should copy that logic here for consistency.
This commit includes the following changes: 1. Updated to allow a analyze spec from URL 2. Removed spec flag The analyzer spec is a positional argument consistent with preflight.
cmd/preflight/cli/root.go
Outdated
| @@ -16,7 +16,7 @@ var ( | |||
|
|
|||
| func RootCmd() *cobra.Command { | |||
| cmd := &cobra.Command{ | |||
| Use: "preflight [url]", | |||
| Use: "preflight [url-or-file]", | |||
There was a problem hiding this comment.
https://github.com/replicatedhq/troubleshoot/blob/master/cmd/preflight/cli/root.go#L19
for consistency, let's just specify url in the help text
There was a problem hiding this comment.
Yeah, that's why I changed BOTH preflight and support-bundle in order to be consistent. But I can go change them both back to just url if you want.
cmd/troubleshoot/cli/analyze.go
Outdated
| Short: "analyze a support bundle", | ||
| Long: `...`, | ||
| Long: `Used to analyze an already downloaded support-bundle`, |
There was a problem hiding this comment.
Analyze a support bundle using the Analyzer definitions provided
cmd/troubleshoot/cli/analyze.go
Outdated
| @@ -59,8 +69,8 @@ func Analyze() *cobra.Command { | |||
| }, | |||
| } | |||
|
|
|||
| cmd.Flags().String("url", "", "URL of the support bundle to analyze") | |||
| cmd.MarkFlagRequired("url") | |||
| cmd.Flags().String("bundle", "", "Filename of the support bundle to analyze") | |||
cmd/troubleshoot/cli/root.go
Outdated
| @@ -17,7 +17,7 @@ var ( | |||
|
|
|||
| func RootCmd() *cobra.Command { | |||
| cmd := &cobra.Command{ | |||
| Use: "troubleshoot [url]", | |||
| Use: "troubleshoot [url-or-file]", | |||
|
Marc said the changes were good. I'll merge now. |
I wanted to make this change now to help speed up testing for my new rook analyzer. I assume nobody is currently using this function yet, so I took the risk of changing the name of the required field from 'url' to 'bundle'. So this DOES NOT make it backwards compatible. If anyone doesn't like this I can change the name back to
url(the name just doesn't fit those because its a support-bundle filename).