Refactor TrackedContainer run_detached/exec_cmd functions #2256
Conversation
| @@ -15,13 +15,13 @@ | |||
| def test_cli_args(container: TrackedContainer, http_client: requests.Session) -> None: | |||
| """Image should respect command line args (e.g., disabling token security)""" | |||
| host_port = find_free_port() | |||
| running_container = container.run_detached( | |||
There was a problem hiding this comment.
There is no need to return anything from run_detached - TrackedContainer has a container inside and it's better to provide specific functions we need (this way we're encapsulating docker functions we use)
| command=["start-notebook.py", "--IdentityProvider.token=''"], | ||
| ports={"8888/tcp": host_port}, | ||
| ) | ||
| resp = http_client.get(f"http://localhost:{host_port}") | ||
| resp.raise_for_status() | ||
| logs = running_container.logs().decode() | ||
| logs = container.get_logs() |
There was a problem hiding this comment.
This is used often and it is cleaner to have a separate method
| @@ -47,8 +47,7 @@ def test_nb_user_change(container: TrackedContainer) -> None: | |||
| ) | |||
| command = f'stat -c "%F %U %G" /home/{nb_user}/.jupyter' | |||
| expected_output = f"directory {nb_user} users" | |||
| exec_result = running_container.exec_run(command, workdir=f"/home/{nb_user}") | |||
| output = exec_result.output.decode().strip("\n") | |||
There was a problem hiding this comment.
There were some .strin("\n") - I don't think they are actually needed, so let's try to remove them
| if status == "healthy": | ||
| return status | ||
|
|
||
| return get_health(running_container, docker_client) | ||
| return status |
There was a problem hiding this comment.
Already have value above, no need to get it again
| network=ipv6_network, | ||
| volumes={str(host_data_dir): {"bind": cont_data_dir, "mode": "ro,z"}}, | ||
| tty=True, | ||
| ) | ||
|
|
||
| command = ["python", f"{cont_data_dir}/check_listening.py"] |
There was a problem hiding this comment.
For simplicity made cmd always a string.
python docker allows both list or str, having str loogs good for us.
Additional improvement - if something goes wrong, copy-paste command will be easier
| exec_result = running_container.exec_run(command) | ||
| LOGGER.info(exec_result.output.decode()) | ||
| assert exec_result.exit_code == 0 |
There was a problem hiding this comment.
There is no need to do any of this, because it's all part of a new method
| @@ -109,25 +109,16 @@ def get_package_import_name(package: str) -> str: | |||
| return PACKAGE_MAPPING.get(package, package) | |||
|
|
|||
|
|
|||
| def _check_import_package( | |||
There was a problem hiding this comment.
With new method, this becomes one simple call, so removed some methods here
| tty=True, | ||
| command=["bash", "-c", "sleep infinity"], | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _conda_export_command(from_history: bool) -> list[str]: |
There was a problem hiding this comment.
In my opinion this adds complexity, but doesn't add too much value, so removed this method
| return self.requested | ||
|
|
||
| def _execute_command(self, command: list[str]) -> str: | ||
| """Execute a command on a running container""" | ||
| exec_result = self.running_container.exec_run(command, stderr=False) |
There was a problem hiding this comment.
Removed stderr here, because mamba was updated recently and now the output is clean (as it was before)
|
|
||
| def get_health(container: Container, client: docker.DockerClient) -> str: |
There was a problem hiding this comment.
Once again, let's not use docker.models.containers unless we really have to
| @@ -19,22 +19,18 @@ class TrackedContainer: | |||
| Docker client instance | |||
| image_name: str | |||
| Name of the docker image to launch | |||
| **kwargs: dict, optional | |||
There was a problem hiding this comment.
We only construct TrackedContainer in one place, it doesn't make sense to allow this customization, and we simply add detach=True in run_detached
| @@ -3,4 +3,4 @@ | |||
| from tagging.utils.docker_runner import DockerRunner | |||
|
|
|||
| with DockerRunner("ubuntu") as container: | |||
| DockerRunner.exec_cmd(container, cmd="env", print_output=True) | |||
There was a problem hiding this comment.
Easier to always print output
Describe your changes
Issue ticket if applicable
Checklist (especially for first-time contributors)