betodealmeida commented on code in PR #21943:
URL: https://github.com/apache/superset/pull/21943#discussion_r1015736288
##########
superset/db_engine_specs/base.py:
##########
@@ -472,7 +472,10 @@ def get_engine(
schema: Optional[str] = None,
source: Optional[utils.QuerySource] = None,
) -> Engine:
- return database.get_sqla_engine(schema=schema, source=source)
+ with database.get_sqla_engine_with_context(
+ schema=schema, source=source
+ ) as engine:
+ return engine
Review Comment:
> so should i just `return database._ get_sqla_engine()`
If you do that, then the SSH tunneling wouldn't be setup in places that call
`DBEngineSpec.get_engine()`, defeating the purpose of the changes in this PR.
You need to convert this into a context manager as well (and update everything
downstream), so that the setup/teardown works as expected.
You can play with a simple Python script to understand how this works:
```python
from contextlib import contextmanager
@contextmanager
def get_sqla_engine_with_context():
print("enabling ssh tunnel")
yield 42
print("disabling ssh tunnel")
def get_engine():
with get_sqla_engine_with_context() as engine:
return engine
print("start")
engine = get_engine()
print("I have the engine, I can now work on it")
print("end")
```
Running the code above prints:
```before
start
enabling ssh tunnel
disabling ssh tunnel
I have the engine, I can now work on it
end
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
