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

Instruct user to set DOCKER_TLS_VERIFY#269

Merged
SvenDowideit merged 3 commits intoboot2docker:masterfrom
aanand:docker-tls-verify-instruction
Oct 14, 2014
Merged

Instruct user to set DOCKER_TLS_VERIFY#269
SvenDowideit merged 3 commits intoboot2docker:masterfrom
aanand:docker-tls-verify-instruction

Conversation

@aanand
Copy link
Copy Markdown
Contributor

@aanand aanand commented Oct 10, 2014

This is waiting on moby/moby#8503

@tianon
Copy link
Copy Markdown
Contributor

tianon commented Oct 10, 2014

❤️

Thanks for being on top of this.

cmds.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: or fmt.Println(" export DOCKER_TLS_VERIFY=1") ... your call.
LGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the idea was to parallel the one above so it's pretty. 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tianon is correct - nice to have it all line up, I reckon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed, +1; makes it easier to spot if we ever screw it up in a case like this. 😄

@SvenDowideit
Copy link
Copy Markdown
Contributor

can we perhaps replace this with a note to tell them to run $(boot2docker shellinit) ?

mmm, though sadly, both these are bash-ish, and not fish-able.

OH. and no, this is actually not enough.

in adding this new Var, you also need to set the DOCKER_CERT_DIR

and cmdShellinit needs to set this too - as thats the better way to set the ENV.

@bfirsh
Copy link
Copy Markdown
Contributor

bfirsh commented Oct 13, 2014

There is also a bit of code that checks whether the environment is set:

https://github.com/boot2docker/boot2docker-cli/blob/master/cmds.go#L146-L151

It'd be quite nice if it could print the commands anyway so I can copy & paste into other shells. Perhaps that logic should print the envvars anyway and then say "but it looks like you've already set them in this shell, so you're good to go!" or something.

@SvenDowideit
Copy link
Copy Markdown
Contributor

thats how the existing code works :) - but you need to add support to the shellinit cmd too

@bfirsh
Copy link
Copy Markdown
Contributor

bfirsh commented Oct 13, 2014

Sorry, the point I was trying to make is that now needs to check for the new environment variable.

@bfirsh bfirsh mentioned this pull request Oct 13, 2014
@bfirsh
Copy link
Copy Markdown
Contributor

bfirsh commented Oct 13, 2014

$(boot2docker shellinit) doesn't work because this line outputs to stdout:

fmt.Printf("Writing %s:\n", certFile)

If any of DOCKER_HOST, DOCKER_CERT_PATH or DOCKER_TLS_VERIFY are
incorrect, `boot2docker up` will inform the user, and
`boot2docker shellinit` will output the correct commands to set/unset
them.

Signed-off-by: Aanand Prasad <[email protected]>
@aanand
Copy link
Copy Markdown
Contributor Author

aanand commented Oct 13, 2014

@bfirsh: fixed.

All: OK, we now check all 3 environment variables and output what needs to be set/unset.

@tianon
Copy link
Copy Markdown
Contributor

tianon commented Oct 13, 2014

LGTM

@bfirsh bfirsh added this to the 1.3.0 milestone Oct 13, 2014
@SvenDowideit
Copy link
Copy Markdown
Contributor

LGTM - though i havn't tested

@SvenDowideit
Copy link
Copy Markdown
Contributor

moby/moby#8503 is in the 1.3.0 merge PR, moby/moby#8323 so I'm going to merge all these PRs

SvenDowideit added a commit that referenced this pull request Oct 14, 2014
@SvenDowideit SvenDowideit merged commit 85cb517 into boot2docker:master Oct 14, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants