Skip to content
This repository has been archived by the owner on May 27, 2020. It is now read-only.

Preserve the container's root process environment #43

Merged
merged 1 commit into from Oct 9, 2014
Merged

Preserve the container's root process environment #43

merged 1 commit into from Oct 9, 2014

Conversation

kolypto
Copy link
Contributor

@kolypto kolypto commented Oct 8, 2014

Fixes #18, when entering a container, the environment is preserved:

kolypto@localhost% ./docker-enter myapp   env | fgrep DB_1_PORT_5432_TCP_ADDR 
DB_1_PORT_5432_TCP_ADDR=172.17.0.63

It also works as a login shell, without the command:

kolypto@localhost% docker-enter myapp
root@b6b93bc18ec2:/# env | fgrep DB_1_PORT_5432_TCP_ADDR       
DB_1_PORT_5432_TCP_ADDR=172.17.0.63

(that means, bash is not hardcoded).

Also did a bit of refactoring on the code, but nothing serious:

  • Replaced if [ ... ] ; then .. fi with [ ... ] && ...
  • Put $1 into $COMMAND
  • Refactored if [ -z "$1" ]; then into setting a default value for $COMMAND

@jpetazzo , what do you think?

if [ -z "$PID" ]; then
exit 1
fi
[ -z "$PID" ] && exit 1

Choose a reason for hiding this comment

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

Although the old notation is longer, it's also clearer for not-so-fluent shell programmers what it does. So if the only reason is to make the script shorter, I'd like to keep the old notation (just my opinion, fwiw)

@thaJeztah
Copy link

Apart from my (preferential) comments, looks good. Haven't been able to test it yet as I'm not at my computer.

Thanks!

@kolypto
Copy link
Contributor Author

kolypto commented Oct 8, 2014

@thaJeztah, with all those long notations the "small shell script" promised in the README is actually surprisingly long!

However, I agree.
Still, I'd rather see a working solution finally released than to have this issue as a show-stopper, right?

@kolypto
Copy link
Contributor Author

kolypto commented Oct 8, 2014

btw thanks to @damccue for the solution, it's really brilliant!

@thaJeztah
Copy link

actually surprisingly long!

Hm, if line count is the definition of long here, yes. Still I prefer readability (again, just my opinion); if it must be short, then the show script could have been put on a single line, shorter variable names and comments removed, And I don't think the extra lines would hinder performance?

But I agree, it's no real show stopper.

In all cases, I'll leave it up to @jpetazzo 😄

Oh, FYI, docker 1.3 is scheduled for release today/tomorrow and will offer "nsenter" of the box via docker exec. I'm running the current master version on my test server and so far it's working really great, so you might want to watch for that release as well

@jpetazzo
Copy link
Owner

jpetazzo commented Oct 9, 2014

Nice. Let's ship this! :shipit:

Thanks a lot.

I have no personal opintion about if; then vs && so I'll just leave it as it is to save everyone's time ;-)

jpetazzo pushed a commit that referenced this pull request Oct 9, 2014
Preserve the container's root process environment
@jpetazzo jpetazzo merged commit b9377ad into jpetazzo:master Oct 9, 2014
@thaJeztah
Copy link

Thanks @kolypto!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants