SameerMesiah97 commented on code in PR #74171:
URL: https://github.com/apache/airflow/pull/74171#discussion_r4174635132
##########
providers/amazon/tests/unit/amazon/aws/operators/test_ecs.py:
##########
@@ -191,6 +191,20 @@ def
test_get_task_log_fetcher_uses_region_name_when_awslogs_region_not_set(self)
assert fetcher.hook.region_name == "region"
+ def test_get_task_log_fetcher_uses_operator_aws_configuration(self):
+ self.set_up_operator(
+ awslogs_group="awslogs-group",
+ awslogs_stream_prefix="prefix",
+ region_name="region",
+ verify="/path/to/ca-bundle.pem",
+ botocore_config={"read_timeout": 10},
+ )
+
+ fetcher = self.ecs._get_task_log_fetcher()
+
+ assert fetcher.hook._verify == "/path/to/ca-bundle.pem"
+ assert fetcher.hook._config.read_timeout == 10
Review Comment:
The above comment applies here too. You are asserting against hook internals
rather than at the interface layer.
##########
providers/amazon/tests/unit/amazon/aws/operators/test_batch.py:
##########
@@ -92,6 +92,28 @@ def setup_method(self, _, get_client_type_mock):
self.mock_context = mock.MagicMock()
+ @patch.object(BatchClientHook, "get_job_awslogs_info")
+ def test_get_batch_log_fetcher_uses_operator_aws_configuration(self,
mock_get_job_awslogs_info):
+ mock_get_job_awslogs_info.return_value = {
+ "awslogs_region": "us-east-1",
+ "awslogs_group": "/aws/batch/job",
+ "awslogs_stream_name": "stream1",
+ }
+ batch = BatchOperator(
+ task_id="test_log_fetcher_configuration",
+ job_name=JOB_NAME,
+ job_queue="queue",
+ job_definition="hello-world",
+ awslogs_enabled=True,
+ verify="/path/to/ca-bundle.pem",
+ botocore_config={"read_timeout": 10},
+ )
+
+ fetcher = batch._get_batch_log_fetcher(JOB_ID)
+
+ assert fetcher.hook._verify == "/path/to/ca-bundle.pem"
+ assert fetcher.hook._config.read_timeout == 10
Review Comment:
Could we mock `AwsTaskLogFetcher` here and assert that `verify` and
`botocore_config` are passed to its constructor? That would test the forwarding
added by this change without depending on the hook’s private `_verify` and
`_config` attributes.
##########
providers/amazon/tests/unit/amazon/aws/utils/test_task_log_fetcher.py:
##########
@@ -44,6 +44,21 @@ def set_up_log_fetcher(self, logger_mock):
def setup_method(self):
self.set_up_log_fetcher()
+ def test_hook_is_built_with_the_given_aws_configuration(self):
+ log_fetcher = AwsTaskLogFetcher(
+ log_group="test_log_group",
+ log_stream_name="test_log_stream_name",
+ fetch_interval=timedelta(milliseconds=1),
+ logger=self.logger_mock,
+ region_name="eu-west-3",
+ verify="/path/to/ca-bundle.pem",
+ botocore_config={"read_timeout": 10},
+ )
+
+ assert log_fetcher.hook.region_name == "eu-west-3"
+ assert log_fetcher.hook._verify == "/path/to/ca-bundle.pem"
+ assert log_fetcher.hook._config.read_timeout == 10
Review Comment:
Same here. Except I think we should mock the hook.
##########
providers/amazon/docs/changelog.rst:
##########
@@ -26,6 +26,14 @@
Changelog
---------
+.. warning::
+ ``EcsRunTaskOperator`` and ``BatchOperator`` now pass ``verify`` and
``botocore_config`` to the
+ CloudWatch client that streams task logs, which previously used boto3
defaults. A deployment that
+ sets an aggressive ``botocore_config`` timeout for the task client will now
apply it to log
+ streaming as well, and one that points ``verify`` at a private CA bundle
will now use it there
+ too. ``EcsRunTaskOperator`` already honoured both settings when reading the
final log line, so
+ this makes the two log paths agree.
Review Comment:
It is debatable whether we need a changelog entry here. I am leaning no but
lets wait for a maintainer to weigh in.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
