[GitHub] [airflow] potiuk commented on a change in pull request #7923: Get Airflow Variables from Environment Variables

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

potiuk commented on a change in pull request #7923: Get Airflow Variables from Environment Variables
URL: https://github.com/apache/airflow/pull/7923#discussion_r399640504
 
 

 ##########
 File path: airflow/secrets/metastore.py
 ##########
 @@ -37,3 +37,16 @@ def get_connections(self, conn_id, session=None) -> List[Connection]:
         conn_list = session.query(Connection).filter(Connection.conn_id == conn_id).all()
         session.expunge_all()
         return conn_list
+
+    @provide_session
+    def get_variable(self, key: str, session=None):
+        """
+        Get Airflow Variable from Metadata DB
+
+        :param key: Variable Key
+        :return: Variable Value
+        """
+        from airflow.models.variable import Variable
 
 Review comment:
   My opinion again:
   
   That's true that there are no cyclical imports detected by the parser, But still, logically there are two separate entities in different modules that depend on each other in both directions. For me, this is a classic example of cyclic dependency which should be avoided (maybe not at all cost, but whenever it's possible).
   
   I strongly believe the code is much cleaner and easier to understand when we avoid cyclical dependencies. Usually, cyclical dependency (not import) is a sign that we should improve the code structure (either split responsibilities better or move the classes/objects to a single file if there are strongly coupled). We've already introduced a number of such refactorings and they let us reason about the code better and (for example) introduce those performance improvements for the number of DB queries - because it was much easier to understand what happened.
   
   But that's just opinion (though I believe in ti quite strongly).

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services