add an ark bug command #774

bookshelfdave · 2018-08-17T18:36:46Z

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.
- I experimented with using a standalone template file, but ran into a backtick escaping nightmare and ETOOCOMPLEX.
The hack/update-generated-issue-template.sh script renders the template (without populating any template vars) to .github/ISSUE_TEMPLATE/bug_report.md, and the hack/verify-generated-issue-template.sh will ensure that the bug_report.md file wasn't updated by hand. This will happen automatically during testing by way of ./hack/verify-all.sh.
- This means that changes to the issue template must be made in 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

nrb · 2018-08-17T20:20:42Z

Thanks @metadave ! I'll take another look at this, but it may be next week.

carlisia · 2018-08-17T22:18:15Z

pkg/cmd/cli/bug/bug.go

+	default:
+		err = fmt.Errorf("Ark can't open a browser window on platform %s", os)
+	}
+	return err


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.

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.

carlisia

Hey @metadave, I added all requests for change I had. Thank you.

carlisia · 2018-08-17T22:56:22Z

pkg/cmd/cli/bug/bug.go

+
+const (
+	// how long we wait for `kubectl version` before killing the process
+	kubectlTimeout = 5 * time.Second


Even though this is not exported, please begin this comment with kubectlTimeout. Also: yeay comment!

carlisia · 2018-08-17T22:57:52Z

pkg/cmd/cli/bug/bug.go

+// 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) {


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!

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)

carlisia · 2018-08-19T16:28:18Z

pkg/cmd/cli/bug/bug.go

+		Use:   "bug",
+		Short: "Report an Ark bug",
+		Long:  "Open a browser window to report an Ark bug",
+		Run: func(c *cobra.Command, args []string) {


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.

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.

Ah. I like this approach. We are not halting the program at all. Good alternative in my view.

carlisia · 2018-08-19T16:30:21Z

pkg/cmd/cli/bug/bug.go

+	var outbuf bytes.Buffer
+	kubectlCmd.Stdout = &outbuf
+	if err := kubectlCmd.Start(); err != nil {
+		log.Fatal(err)


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.

carlisia · 2018-08-19T17:27:18Z

docs/issue-template-gen/main.go

+limitations under the License.
+*/
+
+/*


Please use the commonly used format of

// //

for comments that are not copyright comments.

carlisia · 2018-08-19T17:52:26Z

pkg/cmd/cli/bug/bug.go

+	return buf.String(), nil
+}
+
+// open a browser window to submit a Github issue using a platform specific binary.


Please start comments with the name of the function; in this case: showIssueInBrowser.

carlisia · 2018-08-19T17:56:52Z

pkg/cmd/cli/bug/bug.go

+		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")


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.

carlisia · 2018-08-19T18:07:10Z

pkg/cmd/cli/bug/bug.go

+	select {
+	case <-time.After(kubectlTimeout):
+		// we don't care about the possible error returned from Kill() here,
+		// just return an empty string


Thanks for adding a clarifying comment.

bookshelfdave · 2018-08-20T00:32:35Z

@carlisia thank you for the insightful review (and on a weekend no less!). Changes pushed.

carlisia · 2018-08-20T00:36:48Z

pkg/cmd/cli/bug/bug.go

+}
+
+func newBugInfo(kubectlVersion string) *ArkBugInfo {
+	bugInfo := ArkBugInfo{


Shoot, I forgot to add this comment: I'm pretty sure this variable here is redundant, you may s/bugInfo :=/return.

good catch, fixed!

carlisia

Thanks for promptly addressing the reviews, @metadave , on a weekend no less ;)

Pending the comment for newBugInfo, this lgtm.

carlisia · 2018-08-20T00:46:34Z

Awesome, thank you 🎉. Let's get one more 👍 from the team.

nrb · 2018-08-20T15:40:31Z

pkg/cmd/cli/bug/bug.go

+	// 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 = `---


@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?

Works for us @nrb

Awesome, thanks!

nrb · 2018-08-24T16:22:29Z

Did one more pass on this, and verified that it will be included in make update via hack/update-all.sh.

@metadave One final request - could you add mention of the ark bug command to troubleshooting.md?

bookshelfdave · 2018-08-24T17:06:03Z

troubleshooting.md updated. Thanks for the review @nrb!

EDIT: squashed

nrb · 2018-08-24T17:08:00Z

docs/troubleshooting.md

@@ -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.


Should probably state that it's the ark client version, at least for now.

Signed-off-by: Dave Parfitt <diparfitt@gmail.com>

rosskukulinski added this to the v0.10.0 milestone Aug 17, 2018

rosskukulinski requested review from nrb and skriss August 17, 2018 18:42

carlisia reviewed Aug 17, 2018

View reviewed changes

carlisia suggested changes Aug 19, 2018

View reviewed changes

carlisia added the Enhancement/User End-User Enhancement to Velero label Aug 19, 2018

carlisia reviewed Aug 20, 2018

View reviewed changes

carlisia approved these changes Aug 20, 2018

View reviewed changes

nrb reviewed Aug 20, 2018

View reviewed changes

bookshelfdave force-pushed the dp_report_bug branch from ceacc99 to f10e4d4 Compare August 24, 2018 17:06

nrb reviewed Aug 24, 2018

View reviewed changes

bookshelfdave force-pushed the dp_report_bug branch from f10e4d4 to c67dce1 Compare August 24, 2018 17:10

add an ark bug command

342a1c6

Signed-off-by: Dave Parfitt <diparfitt@gmail.com>

bookshelfdave force-pushed the dp_report_bug branch from c67dce1 to 342a1c6 Compare August 24, 2018 17:11

nrb merged commit adc29a2 into vmware-tanzu:master Aug 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add an ark bug command #774

add an ark bug command #774

bookshelfdave commented Aug 17, 2018

nrb commented Aug 17, 2018

carlisia Aug 17, 2018

bookshelfdave Aug 17, 2018

carlisia left a comment

carlisia Aug 17, 2018

carlisia Aug 17, 2018

carlisia Aug 19, 2018

carlisia Aug 19, 2018 •

edited

Loading

bookshelfdave Aug 20, 2018 •

edited

Loading

carlisia Aug 20, 2018

carlisia Aug 19, 2018 •

edited

Loading

carlisia Aug 19, 2018

carlisia Aug 19, 2018

carlisia Aug 19, 2018

carlisia Aug 19, 2018

bookshelfdave commented Aug 20, 2018

carlisia Aug 20, 2018

bookshelfdave Aug 20, 2018

carlisia left a comment

carlisia commented Aug 20, 2018

nrb Aug 20, 2018

jhamilton1 Aug 24, 2018

nrb Aug 24, 2018

nrb commented Aug 24, 2018 •

edited

Loading

bookshelfdave commented Aug 24, 2018 •

edited

Loading

nrb Aug 24, 2018

bookshelfdave Aug 24, 2018

		@@ -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.

add an ark bug command #774

add an ark bug command #774

Conversation

bookshelfdave commented Aug 17, 2018

nrb commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlisia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlisia Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

bookshelfdave Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlisia Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bookshelfdave commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlisia left a comment

Choose a reason for hiding this comment

carlisia commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrb commented Aug 24, 2018 • edited Loading

bookshelfdave commented Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlisia Aug 19, 2018 •

edited

Loading

bookshelfdave Aug 20, 2018 •

edited

Loading

carlisia Aug 19, 2018 •

edited

Loading

nrb commented Aug 24, 2018 •

edited

Loading

bookshelfdave commented Aug 24, 2018 •

edited

Loading