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:
us...@infra.apache.org
With regards,
Apache Git Services
