[WIP][RFC] New Image Builder Interface #817
Conversation
bolasim
requested review from
squidarth,
helenlyang,
aspctu,
joostinyi and
marius-baseten
| self.tag = tag | ||
| self._as = AS | ||
|
|
||
| def serialize(self): |
There was a problem hiding this comment.
Add str return type.
When I read "serialize" I usually assume it's bytes.
Maybe even consider renaming this method to to_str or as_str so it's immediately deductable from the name. Since this gets later called as write_text(image.serialize()) it might also make sense as_text.
| ret += f":{self.tag}" | ||
| if self._as is not None: | ||
| ret += f" AS {self._as}" | ||
| return ret |
There was a problem hiding this comment.
Nit: I know that in this context runtime is not really an issue, but I got used to try to always use efficient implementations - unless it's significantly more complex and less readable.
So here you could collect the conditional command in parts: list[str] and and the end " ".join(parts).
|
|
||
|
|
||
| class FromCommand(Command): | ||
| def __init__(self, image, tag=None, AS=None): |
There was a problem hiding this comment.
Add type annotations.
| if self.mounts is not None: | ||
| for mount in self.mounts: | ||
| cmd += f"--mount={mount} " | ||
| cmd += self.command |
There was a problem hiding this comment.
Nit: like previous comment, collect parts in list and join at the end. This loop has in principle quadratic runtime.
| def __init__(self) -> None: | ||
| self._base_image = "" | ||
| self._commands: list[Command] = [] | ||
| self._base_image: Optional[str] = None |
There was a problem hiding this comment.
Nit: To be very precise, class attributes should be type annotated before __init__.
class Image:
_base_image: Optional[str]
_commands: list[str]
def __init__(self) -> None:
...
There was a problem hiding this comment.
when you say class attributes, this meand instance variables? They belong to self not cls, right?
There was a problem hiding this comment.
Yes, both instance and class variables would be on top level and distinguished buy ClassVar. E.g.
class Image:
_some_data: str # Instance variable.
_registry: ClassVar[Registry] # Class variable
def __init__(self) -> None:
...
See https://peps.python.org/pep-0526/#class-and-instance-variable-annotations
|
|
||
| def serialize(self) -> str: | ||
| dockerfile = "# syntax = docker/dockerfile:1\n" # Support BuildKit | ||
| dockerfile += "# Auto-generated by truss.build, do not edit!\n" |
There was a problem hiding this comment.
Nit: consider making these string literals module- (or class-) private constants (_SYNTAX_HEADER: ClassVar[str] = "...")
I really think the less string literals appear somewhere hidden/inline in the depth of code the better...
| dockerfile = "# syntax = docker/dockerfile:1\n" # Support BuildKit | ||
| dockerfile += "# Auto-generated by truss.build, do not edit!\n" | ||
| for command in self._commands: | ||
| dockerfile += command.serialize() + "\n" |
There was a problem hiding this comment.
Use return "\n".join(self._commands) -> shorter code and more performant.
|
|
||
| def apt_install( | ||
| self, | ||
| *packages: Union[str, List[str]], |
There was a problem hiding this comment.
Optional/Opinion: is there a reason to support both variable args and list?
While it may seem more user friendly to have multiple and flexible options, I personally think it's less mental load to have exactly one right way of doing things and seeing that everywhere consistently, as opposed to having to make a decision each time knowing about "two forms".
There was a problem hiding this comment.
I am mostly trying to be API compatible with modal but we can have our own take here. Open to discussion.
| self._commands.append(WorkdirCommand(path)) | ||
| return self | ||
|
|
||
| def env(self, vars: Dict[str, str]) -> "Image": |
There was a problem hiding this comment.
Nit: to denote that vars is immutable ("const") for this function, use from typing import Mapping.
馃殌 What
馃捇 How
馃敩 Testing