Skip to content
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

Let CMD <cmd> in the Dockerfile template be configurable #957

Closed
wants to merge 5 commits into from
Closed

Let CMD <cmd> in the Dockerfile template be configurable #957

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a806c7
Add command variable to Jinja Dockerfile template and corresponding m…
AliMirlou Jun 6, 2020
e21086e
Fix whitespaces
AliMirlou Jun 15, 2020
f0a3fb0
Add handling of CMD shell form
AliMirlou Jul 24, 2020
f7ec92e
Remove redundant import
AliMirlou Sep 12, 2020
4ff3a78
Lint files
AliMirlou Sep 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion repo2docker/buildpacks/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CMD {{ command }}
CMD {{ cmd }}


{% if appendix -%}
# Appendix:
Expand Down Expand Up @@ -482,6 +482,14 @@ def get_start_script(self):
"""
return None

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`.
Comment on lines +485 to +489
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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`.

"""
return ["jupyter", "notebook", "--ip", "0.0.0.0"]

@property
def binder_dir(self):
has_binder = os.path.isdir("binder")
Expand Down Expand Up @@ -554,6 +562,19 @@ def render(self):
for k, v in self.get_build_script_files().items()
}

# 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__
)
Comment on lines +565 to +576
Copy link
Member

Choose a reason for hiding this comment

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

  • 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.
Suggested change
# 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.


return t.render(
packages=sorted(self.get_packages()),
path=self.get_path(),
Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
command=cmd,
cmd=cmd,

appendix=self.appendix,
)

Expand Down