deis / builder

Git server and application builder for Deis Workflow
https://deis.com
MIT License
40 stars 41 forks source link

chore(deis-builder-rc.yaml,Dockerfile): remove entrypoint and specify it in the manifest #36

Closed arschles closed 8 years ago

technosophos commented 8 years ago

What's your thinking about how we should do this across projects? Is this solving a special case, or are you thinking this is the way we should do things?

bacongobbler commented 8 years ago

I think we should leave this in the Dockerfile, IMO. That way we can still use this docker image outside of kubernetes without having to know the boot incantation.

I think this also break things as /bin/boot is not called in entrypoint.sh from what I can see.

arschles commented 8 years ago

Sorry about the upcoming wall of text:

@technosophos I made this change so I could use the mc client in the image locally, for debugging purposes. I had intended to back it out but realized that, since this image has more than 1 artifact in it (even though that may not be a best practice), I believe it's overall more flexibly to not have an ENTRYPOINT.

I won't say that we should never use ENTRYPOINT, but in this case I think the flexibility of removing it is worthwhile.

Relating to what @bacongobbler said, I could certainly add the CMD back in, and I'm now thinking that is probably a happy medium - we could remove the command: from the manifest while keeping the simplicity and retaining the aforementioned flexibility.

Also, the Makefile builds a builder binary in this project instead of a boot. I know the prototype repo builds boot so I'm considering it standard. I've created https://github.com/deis/builder/issues/37 to make the change later.

Thoughts on any of the above?

bacongobbler commented 8 years ago

+1 on migrating the ENTRYPOINT to the boot script. That'll give you the ability to use the mc client while still retaining backwards compat.

technosophos commented 8 years ago

Sounds good. The binary named builder is an artifact of the 1.x tree and should be changed.

arschles commented 8 years ago

tracking the builder=>boot change in https://github.com/deis/builder/issues/37

smothiki commented 8 years ago

Im good with this . I'll give LGTM once the rebase is done

arschles commented 8 years ago

closing, as #72 made these changes anyway