Tweak output of troubleshoot command when the CLI is unconfigured #675

kytrinyx · 2018-07-24T22:00:01Z

Right now the output of troubleshoot is a bit unhelpful when the CLI is unconfigured.

Configuration
----------------
Home:      
Workspace: 
Config:    
API key:   <not configured>
Find your API key at https://exercism.io/my/settings

API Reachability
----------------
Exercism:
    * /ping
    * [Get /ping: unsupported protocol scheme &#34;&#34;]
    * 52.222µs

The configuration section should be able to tell us what the home directory is, no matter what (that should not rely on configuring the CLI).

We can output what the default workspace would be, as long as we clarify that it's not actually configured.

The Config field should show the default config directory, even if we haven't create it.

Lastly, we should use the default site URL for ping.

This PR fixes all of the above:

Configuration
----------------
Home:      /Users/kytrinyx
Workspace: /Users/kytrinyx/Test-On-Branch (default)
Config:    /Users/kytrinyx/.config/test-on-branch
API key:   <not configured>
Find your API key at https://exercism.io/my/settings

API Reachability
----------------
Exercism:
    * https://api.exercism.io/v1/ping
    * [connected]
    * 663.848701ms

It is probably easiest to review commit-by-commit.

Closes #671

Instead of passing just the viper config, which could be empty, pass the full config.Configuration value, which has access to defaults. This will let us give better debugging output.

The home directory is never written to the viper config, we get it from the environment, which is stored in the config.Configuration value.

Rather than providing an empty string for the workspace in the troubleshoot command when the CLI is unconfigured, this now says Workspace: /the/actual/path (default)

In the old CLI we had a single config file. Now we potentially have several. They will all live in the config directory. The troubleshoot command now outputs the directory instead of the file. If the troubleshoot command is unconfigured, this would previously have given an empty string for the config file. Now it will always print the default config directory, unless the user has specifically defined an override using environment variables.

It was unnecessary to have an additional local variable.

The config package has logic for the settings URL, we shouldn't be hard-coding it here.

nywilken · 2018-07-24T23:47:20Z

Is the <not configured> for the API token enough to denote that the CLI was never configured? Or should we be a bit more explicit with something like the following

Workspace:  <not configured>  (default /Users/kytrinyx/Test-On-Branch)

nywilken

I left a couple of questions for you. But this is otherwise good.

nywilken · 2018-07-25T01:32:32Z

cmd/troubleshoot.go

@@ -112,7 +114,7 @@ func (status *Status) check() (string, error) {
 	status.Version = newVersionStatus(status.cli)
 	status.System = newSystemStatus()
 	status.Configuration = newConfigurationStatus(status)
-	status.APIReachability = newAPIReachabilityStatus(status.cfg.GetString("apibaseurl"))
+	status.APIReachability = newAPIReachabilityStatus(status.cfg)


Passing the config here seems a bit heavy handed. What if instead of the original value you passed config.InferSiteURL(v.GetString("apibaseurl"))? Would that not achieve the same thing?

No, the site URL is not the same as the API, so it would call https://exercism.io/ping (which would fail with a 404).

kytrinyx · 2018-07-25T04:58:52Z

Is the <not configured> for the API token enough to denote that the CLI was never configured?

The <not configured> for the API token means that the API token is not configured. It doesn't speak for the rest of the values.

I went with (default) because if you don't explicitly overwrite the workspace, that is what will be written, but I think that your suggestion would be fine as well. I don't have very strong feelings either way. Which do you prefer?

nywilken · 2018-07-25T21:57:09Z

I went with (default) because if you don't explicitly overwrite the workspace, that is what will be written, but I think that your suggestion would be fine as well. I don't have very strong feelings either way. Which do you prefer?

If you modify the config file and remove only the workspace config the CLI reports that the API token is also not configured even if it is part of the config. It appears to me that we should should report for the Workspace so that if a user opens an issue we can quickly see that their CLI is entirely unconfigured (I.e missing the most important part the workspace)

nywilken

Actually, I think what you have is better. It is easy to see that workspace is not configured because of the string (default).

Katrina Owen added 7 commits July 24, 2018 14:41

Pass full configuration value to troubleshooting status

61b1b61

Instead of passing just the viper config, which could be empty, pass the full config.Configuration value, which has access to defaults. This will let us give better debugging output.

Fix output for 'home' in troubleshoot command

635f5dd

The home directory is never written to the viper config, we get it from the environment, which is stored in the config.Configuration value.

Output default workspace in troubleshoot if unconfigured

0383f6b

Rather than providing an empty string for the workspace in the troubleshoot command when the CLI is unconfigured, this now says Workspace: /the/actual/path (default)

Inline token in troubleshoot config status

ec1c4ee

It was unnecessary to have an additional local variable.

Use proper helper method for settings URL in troubleshoot command

edafb3f

The config package has logic for the settings URL, we shouldn't be hard-coding it here.

Fix ping when troubleshoot is unconfigured

479a9bd

kytrinyx requested a review from nywilken July 24, 2018 22:00

kytrinyx added the size/small label Jul 24, 2018

nywilken reviewed Jul 25, 2018

View reviewed changes

nywilken approved these changes Jul 26, 2018

View reviewed changes

nywilken merged commit 41999e7 into master Jul 26, 2018

nywilken deleted the unconfigured-troubleshoot branch July 26, 2018 03:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak output of troubleshoot command when the CLI is unconfigured #675

Tweak output of troubleshoot command when the CLI is unconfigured #675

kytrinyx commented Jul 24, 2018 •

edited

Loading

nywilken commented Jul 24, 2018 •

edited

Loading

nywilken left a comment

nywilken Jul 25, 2018

kytrinyx Jul 25, 2018

kytrinyx commented Jul 25, 2018

nywilken commented Jul 25, 2018

nywilken left a comment

Tweak output of troubleshoot command when the CLI is unconfigured #675

Tweak output of troubleshoot command when the CLI is unconfigured #675

Conversation

kytrinyx commented Jul 24, 2018 • edited Loading

nywilken commented Jul 24, 2018 • edited Loading

nywilken left a comment

Choose a reason for hiding this comment

nywilken Jul 25, 2018

Choose a reason for hiding this comment

kytrinyx Jul 25, 2018

Choose a reason for hiding this comment

kytrinyx commented Jul 25, 2018

nywilken commented Jul 25, 2018

nywilken left a comment

Choose a reason for hiding this comment

kytrinyx commented Jul 24, 2018 •

edited

Loading

nywilken commented Jul 24, 2018 •

edited

Loading