Skip to content
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

Shorten printed Windows paths on docker help command #10886

Merged
merged 3 commits into from Feb 19, 2015
Merged

Shorten printed Windows paths on docker help command #10886

merged 3 commits into from Feb 19, 2015

Conversation

@ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Feb 18, 2015

This makes use of %USERPROFILE% as a substitute for
~ on Windows and prints shorter strings for default
cert paths etc.

Removes string escaping/quotes around default
path values printed in docker help command as they
are not really necessary and adds double backslashes
() on windows to gain some chars and reworded
messages with shorter ones for --tls and -H flags.

Fixes test TestMainHelpWidth on Windows (width
is exactly at 80 char limit).

On Unix it becomes:

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @duglin @SvenDowideit @unclejack @jfrazelle

@@ -26,10 +22,12 @@ func TestMainHelpWidth(t *testing.T) {
lines := strings.Split(out, "\n")
for _, line := range lines {
if len(line) > 80 {
t.Log(out)
Copy link
Contributor

@icecrime icecrime Feb 18, 2015

Is this a testing artifact?

Copy link
Contributor Author

@ahmetb ahmetb Feb 18, 2015

Oh my bad.

@icecrime
Copy link
Contributor

@icecrime icecrime commented Feb 18, 2015

I have a change of behavior that I don't really explain (see the path below).

Without the PR:

$  docker --help | grep ca.pem
  --tlscacert="/home/icecrime/.docker/ca.pem"    Trust only remotes providing a certificate signed by the CA given here

With the PR:

$ docker --help | grep ca.pem
  --tlscacert=~/.docker/ca.pem         Trust certs signed only by this CA

@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Feb 18, 2015

@icecrime umm "Without the PR" is probably the stable build and doesn't contain #10547. That's when ~ is introduced.

@icecrime
Copy link
Contributor

@icecrime icecrime commented Feb 18, 2015

Oh good catch, that explains while I couldn't explain the change from reading the code :-)

}
if home != "" && strings.Contains(line, home) {
t.Fatalf("Line should use ~ instead of %q:\n%s", home, line)
t.Fatalf("Line should use home shortcut instead of %q:\n%s", home, line)
Copy link
Contributor

@icecrime icecrime Feb 18, 2015

Just an idea, but shouldn't we just create the following (either that or a const conditioned by a build tag, whatever):

func homeDirectorySpecifier() string {
    if runtime.GOOS == "windows" {
        return "%USERPROFILE%"
    }
    return "~"
}

That would help us keep the same output as we have today in these error messages (t.Fatalf("Line should use %s instead of...", homeDirectorySpecifier(), ...)), and could be use below for PrintDefaults() implementation.

Copy link
Contributor Author

@ahmetb ahmetb Feb 18, 2015

@icecrime I actually thought about putting this to pkg/homedir but I'm not sure that's the right place. we use exact same piece of code at pkg/mflag/flag.go below as you can see. suggestions?

Copy link
Contributor Author

@ahmetb ahmetb Feb 18, 2015

@icecrime oh you already said that. okay should I expose this in pkg/mflag or pkg/homedir?

Copy link
Contributor

@icecrime icecrime Feb 18, 2015

Mmm sorry I have too little overview of pkg/ to provide a relevant opinion here... @tianon maybe? <3

Copy link
Contributor

@duglin duglin Feb 19, 2015

I like the idea of putting all of this home-related code into pkg/homedir

Copy link
Contributor Author

@ahmetb ahmetb Feb 19, 2015

updated this by putting this functionality into pkg/homedir. please review again.

Copy link
Member

@tianon tianon Feb 19, 2015

👍

@SvenDowideit
Copy link
Contributor

@SvenDowideit SvenDowideit commented Feb 19, 2015

nice!

ahmetb added 3 commits Feb 19, 2015
In order to fit printed messages to fit 80 chars,
rewording messages for `-H` and `--tls` flags.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
This makes use of `%USERPROFILE%` as a substitute for
`~` on Windows and prints shorter strings for default
cert paths etc.

Also removes string escaping/quotes around default
path values printed in `docker help` command as they
are not really necessary and adds double backslashes
(\\) on windows.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@icecrime
Copy link
Contributor

@icecrime icecrime commented Feb 19, 2015

LGTM

@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Feb 19, 2015

@icecrime thanks for reviewing. Jenkins is also happy.

https://jenkins.dockerproject.com/job/Windows-PRs/65/console
=== RUN TestMainHelpWidth
[PASSED]: help - verify main width
--- PASS: TestMainHelpWidth (0.02s) 👍

@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Feb 19, 2015

LGTM

jessfraz pushed a commit that referenced this issue Feb 19, 2015
…th-fix

Shorten printed Windows paths on docker help command
@jessfraz jessfraz merged commit e3fa8b3 into moby:master Feb 19, 2015
1 of 2 checks passed
@ahmetb ahmetb deleted the win-cli/TestMainHelpWidth-fix branch Feb 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants