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.

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

fixing doc for stream param in exec_run #3292

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

yasonk
Copy link
Contributor

@yasonk yasonk commented Sep 30, 2024

Looks like this doc update was missed.

When you run the code below, you get <class 'str'>. If you change detach to False, you get <class 'docker.types.daemon.CancellableStream'>. This means stream param is getting ignored and should be explicitly documented just like it is for the run command.

import docker
from dotenv import load_dotenv

load_dotenv()

client = docker.from_env()

container = client.containers.run(
    "python:3",
    command="/bin/bash",
    stdin_open=True,
    tty=True,
    detach=True,
    working_dir="/usr/src"
)

exit_code, output = container.exec_run(
    "ls -la /",
    stdout=True,
    stderr=True,
    tty=True,
    stream=True,
    detach=True,
)

print(type(output))

Comment on lines -184 to +185
stream (bool): Stream response data. Default: False
stream (bool): Stream response data. Ignored if ``detach`` is true.
Default: False
Copy link
Member

Choose a reason for hiding this comment

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

This change SGTM - we should probably look (maybe from an API perspective) to make this combination an error instead of silently ignore (I recall some recent conversation around a similar case with the Go SDK) /cc @laurazard

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM if CI is happy

@thaJeztah thaJeztah merged commit a8bac88 into docker:main Sep 30, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants