[GitHub] [airflow] potiuk commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend

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

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


##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
 
         return conn_string
 
-    def get_connection(self, conn_id: str) -> Optional[Connection]:
+    def get_connection(self, conn_id: str) -> Optional["Connection"]:
         if not self.full_url_mode:
+            # Avoid circular import problems when instantiating the backend during configuration.
+            from airflow.models.connection import Connection

Review Comment:
   Yeah. We definitely import too much in some core packages. 
   
   And we should indeed add  secrets manager tests. But I think #24486 is not the solution to this problem. It is a good approach but we have a problem with internal architecture, not with imports. 
   
   The real problem is not thmodes limporting stuff, the problem is that we actually HAVE a circular dependency here and it's because our configuration approach is pretty badly intertwined with other parts of the system in this case we have:
   
   * configuration can read values by delegating to  secrets manager
   * secrets manager uses configuration to configure itself
   
   While I agree local imports and TYPE_CHECKING can provide some solution to the problem, they are very prone to trigger similar problems.
   
   
   I think we should finally do something about it - I think ti's a bad approach that two independent parts of systems are dependeing each other :). The dependency between those should be one way - either conf is using secrets backend or secrets backend using conf. Basically we have `secrets -> conf -> secret` dependency chain which makes it very easy to turn it into circular import problem. 
   
   Probably this is the best solution - is to  split out the part that secret REALLY need from conf and make it an "internal conf" (and then `conf -> secret -> "internal conf"`  will be the correct dependency  chain that will not lead to accidental triggering of such cricular imports.



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