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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
