[GitHub] [airflow] ashb commented on a diff in pull request #24726: 24681: Fixed issue where cli.logs was giving character array instead of string

[GitBox <gi...@apache.org>]

ashb commented on code in PR #24726:
URL: https://github.com/apache/airflow/pull/24726#discussion_r909605527


##########
airflow/providers/docker/operators/docker.py:
##########
@@ -317,18 +317,13 @@ def _run_image_with_mounts(
             if self.retrieve_output:
                 return self._attempt_to_retrieve_result()
             elif self.do_xcom_push:
-                log_parameters = {
-                    'container': self.container['Id'],
-                    'stdout': True,
-                    'stderr': True,
-                    'stream': True,
-                }
+                if len(log_lines) == 0:
+                    return None
                 try:
                     if self.xcom_all:
-                        return [stringify(line).strip() for line in self.cli.logs(**log_parameters)]
+                        return [stringify(line).strip() for line in log_lines]
                     else:
-                        lines = [stringify(line).strip() for line in self.cli.logs(**log_parameters, tail=1)]
-                        return lines[-1] if lines else None
+                        return stringify(log_lines[-1]).strip()

Review Comment:
   ```suggestion
                           return log_lines[-1]
   ```



##########
tests/providers/docker/operators/test_docker.py:
##########
@@ -471,7 +462,7 @@ def test_execute_xcom_behavior_bytes(self):
         self.client_mock.pull.return_value = [b'{"status":"pull log"}']
         self.client_mock.attach.return_value = iter([b'container log 1 ', b'container log 2'])
         # Make sure the logs side effect is updated after the change

Review Comment:
   This comment doesn't match the code anymore.



##########
airflow/providers/docker/operators/docker.py:
##########
@@ -317,18 +317,13 @@ def _run_image_with_mounts(
             if self.retrieve_output:
                 return self._attempt_to_retrieve_result()
             elif self.do_xcom_push:
-                log_parameters = {
-                    'container': self.container['Id'],
-                    'stdout': True,
-                    'stderr': True,
-                    'stream': True,
-                }
+                if len(log_lines) == 0:
+                    return None
                 try:
                     if self.xcom_all:
-                        return [stringify(line).strip() for line in self.cli.logs(**log_parameters)]
+                        return [stringify(line).strip() for line in log_lines]

Review Comment:
   stringify and strip is already done in `log_lines`
   ```suggestion
                           return log_lines
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org