Let CMD <cmd> in the Dockerfile template be configurable
#957
Conversation
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
|
I haven't reflected the changes in the documentations and changelog yet. Waiting for someone's approval first. |
|
Also can anyone explain why the "lint" check is failing? Is it because of too many characters in one line? |
|
We use pre-commit and the black autoformatter. If you install pre-commit (e.g. with pip) and run the code should pass the CI lint check. |
|
@manics Can you review this PR yourself? It's not a major change. |
|
@betatim I need some approval to continue this PR. Can you give a quick review? |
|
Or @minrk perhaps? |
|
@manics Can you review this PR now? |
consideRatio
changed the title
consideRatio
changed the title
get_command in base's buldpack
There was a problem hiding this comment.
I think it could be acceptable to provide this, but I'm not sure.
I made a few suggestions towards making me feel comfortable with the implementation. I think a very strong case needs to be made for a string replacement still though, and it probably needs to be documented if a strong case for it is presented.
| def get_command(self): | ||
| """ | ||
| The default command to be run by docker. | ||
|
|
||
| This should return a list of strings to be used as Dockerfile `CMD`. |
There was a problem hiding this comment.
| def get_command(self): | |
| """ | |
| The default command to be run by docker. | |
| This should return a list of strings to be used as Dockerfile `CMD`. | |
| def get_cmd(self): | |
| """ | |
| A list of strings to be used for the Dockerfile's `CMD`. |
| # If get_command returns a list or a tuple, it will be stringified (exec form). | ||
| # If it returns a string, it will be used as is (shell form). | ||
| cmd = self.get_command() | ||
| if isinstance(cmd, tuple): | ||
| cmd = list(cmd) | ||
| if isinstance(cmd, list): | ||
| cmd = str(cmd).replace("'", '"') | ||
| if not isinstance(cmd, str): | ||
| raise ValueError( | ||
| 'Method "get_command" of buildpack "%s" must return a string, a list or a tuple.' | ||
| % self.__class__.__name__ | ||
| ) |
There was a problem hiding this comment.
- Removed validation as this isn't user facing, just for those making custom buildpacks. My goal is to minimize complexity and ease the long term maintenance and code comprehension by doing that.
- Removed tuples fixup for similar reasons.
| # If get_command returns a list or a tuple, it will be stringified (exec form). | |
| # If it returns a string, it will be used as is (shell form). | |
| cmd = self.get_command() | |
| if isinstance(cmd, tuple): | |
| cmd = list(cmd) | |
| if isinstance(cmd, list): | |
| cmd = str(cmd).replace("'", '"') | |
| if not isinstance(cmd, str): | |
| raise ValueError( | |
| 'Method "get_command" of buildpack "%s" must return a string, a list or a tuple.' | |
| % self.__class__.__name__ | |
| ) | |
| cmd = self.get_command() | |
| if isinstance(cmd, list): | |
| cmd = str(cmd).replace("'", '"') |
I don't understand what is going on with regards to the replacement operation that remains, but I suspect its not reasonable to provide either.
| @@ -568,6 +589,7 @@ def render(self): | |||
| base_packages=sorted(self.get_base_packages()), | |||
| post_build_scripts=self.get_post_build_scripts(), | |||
| start_script=self.get_start_script(), | |||
| command=cmd, | |||
There was a problem hiding this comment.
| command=cmd, | |
| cmd=cmd, |
| @@ -191,7 +191,7 @@ | |||
| ENTRYPOINT ["/usr/local/bin/repo2docker-entrypoint"] | |||
|
|
|||
| # Specify the default command to run | |||
| CMD ["jupyter", "notebook", "--ip", "0.0.0.0"] | |||
| CMD {{ command }} | |||
There was a problem hiding this comment.
| CMD {{ command }} | |
| CMD {{ cmd }} |
consideRatio
changed the title
get_command in base's buldpack
consideRatio
added
new
and removed
maintenance
consideRatio
changed the title
consideRatio
changed the title
CMD <cmd> in the Dockerfile template be configurable
|
Thanks for the PR. I'm closing since there's been no updates, and it's already possible to override |
A configurable CMD might not be something this project wants but there are many use-cases for this, like starting the notebook in a different way or even using Repo2Docker as a library to build non-jupyter Docker images. It would be nice to have this feature.
By the way, this change is completely backward-compatible and does not affect anyone who does not want to change the CMD.