-
Notifications
You must be signed in to change notification settings - Fork 53
Added command to check your local installs. #121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| for _, r := range required { | ||
| fmt.Printf("%s", r.name) | ||
| out, err := exec.Command(r.command, r.args...).CombinedOutput() | ||
| if err != nil { |
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.
Can we make a differentiation here between "the command doesn't exist" and "executing this command returned a non-zero exit code"?
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.
Yeah, I'll investigate this some more. I was able to infer based on the output of the command if it's missing or not as the error is consistent. We would need to check cross-platform to make sure it returns a similar error. There may also be another way to run commands that is more verbose.
E.g.
Command Error Info
--------- --------- ---------
kubectl version exit status 1 https://kubernetes.io/docs/tasks/tools/install-kubectl/
jq2 --version exec: "jq2": executable file not found in $PATH https://stedolan.github.io/jq/download/
git Zero requires git version 2.12 or greater. https://git-scm.com/book/en/v2/Getting-Started-Installing-Git% | } | ||
| } | ||
| fmt.Println() | ||
| }, |
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.
Should return zero exit code if the check succeeded, non-zero if it failed.
cmd/check.go
Outdated
|
|
||
| ver = fmt.Sprintf("%d.%d.%d", major, minor, patch) | ||
|
|
||
| if major < 1 || (major == 1 && minor < 16) { |
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.
Can we push these version numbers up to config? Even if it's just hard-coded config for now.
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.
Can we reliably count on all the tools we are checking adhering to some version of semver? If so we could have this as part of the config or even use a semver library to do these calculations.
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.
My initial thought was to simply have each command use a regex and a semver minimum version as part of the config, then push all the logic to a shared function. But I was concerned that perhaps some tools wouldn't fix that. Given all the current tools allow that I might just go back to it.
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. If we introduce a dependency that doesn't use semver we can reassess then.
cmd/check.go
Outdated
| name: "AWS CLI\t\t", | ||
| command: "aws", | ||
| args: []string{"--version"}, | ||
| docsURL: "", |
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.
Should add the AWS CLI install URL here also.
cmd/check.go
Outdated
| name string | ||
| command string | ||
| args []string | ||
| docsURL string |
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.
At some point we could have our own documentation string here that could have an OS-specific description of how to install this tool. (hopefully just a single command)
No description provided.