Skip to content
This repository was archived by the owner on Feb 27, 2018. It is now read-only.

Don't mix unset and exports, and cater for spaces in the cert path #351

Closed
SvenDowideit wants to merge 1 commit into from
Closed

Don't mix unset and exports, and cater for spaces in the cert path #351

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 9 additions & 16 deletions cmds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,21 +198,14 @@ func checkEnvironment(socket, certPath string) bool {
}

func printExport(socket, certPath string) {
for name, value := range exports(socket, certPath) {
switch filepath.Base(os.Getenv("SHELL")) {
case "fish":
if value == "" {
fmt.Printf(" set -e %s\n", name)
} else {
fmt.Printf(" set -x %s %s\n", name, value)
}
default: // default command to export variables POSIX shells, like bash, zsh, etc.
if value == "" {
fmt.Printf(" unset %s\n", name)
} else {
fmt.Printf(" export %s=%s\n", name, value)
}
}
env := exports(socket, certPath)
Copy link
Contributor

@gmlewis gmlewis Mar 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside to this is that if another env var is introduced tomorrow, this change will no longer report it, as it hard-codes the env var keys. Your call, though, Sven.

nit: looks like 201/202 were not both formatted by gofmt.

Otherwise, LGTM.

Copy link
Contributor Author

@SvenDowideit SvenDowideit Mar 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the magic future proofing was neat looking, and worked for TLS, but with b2d being slowly on the way out, and as I fixed the spaced path in docker-machine, figure going with the same code they have is safe enough :)

switch filepath.Base(os.Getenv("SHELL")) {
case "fish":
fmt.Printf("set -x DOCKER_TLS_VERIFY %s\nset -x DOCKER_CERT_PATH %s\nset -x DOCKER_HOST %s\n",
env["DOCKER_TLS_VERIFY"], strings.Replace(env["DOCKER_CERT_PATH"], ` `, `\ `, -1), env["DOCKER_HOST"])
default:
fmt.Printf("export DOCKER_TLS_VERIFY=%s DOCKER_CERT_PATH=%s DOCKER_HOST=%s\n",
Copy link
Contributor

@tianon tianon Mar 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty strongly -1 on a hard-coded list like this, especially when we already have key-value pairs telling us the full list of variables we're expected to set. This would mean that anyone adding a new variable to the list of vars we set would have to know all the places they need to update, which is error-prone, especially if we look to add more shells here.

Copy link
Contributor Author

@SvenDowideit SvenDowideit Mar 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - so you're -1 on the code in docker machine....

I don't really have time for this atm - can you please take the PR over and fix the bug?

Copy link
Contributor

@tianon tianon Mar 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If got the same unnecessary hard-coding, then yes, I'm -1 on the code in docker-machine.

Copy link
Contributor

@tianon tianon Mar 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/docker/machine/blob/7a44b3575c7df23cde0e04aa0a755a05a46e1cbe/commands.go#L529 << docker-machine doesn't have a map with the keys already

env["DOCKER_TLS_VERIFY"], strings.Replace(env["DOCKER_CERT_PATH"], ` `, `\ `, -1), env["DOCKER_HOST"])
}
}

Expand All @@ -225,7 +218,7 @@ func exports(socket, certPath string) map[string]string {
if certPath == "" {
out["DOCKER_TLS_VERIFY"] = ""
} else {
out["DOCKER_TLS_VERIFY"] = "1"
out["DOCKER_TLS_VERIFY"] = "yes"
Copy link
Contributor

@tianon tianon Mar 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for changing this one?

Copy link
Contributor Author

@SvenDowideit SvenDowideit Mar 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to make it the same as in machine....

I'm basically taking the POV that if our code looks more like theirs, then its will be easier for people that might need to back-port changes.

Copy link
Contributor

@tianon tianon Mar 17, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@SvenDowideit SvenDowideit Mar 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/docker/machine/pull/696/files

}

return out
Expand Down