feluelle commented on a change in pull request #12096:
URL: https://github.com/apache/airflow/pull/12096#discussion_r530843994
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -260,6 +267,27 @@ def _wait_for_task_ended(self) -> None:
waiter.config.max_attempts = sys.maxsize # timeout is managed by
airflow
waiter.wait(cluster=self.cluster, tasks=[self.arn])
+ return
+
+ def _cloudwatch_log_events(self):
+ if not self.awslogs_group:
+ return None
+
+ if not self.awslogs_stream_prefix:
+ return None
Review comment:
Maybe we want to pull the "requirements" out of the method and do sth
like: if requirement than call function _else_ return None. WDYT?
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -268,11 +296,9 @@ def _check_success_task(self) -> None:
self.log.info('ECS Task stopped, check status: %s', response)
# Get logs from CloudWatch if the awslogs log driver was used
- if self.awslogs_group and self.awslogs_stream_prefix:
- self.log.info('ECS Task logs output:')
- task_id = self.arn.split("/")[-1]
- stream_name = f"{self.awslogs_stream_prefix}/{task_id}"
- for event in
self.get_logs_hook().get_log_events(self.awslogs_group, stream_name):
+ log_events = self._cloudwatch_log_events()
+ if log_events is not None:
+ for event in log_events:
Review comment:
Woudn't it make more sense to return an empty list so you don't have to
check if not none? So the loop will go over 0 items instead?
Or just do `log_events = self._cloudwatch_log_events() or []` (but I would
suggest to do this in the function)
WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
