-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
add an ark bug command #774
Conversation
Thanks @metadave ! I'll take another look at this, but it may be next week. |
pkg/cmd/cli/bug/bug.go
Outdated
default: | ||
err = fmt.Errorf("Ark can't open a browser window on platform %s", os) | ||
} | ||
return err |
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.
Hey @metadave this is a huge contribution!
I wanted to propose an alternative way of doing this switch only for the benefit of readability:
switch runtime.GOOS {
case "darwin":
return exec.Command("open", url).Start()
case "linux":
if cmdExistsOnPath("xdg-open") {
return exec.Command("xdg-open", url).Start()
}
return fmt.Errorf("Ark can't open a browser window using the command '%s'", "xdg-open")
case "windows":
return exec.Command("rundll32", "url.dll,FileProtocolHandler", url).Start()
default:
return fmt.Errorf("Ark can't open a browser window on platform %s", runtime.GOOS)
}
. if we use return
everywhere right where it happens, it saves the reader from continue to read the code to find where it returns. In this case, it would return in each of those cases if there is an error and this alternative makes it very obvious.
. in Go, every time we add an else
we may profitably question if we can remove it. The majority of the time it will be redundant. It is in this case so I removed it. I submit that it also reads better this way.
. it's not necessary to return the err
variable in the end, because there's a default.
. If runtime.GOOS
is used as is, there's more code and logic that can be removed. Not much is gained here by keeping that, imo.
. the reason why I would do away with the constant for the value "xdg-open"
is that it is not abstracting a string that is likely to change, which one could argue is most of the purpose of a constant. I personally also would rather see the value spelled out where it is being used.
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.
Your proposed changes are indeed easier to read :-) Thank you for the feedback!
I generally introduce a constant if I have to type a string value more than once or twice, as I've been burned by typos in the past. However, I agree with your comment above and will apply that change.
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.
Hey @metadave, I added all requests for change I had. Thank you.
pkg/cmd/cli/bug/bug.go
Outdated
|
||
const ( | ||
// how long we wait for `kubectl version` before killing the process | ||
kubectlTimeout = 5 * time.Second |
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.
Even though this is not exported, please begin this comment with kubectlTimeout
. Also: yeay comment!
pkg/cmd/cli/bug/bug.go
Outdated
// Make a best-effort to run `kubectl version` and return it's output. | ||
// This func will Timeout and return an empty string after | ||
// kubectlTimeout if we're not connected to a cluster. | ||
func getKubectlVersion() (kubectlVersion 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.
Again, thank you for for adding comments. Please change this one to begin with the function name (getKubectlVersion
). Same with comments for the other methods. Thank you!
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.
To be consistent with the rest of the code base, please remove the naked return from this method's signature. See the other comment elsewhere, so this should end up being: getKubectlVersion() (string, error)
Use: "bug", | ||
Short: "Report an Ark bug", | ||
Long: "Open a browser window to report an Ark bug", | ||
Run: func(c *cobra.Command, args []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.
This is related to a comment inside getKubectlVersion
. In the function getKubectlVersion
don't log.Fatal
. Instead, change its signature to return an error, and return an error instead of doing a log.Fatal
. Here, call that function and do a cmd.CheckError(err)
. Then fetch the string from it and pass it into newBugInfo
as an argument.
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.
I cleaned up the way getKubectlVersion
returns errors, but I'd prefer not to use cmd.CheckError(err)
on the result, as not being connected to a cluster would prevent a user from opening a new bug.
For example, I have access to several clusters, but most of the time I can't access those via kubectl
unless I unencrypt the kubeconfig and/or open the firewall. But I may be experimenting with the ark cli, and might want to report an issue.
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.
Ah. I like this approach. We are not halting the program at all. Good alternative in my view.
pkg/cmd/cli/bug/bug.go
Outdated
var outbuf bytes.Buffer | ||
kubectlCmd.Stdout = &outbuf | ||
if err := kubectlCmd.Start(); err != nil { | ||
log.Fatal(err) |
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.
There is a place where we check for errors explicitly, and I would rather this code change a bit to continue with that consistency. It'd also help abide by the principle of least surprise. Please check the comment around LN 91 that has to do with making this return an error instead of log.Fatal
.
docs/issue-template-gen/main.go
Outdated
limitations under the License. | ||
*/ | ||
|
||
/* |
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.
Please use the commonly used format of
//
//
for comments that are not copyright comments.
pkg/cmd/cli/bug/bug.go
Outdated
return buf.String(), nil | ||
} | ||
|
||
// open a browser window to submit a Github issue using a platform specific binary. |
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.
Please start comments with the name of the function; in this case: showIssueInBrowser
.
pkg/cmd/cli/bug/bug.go
Outdated
if cmdExistsOnPath("xdg-open") { | ||
return exec.Command("xdg-open", url).Start() | ||
} | ||
return fmt.Errorf("Ark can't open a browser window using the command '%s'", "xdg-open") |
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.
Please change this and other error messages to start with a lower case letter. The error messages get mixed into larger ones, and it'd be odd to have an upper case letter in the middle of a sentence.
select { | ||
case <-time.After(kubectlTimeout): | ||
// we don't care about the possible error returned from Kill() here, | ||
// just return an empty 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.
Thanks for adding a clarifying comment.
@carlisia thank you for the insightful review (and on a weekend no less!). Changes pushed. |
pkg/cmd/cli/bug/bug.go
Outdated
} | ||
|
||
func newBugInfo(kubectlVersion string) *ArkBugInfo { | ||
bugInfo := ArkBugInfo{ |
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.
Shoot, I forgot to add this comment: I'm pretty sure this variable here is redundant, you may s/bugInfo :=/return
.
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.
good catch, fixed!
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 promptly addressing the reviews, @metadave , on a weekend no less ;)
Pending the comment for newBugInfo
, this lgtm.
Awesome, thank you 🎉. Let's get one more 👍 from the team. |
// IssueTemplate is used to generate .github/ISSUE_TEMPLATE/bug_report.md | ||
// as well as the initial text that's place in a new Github issue as | ||
// the result of running `ark bug`. | ||
IssueTemplate = `--- |
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.
@jhamilton1 This will be the location for the bug template, if we merge this. What it means is that the Go file will have to be updated, then we run the update-generated-issue-template.sh
script in this repo to get the file that GitHub uses. That enables us to not repeat the template in 2 files.
Is this approach ok with you?
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.
Works for us @nrb
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.
Awesome, thanks!
Did one more pass on this, and verified that it will be included in @metadave One final request - could you add mention of the |
EDIT: squashed |
ceacc99
to
f10e4d4
Compare
docs/troubleshooting.md
Outdated
@@ -2,6 +2,8 @@ | |||
|
|||
These tips can help you troubleshoot known issues. If they don't help, you can [file an issue][4], or talk to us on the [#ark-dr channel][25] on the Kubernetes Slack server. | |||
|
|||
In `ark` version >= `0.1.0`, you can use the `ark bug` command to open a [Github issue][4] by launching a browser window with some prepopulated values. Values included are OS, CPU architecture, `kubectl` client and server versions (if available) and the `ark` version. This information isn't submitted to Github until you click the `Submit new issue` button in the Github UI, so feel free to add, remove or update whatever information you like. |
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 probably state that it's the ark client version, at least 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.
fixed
f10e4d4
to
c67dce1
Compare
Signed-off-by: Dave Parfitt <diparfitt@gmail.com>
c67dce1
to
342a1c6
Compare
This PR adds the
ark bug
subcommand, which gathers a few bits of information and opens a new browser window with the Github issue template partially populated.some of this is based on the existing
./hack/update-*
and./hack/verify-*
scripts, so hopefully it's not too off base.The Github issue golang template is stored as a constant in
pkg/cmd/cli/bug/bug.go
.The
hack/update-generated-issue-template.sh
script renders the template (without populating any template vars) to.github/ISSUE_TEMPLATE/bug_report.md
, and thehack/verify-generated-issue-template.sh
will ensure that thebug_report.md
file wasn't updated by hand. This will happen automatically during testing by way of./hack/verify-all.sh
.pkg/cmd/cli/bug/bug.go
.I realize that outside of the
./hack/verify-generated-issue-template.sh
script, there isn't any testing. I'm open to suggestions on ways to test this without jumping through too many hoops :-)I experimented with downloading the template, however I feel as though it's better to embed directly into the binary as the struct or the template itself could change and cause a mismatch between
ark
client versions. One way around this is to link a particular hosted version, but it seems like overkill. Also, it's hard to submit a PR that depends on a hosted file that's in the same PR :-)Eventually, this could include an ark server version via Ability to retrieve Ark server version #770
see also #713
Closes #578