Re: [PR] Added logging device and logging device options [airflow]

["josix (via GitHub)" <gi...@apache.org>]

josix commented on code in PR #41416:
URL: https://github.com/apache/airflow/pull/41416#discussion_r1715012521


##########
airflow/providers/docker/operators/docker_swarm.py:
##########
@@ -128,12 +140,25 @@ def __init__(
         self.networks = networks
         self.placement = placement
         self.container_resources = container_resources or types.Resources(mem_limit=self.mem_limit)
+        self.logging_driver = logging_driver
+        self.logging_driver_opts = logging_driver_opts
 
     def execute(self, context: Context) -> None:
         self.environment["AIRFLOW_TMP_DIR"] = self.tmp_dir
         return self._run_service()
 
     def _run_service(self) -> None:
+        if self.logging_driver:
+            logging_driver = self.logging_driver.lower()
+            supported_logging_drivers = ["json-file", "gelf"]
+            if logging_driver not in supported_logging_drivers:
+                raise AirflowException(
+                    f"Unsupported logging driver provided: {logging_driver}. Must be one of: [{', '.join(supported_logging_drivers)}]"
+                )
+            log_driver = types.DriverConfig(logging_driver, self.logging_driver_opts)

Review Comment:
   question (non-blocking): how about moving the validation into initialization or a separate method instead of run_service? That would be helpful for avoiding mixing purposes (like setting the correct log_driver and create the service) in `_run_service`. I think It would also raise the question of whether we expect the exception to be raised at execution time or at the DAG definition phase (CMIIW).



-- 
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