[GitHub] [airflow] potiuk commented on a diff in pull request #27366: Fix coertion for VERBOSE and DRY_RUN env variables

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

potiuk commented on code in PR #27366:
URL: https://github.com/apache/airflow/pull/27366#discussion_r1008732466


##########
dev/breeze/src/airflow_breeze/utils/shared_options.py:
##########
@@ -19,7 +19,15 @@
 
 import os
 
-__verbose_value: bool = os.environ.get("VERBOSE", "false")[0].lower() == "t"
+
+def __get_default_bool_value(env_var: str) -> bool:
+    string_val = os.environ.get(env_var, "")
+    if not string_val:  # handle "" and other false-y coerce-able values
+        return False
+    return string_val[0].lower() in ["t", "y"]  # handle all kinds of truth-y/yes-y/false-y/non-sy strings

Review Comment:
   Very good point. 
   
   Generally I want to avoid the "airflow" fallacy where there are some intertwined imports between utils that are ending up often in circular references and local imports (hello `config` and `secrets`) so you have to be rather careful with such coupling of independent modules. Here the potential difficult lies with, the Custom Params and shared_options import sequence in various scenarios. 
   
   This would work in this case but I can imagine this migh be a problem - even accidentally - in the future so I think even better soluion is to extract coerce method to a separate module and use it in both places. I am rather aware of cyclomatic complexity https://en.wikipedia.org/wiki/Cyclomatic_complexity and while the ship has sailed for Airflow (or maybe we will be able to fix it over time who knows I tried to fix cyclomtic complexity for config few times and failed miserably) but I try to keep it low for breeze (even without measuring it). 
   
   Fix is coming.
   



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