Extend version command to help troubleshoot porter #642

phillipahereza · 2019-09-24T15:31:44Z

What does this change

Adds --system flag to the version command to print additional information about what the system on which porter is being run and any installed mixins

$ porter version --system
porter v0.14.1-beta.2 (72dd5df)

System
--------
os: darwin
arch: amd64

Mixins
------
Name         Version          Author
aws          v0.1.2-beta.1    DeisLabs
az           v0.2.1-beta.1    DeisLabs

What issue does it fix

Closes #614

Notes for the reviewer

This is a draft PR to get help

Checklist

Unit Tests
Documentation
- Documentation Not Impacted

msftclas · 2019-09-24T15:32:00Z

All CLA requirements met.

phillipahereza · 2019-09-24T15:32:09Z

@carolynvs am I on the right path

carolynvs · 2019-09-25T15:07:02Z

@phillipahereza Yes, this is what I was looking for. 👍 Once you are ready for a review, just take the PR out of draft, by clicking the button "Ready for Review" or maybe it's called "Request Reviews".

Then I'll go over it and provide detailed feedback. I just redid our CI pipeline, so I'll kick off a new test run for you right now.

carolynvs · 2019-09-25T15:07:11Z

/azp run

azure-pipelines · 2019-09-25T15:07:27Z

Azure Pipelines successfully started running 2 pipeline(s).

dev-drprasad · 2019-09-25T16:50:37Z

pkg/porter/version.go

+		return errors.Wrap(err, "Failed to print version")
+	}
+
+	printSectionHeader(p.Context.Out, "System")


You can achieve this functionality using go templates. I think it will more readable. Just a opinion

example:

const ts = ` version: {{.Version}} {{if .Sys -}} System -------- os: {{.Sys.OS}} arch: {{.Sys.Arch}} {{- end}} ` v := Version{ Version: "1.0.0", Sys: nil, } tmpl, _ := template.New("system").Parse(ts) tmpl.Execute(p.Context.Out, v)

carolynvs

Please ignore the failed CI test, I am looking into that.

I have a few requested changes but this is looking very good!

carolynvs · 2019-09-26T20:14:52Z

cmd/porter/version.go

@@ -7,15 +7,18 @@ import (
 )

 func buildVersionCommand(p *porter.Porter) *cobra.Command {
-	opts := version.Options{}
+	opts := version.VersionOpts{}


Would you please move VersionOpts into pkg/porter/version.go. The version package is intended for other mixins to reuse when they implement the version command.

That will change this line to look like to opts := porter.VersionOpts{} but won't otherwise change much. It's just to help with the organization of the code.

carolynvs · 2019-09-26T20:18:44Z

cmd/porter/version.go

 	cmd := &cobra.Command{
 		Use:   "version",
 		Short: "Print the application version",
 		PreRunE: func(cmd *cobra.Command, args []string) error {
-			return opts.Validate()
+			return opts.Options.Validate()


Please simplify the here back to:

Suggested change

return opts.Options.Validate()

return opts.Validate()

We can do that because Options is embedded in VersionOpts (it isn't a named field) so functions available on that type can be called without having to specify that it's coming from Options. Apologies if you knew about that, I'm just sharing Go tips in case they are helpful! 😀

Thanks, this is all very helpful 🙏🏾

carolynvs · 2019-09-26T20:24:59Z

cmd/porter/version.go

 		},
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return p.PrintVersion(opts)
+			if opts.System {


The convention in Porter is that all logic should be in pkg/porter, and cmd/porter should just wire up cobra to functions defined in that package.

Please move the check against opts.System into PrintVersion, so that the code in this file goes back to looking like:

RunE: func(cmd *cobra.Command, args []string) error { return p.PrintVersion(opts) }

If you are curious about why, I did a talk about how Porter is designed at GopherCon. The short answer is that it helps us test, and allows for people to build on top of porter's libraries.

carolynvs · 2019-09-26T20:29:02Z

pkg/porter/version.go

+}
+
+func (p *Porter) PrintDebugInfo(opts version.Options) error {
+	// force opts to print version as plaintext


Apologies for not catching this when I first checked the PR, but this new --system flag needs to work in conjunction with the --output plaintext|json flag. So PrintDebugInfo needs to support printing to json as well.

carolynvs · 2019-09-26T20:31:56Z

pkg/porter/version_test.go

+	p.TestConfig.SetupPorterHome()
+	err := opts.Validate()
+	require.Nil(t, err)
+	err = p.PrintDebugInfo(opts)


After you move this method and the check for opts.System into p.PrintVersion, I recommend updating this unit test, to exercise your if statement as well. So it should create VersionOptions {System: true} and pass that to p.PrintVersion(opts).

carolynvs

Just some tweaks to the error handling and then this is ready to merge! 💪

carolynvs · 2019-09-27T20:25:27Z

pkg/porter/version.go

 	if err != nil {
-		return errors.Wrap(err, "Failed to print version")
+		_ = errors.Wrap(err, "Failed to get list of mixins")


Instead of ignoring the error (like it's doing now), or returning it (which would make it very hard to use this command for troubleshooting problems 😅 ), we can have it print a debug message and then keep going:

The contributing guide has a section on how to handle debug logging:

https://github.com/deislabs/porter/blob/master/CONTRIBUTING.md#logging

You can check p.Debug and then print to p.Err following the example in the guide, then have the rest of the command continue. That way even if we can't print any mixin data, at least we are able to print the data we do have.

carolynvs · 2019-09-27T20:26:29Z

pkg/porter/version.go

+`
+		tmpl, err := template.New("systemDebugInfo").Parse(plaintextTmpl)
+		if err != nil {
+			_ = errors.Wrap(err, "Failed to print system debug information")


At this point, if we can't parse the template, then we should return the error, and not ignore it with _ because it's an unrecoverable error.

If the template isn't parsed, then continuing on to call Execute on the template (which is nil when err is populated) will cause a nil reference exception.

carolynvs

Thanks for adding this into Porter! It's going to really help out when people submit bug reports. 💯

phillipahereza · 2019-09-30T14:28:39Z

Thanks for adding this into Porter! It's going to really help out when people submit bug reports. 💯

My pleasure, thanks for guiding me through the whole process

print system info

ecbdbcd

phillipahereza added 3 commits September 24, 2019 19:50

added section header underline

bf5f635

added test for printing debug information

822be22

undo unnecessary changes

829863c

phillipahereza marked this pull request as ready for review September 25, 2019 15:09

phillipahereza requested review from carolynvs, jeremyrickard and vdice as code owners September 25, 2019 15:09

carolynvs self-assigned this Sep 25, 2019

dev-drprasad reviewed Sep 25, 2019

View reviewed changes

carolynvs requested changes Sep 26, 2019

View reviewed changes

allow system flag to use json/plaintext output formats

51f0069

phillipahereza requested a review from carolynvs September 27, 2019 08:15

phillipahereza added 2 commits September 27, 2019 11:16

remove unnecessary commented out test function

665e909

fix tests

6390e72

carolynvs requested changes Sep 27, 2019

View reviewed changes

phillipahereza added 3 commits September 29, 2019 10:56

fix error handling

591631c

fix error message

266a307

fix failing test

b5f7fcf

carolynvs approved these changes Sep 30, 2019

View reviewed changes

carolynvs merged commit f0a7949 into getporter:master Sep 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend version command to help troubleshoot porter #642

Extend version command to help troubleshoot porter #642

phillipahereza commented Sep 24, 2019 •

edited

Loading

msftclas commented Sep 24, 2019 •

edited

Loading

phillipahereza commented Sep 24, 2019

carolynvs commented Sep 25, 2019

carolynvs commented Sep 25, 2019

azure-pipelines bot commented Sep 25, 2019

dev-drprasad Sep 25, 2019

carolynvs left a comment

carolynvs Sep 26, 2019

carolynvs Sep 26, 2019

phillipahereza Sep 27, 2019

carolynvs Sep 26, 2019

carolynvs Sep 26, 2019

carolynvs Sep 26, 2019

carolynvs left a comment

carolynvs Sep 27, 2019

carolynvs Sep 27, 2019

carolynvs left a comment

phillipahereza commented Sep 30, 2019

Extend version command to help troubleshoot porter #642

Extend version command to help troubleshoot porter #642

Conversation

phillipahereza commented Sep 24, 2019 • edited Loading

What does this change

What issue does it fix

Notes for the reviewer

Checklist

msftclas commented Sep 24, 2019 • edited Loading

phillipahereza commented Sep 24, 2019

carolynvs commented Sep 25, 2019

carolynvs commented Sep 25, 2019

azure-pipelines bot commented Sep 25, 2019

Choose a reason for hiding this comment

carolynvs 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

phillipahereza commented Sep 30, 2019

phillipahereza commented Sep 24, 2019 •

edited

Loading

msftclas commented Sep 24, 2019 •

edited

Loading