Skip to content

Allow to specify Docker entrypoint using parameters #409

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.

Sign up for GitHub

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

Jump to bottom
Merged
alcohol merged 1 commit into from
Apr 7, 2017
Merged

Allow to specify Docker entrypoint using parameters #409

alcohol merged 1 commit into from
Apr 7, 2017

Conversation

@VasekPurchart
Copy link
Contributor

@VasekPurchart VasekPurchart commented Apr 6, 2017 •
edited
Loading

Using the composer/satis image is neat, because thanks to being the entrypoint to the container, you can call the container as you would the binary itself. Of course this is not always what you want and thanks for documenting the way in the readme using custom --entrypoint. Unfortunately sometimes you do not control the way the container is being run - typicaly in a docker "cloud", an example of this being the builds in GitLab.

I have dug into this and found out, that a lot of images, which are packaged binaries by default allow to specify a different entrypoint using command line parameters. Great example of this is the official php images. If you run them without parameters, you get the interactive shell. If you run them with a parameter (and its not an PHP parameter) then this is used as entrypoint.

This is achieved with a custom entrypoint script. I have adapted it for the pupose of Satis in this PR and I think it works pretty well.

Here are some situations before and after:

  1. without parameters (and no mounted volumes)

before:

sudo docker run --rm -it composer/satis
File not found: /build/satis.json

after:

sudo docker run --rm -it satis:docker-entrypoint
File not found: /build/satis.json
  1. Running with a parameter

before:

sudo docker run --rm -it composer/satis -v
Satis version 1.0.0-dev

...

after:

sudo docker run --rm -it satis:docker-entrypoint -v
Satis version 1.0.0-dev

...
  1. Running a specific command

before:

sudo docker run --rm -it composer/satis build
File not found: ./satis.json

after:

$ sudo docker run --rm -it satis:docker-entrypoint build
File not found: ./satis.json
  1. Specifying a binary

before:

$ sudo docker run --rm -it composer/satis /bin/sh

  [Symfony\Component\Console\Exception\CommandNotFoundException]  
  Command "/bin/sh" is not defined.

after:

sudo docker run --rm -it satis:docker-entrypoint /bin/sh
/satis #
  1. Running with parameters incorrectly

before:

sudo docker run --rm -it composer/satis xxx

  [Symfony\Component\Console\Exception\CommandNotFoundException]  
  Command "xxx" is not defined.

after:

sudo docker run --rm -it satis:docker-entrypoint xxx
/satis/bin/docker-entrypoint: exec: line 17: xxx: not found

As you can see the usual behavior does not change, only when intented and with the side effect that the unrecognized commands would behave differently. Please tell me if I have missed some case.

You can now run info about the other tools in the package, for example Composer:

sudo docker run --rm -it satis:docker-entrypoint composer -V
Composer version 1.4.1 2017-03-10 09:29:45

@alcohol
Copy link
Member

alcohol commented Apr 7, 2017 •
edited
Loading

Would you mind basing your docker-entrypoint on this one instead?

https://github.com/composer/docker/blob/master/docker-entrypoint.sh

And changing the Dockerfile to just point to it like this:

https://github.com/composer/docker/blob/master/Dockerfile.template#L30-L32

@alcohol
Copy link
Member

alcohol commented Apr 7, 2017

Also if I am not mistaken, you will need to add the docker-entrypoint.sh file to the .dockerignore file with an exclude rule. By default we ignore everything and then specifically unignore files we want to be able to copy into the image.

@VasekPurchart
Copy link
Contributor Author

VasekPurchart commented Apr 7, 2017

Would you mind basing your docker-entrypoint on this one instead?

Sure, I can do that, I have no experience with tiny so I didn't know how to do that correctly, thanks for pointing me in the right direction.

Also if I am not mistaken, you will need to add the docker-entrypoint.sh file to the .dockerignore file with an exclude rule.

I put the file in the bin dir, which is copied by a specific rule and also excluded in the .dockerignore. First I wanted to put it in the root, but since it is after all an executable I thought the bin dir would be more suitable. Plus it has the benefit that it does not further "polute" the root dir, where lately many files reside :)

Let me know if you want me to put it in the root.

@alcohol
Copy link
Member

alcohol commented Apr 7, 2017

I missed that, my bad. The bin directory is fine :-)

@VasekPurchart
Copy link
Contributor Author

VasekPurchart commented Apr 7, 2017

The PR is updated and green. I tested manually again all of the situations above and as expected nothing has changed. Plus there is now handling of satis as first argument to be consistent with the Composer image.

Btw I am not sure if we should update the readme section to mention these new possibilities or if it would cram the space too much?

@alcohol alcohol merged commit e2f8b9a into composer:master Apr 7, 2017
@alcohol
Copy link
Member

alcohol commented Apr 7, 2017

You are welcome to submit some more doc details. See #403 also. But it is not a requirement for this PR. Thanks for your contribution!

@VasekPurchart VasekPurchart deleted the docker-entrypoint branch April 7, 2017 14:20
@VasekPurchart
Copy link
Contributor Author

VasekPurchart commented Apr 7, 2017

I have now verified, that it is working with GitLab builds now, yay!

Thanks for being so responsive about this, saved me a lot of hacking around that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@VasekPurchart @alcohol