deis / controller

Deis Workflow Controller (API)
https://deis.com
MIT License
41 stars 53 forks source link

fix(api): split command arguments by space if entrypoint is not `/bin/bash -c` #1264

Closed bacongobbler closed 7 years ago

bacongobbler commented 7 years ago

closes deis/workflow#715

deis-bot commented 7 years ago

@mboersma, @Joshua-Anderson and @krancour are potential reviewers of this pull request based on my analysis of git blame information. Thanks @bacongobbler!

bacongobbler commented 7 years ago

since we already make it clear that bash should be present in deis pull apps, this should not affect anyone. Doubly so that it was broken in the first place.

bacongobbler commented 7 years ago

On the contrary, this may not be the fix as this will override docker images that have an entrypoint defined. I think a separate fix is necessary here.

cristiangraz commented 7 years ago

@bacongobbler Will this only apply to entries in the Procfile named cmd? Just want to confirm that using a Docker image deployed via deis pull with a Procfile like this would still work:

cmd: yarn start-service
worker1: ./worker --name worker1
worker2: ./worker --name worker2

Since the other process types are still using the same docker image, we'd need to take into account multiple commands in each process type, not just the cmd one:

cmd: ["yarn", "start-service"]
worker1: ["./worker", "--name", "worker1"]
worker2: ["./worker", "--name", "worker2"]

I apologize in advance if this is already the case, I'm not familiar with the Deis internals or python, but saw a reference to a dockerfile and container_type == cmd in the diff so just wanted to clarify.

Also will the Dockerfile need to be present in the WORKDIR of the image for this to work (I have it in .dockerignore)

bacongobbler commented 7 years ago

Yes, the bug in question only applies to cmd process types. If you rename cmd to web it should work for you just fine.

bacongobbler commented 7 years ago

k, so the new proposed fix should do the following:

  1. respect the docker image's defined ENTRYPOINT
  2. supply the arguments as a list to the manifest

With this, I can successfully deploy example-dockerfile-procfile-http as expected:

><> cat Procfile
cmd: python -m SimpleHTTPServer 5000
><> deis pull deis/example-dockerfile-python
codecov-io commented 7 years ago

Codecov Report

Merging #1264 into master will increase coverage by <.01%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1264      +/-   ##
==========================================
+ Coverage   87.25%   87.26%   +<.01%     
==========================================
  Files          44       44              
  Lines        3884     3887       +3     
  Branches      674      675       +1     
==========================================
+ Hits         3389     3392       +3     
  Misses        327      327              
  Partials      168      168

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1156475...02206b7. Read the comment docs.