Skip to content

Fix returning last log line in docker operator #13793

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

Closed
davidcaron wants to merge 2 commits into from
Closed

Fix returning last log line in docker operator #13793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce1d107
Fix returning last log line in docker operator
davidcaron Jan 20, 2021
36367bc
Fix DockerOperator.execute return type
davidcaron Jan 20, 2021
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
9 changes: 6 additions & 3 deletions airflow/providers/docker/operators/docker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ def get_hook(self) -> DockerHook:
tls=self.__get_tls_config(),
)

def _run_image(self) -> Optional[str]:
def _run_image(self) -> Optional[bytes]:
"""Run a Docker container with the provided image"""
self.log.info('Starting docker container from image %s', self.image)

Expand Down Expand Up @@ -269,14 +269,17 @@ def _run_image(self) -> Optional[str]:
# duplicated conditional logic because of expensive operation
ret = None
if self.do_xcom_push:
ret = self.cli.logs(container=self.container['Id']) if self.xcom_all else line.encode('utf-8')
if self.xcom_all:
ret = self.cli.logs(container=self.container['Id'])

@dstandish dstandish Feb 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not strip here also?

@davidcaron davidcaron Feb 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree it should probably be stripped, but my intention was to keep the current behavior when do_xcom_push and xcom_all are all true.

else:
ret = self.cli.logs(container=self.container['Id'], tail=1).strip()

if self.auto_remove:
self.cli.remove_container(self.container['Id'])

return ret

def execute(self, context) -> Optional[str]:
def execute(self, context) -> Optional[bytes]:

@dstandish dstandish Feb 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we perhaps decode utf-8 (assuming that's what comes out of logs, so that we return a string type? sincere question... 🤔

@dstandish dstandish Feb 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

coincidence someone is doing exactly that: https://github.com/apache/airflow/pull/13536/files

@davidcaron davidcaron Feb 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I think this would make more sense, but again I wanted to avoid a breaking change...

I would actually be happy with this other PR being merged, as it now includes the fix in my PR.

self.cli = self._get_cli()
if not self.cli:
raise Exception("The 'cli' should be initialized before!")
Expand Down
4 changes: 2 additions & 2 deletions tests/providers/docker/operators/test_docker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ def setUp(self):
self.client_mock = mock.Mock(spec=APIClient)
self.client_mock.create_container.return_value = {'Id': 'some_id'}
self.client_mock.images.return_value = []
self.client_mock.attach.return_value = ['container log']
self.client_mock.logs.return_value = ['container log']
self.client_mock.attach.return_value = [b'container log']
self.client_mock.logs.return_value = b'container log' # logs(..., stream=False) returns bytes
self.client_mock.pull.return_value = {"status": "pull log"}
self.client_mock.wait.return_value = {"StatusCode": 0}
self.client_mock.create_host_config.return_value = mock.Mock()
Expand Down