You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/11/22 22:30:41 UTC
[GitHub] [superset] hughhhh opened a new pull request, #22201: Ssh manager config
hughhhh opened a new pull request, #22201:
URL: https://github.com/apache/superset/pull/22201
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh merged pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
hughhhh merged PR #22201:
URL: https://github.com/apache/superset/pull/22201
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035420524
##########
superset/models/core.py:
##########
@@ -381,25 +383,19 @@ def get_sqla_engine_with_context(
ssh_tunnel.bind_host = url.host
ssh_tunnel.bind_port = url.port
ssh_params = ssh_tunnel.parameters()
- try:
- with sshtunnel.open_tunnel(**ssh_params) as server:
- yield self._get_sqla_engine(
- schema=schema,
- nullpool=nullpool,
- source=source,
- ssh_tunnel_server=server,
- )
- except Exception as ex:
- raise ex
+ if ssh_tunnel_manager and is_feature_enabled("SSH_TUNNELING"):
+ ssh_params = ssh_tunnel_manager.mutate(ssh_params)
- else:
- # do look up in table for using database_id
- try:
+ with sshtunnel.open_tunnel(**ssh_params) as server:
Review Comment:
This is a little confusing because it seems like you're still opening a tunnel even if the feature flag isn't enabled. I expect that it likely checks again for the feature flag downstream, but it doesn't read that way here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #22201: Ssh manager config
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22201:
URL: https://github.com/apache/superset/pull/22201#issuecomment-1324325577
# [Codecov](https://codecov.io/gh/apache/superset/pull/22201?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22201](https://codecov.io/gh/apache/superset/pull/22201?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2502f63) into [create-sshtunnelconfig-tbl](https://codecov.io/gh/apache/superset/commit/6c59663638e0293da4f243dcb8f13f5a071a79ef?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6c59663) will **decrease** coverage by `0.74%`.
> The diff coverage is `71.42%`.
> :exclamation: Current head 2502f63 differs from pull request most recent head 2522660. Consider uploading reports for the commit 2522660 to get more accurate results
```diff
@@ Coverage Diff @@
## create-sshtunnelconfig-tbl #22201 +/- ##
==============================================================
- Coverage 67.00% 66.25% -0.75%
==============================================================
Files 1841 1841
Lines 70087 70101 +14
Branches 7590 7590
==============================================================
- Hits 46959 46448 -511
- Misses 21162 21687 +525
Partials 1966 1966
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.58% <50.00%> (-0.01%)` | :arrow_down: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.48% <50.00%> (-0.01%)` | :arrow_down: |
| python | `79.79% <71.42%> (-1.56%)` | :arrow_down: |
| sqlite | `76.57% <50.00%> (-0.01%)` | :arrow_down: |
| unit | `51.06% <71.42%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `86.15% <33.33%> (-2.13%)` | :arrow_down: |
| [superset/databases/ssh\_tunnel/commands/create.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NzaF90dW5uZWwvY29tbWFuZHMvY3JlYXRlLnB5) | `89.28% <80.00%> (-2.02%)` | :arrow_down: |
| [superset/databases/ssh\_tunnel/commands/update.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NzaF90dW5uZWwvY29tbWFuZHMvdXBkYXRlLnB5) | `87.09% <80.00%> (-1.37%)` | :arrow_down: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.87% <100.00%> (+0.02%)` | :arrow_up: |
| [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `30.61% <0.00%> (-69.39%)` | :arrow_down: |
| [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
| [superset/datasets/commands/bulk\_delete.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvYnVsa19kZWxldGUucHk=) | `33.33% <0.00%> (-53.34%)` | :arrow_down: |
| [superset/datasets/columns/commands/delete.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29sdW1ucy9jb21tYW5kcy9kZWxldGUucHk=) | `44.11% <0.00%> (-52.95%)` | :arrow_down: |
| [superset/datasets/metrics/commands/delete.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvbWV0cmljcy9jb21tYW5kcy9kZWxldGUucHk=) | `44.11% <0.00%> (-52.95%)` | :arrow_down: |
| [superset/datasets/dao.py](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvZGFvLnB5) | `44.21% <0.00%> (-50.35%)` | :arrow_down: |
| ... and [30 more](https://codecov.io/gh/apache/superset/pull/22201/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] craig-rueda commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035166730
##########
superset/databases/ssh_tunnel/commands/create.py:
##########
@@ -20,16 +20,24 @@
from flask_appbuilder.models.sqla import Model
from marshmallow import ValidationError
+
+from superset import app, is_feature_enabled
from superset.commands.base import BaseCommand
from superset.dao.exceptions import DAOCreateFailedError
from superset.databases.dao import DatabaseDAO
from superset.databases.ssh_tunnel.commands.exceptions import SSHTunnelCreateFailedError
from superset.databases.ssh_tunnel.dao import SSHTunnelDAO
+config = app.config
+ssh_tunnel_manager = config["SSH_TUNNEL_MANAGER"]
Review Comment:
Put this inside the command's `init()` and leverage `current_app.config`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] craig-rueda commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035165682
##########
superset/config.py:
##########
@@ -471,8 +471,46 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
"DRILL_TO_DETAIL": False,
"DATAPANEL_CLOSED_BY_DEFAULT": False,
"HORIZONTAL_FILTER_BAR": False,
+ # Allow users to enable ssh tunneling when creating a DB.
+ # Users must check whether the DB engine supports SSH Tunnels
+ # otherwise enabling this flag won't have any effect on the DB.
+ "SSH_TUNNELING": False,
}
+# ------------------------------
+# SSH Tunnel
+# ------------------------------
+# Allow users to set the host used when connecting to the SSH Tunnel
+# as localhost and any other alias (0.0.0.0)
+# ----------------------------------------------------------------------
+# |
+# -------------+ | +----------+
+# LOCAL | | | REMOTE | :22 SSH
+# CLIENT | <== SSH ========> | SERVER | :8080 web service
+# -------------+ | +----------+
+# |
+# FIREWALL (only port 22 is open)
+
+# ----------------------------------------------------------------------
+# class SSHManager:
+# def validate(self, ssh_tunnel_params: Dict[str, Any]) -> None:
+# # validation on CREATE + UPDATE on SSHTunnel Model
+# # to block a request this function most raise an exception
+# raise NotImplemented()
+
+# def mutator(self, ssh_tunnel_params: Dict[str, Any]) -> Dict[str, Any]:
+# # override any ssh tunnel configuration object
+# raise NotImplemented()
+
+# @property
+# def local_bind_address(self):
+# # set the local binding address for the local client
+# # the port will be dynamically configured by the sshtunnel.SSHTunnelForwarder
+# # `server` return value
+# return "127.0.0.1"
+
+SSH_TUNNEL_MANAGER = None
Review Comment:
Should we provide a default impl here?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] craig-rueda commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035168343
##########
superset/config.py:
##########
@@ -471,8 +471,46 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
"DRILL_TO_DETAIL": False,
"DATAPANEL_CLOSED_BY_DEFAULT": False,
"HORIZONTAL_FILTER_BAR": False,
+ # Allow users to enable ssh tunneling when creating a DB.
+ # Users must check whether the DB engine supports SSH Tunnels
+ # otherwise enabling this flag won't have any effect on the DB.
+ "SSH_TUNNELING": False,
}
+# ------------------------------
+# SSH Tunnel
+# ------------------------------
+# Allow users to set the host used when connecting to the SSH Tunnel
+# as localhost and any other alias (0.0.0.0)
+# ----------------------------------------------------------------------
+# |
+# -------------+ | +----------+
+# LOCAL | | | REMOTE | :22 SSH
+# CLIENT | <== SSH ========> | SERVER | :8080 web service
+# -------------+ | +----------+
+# |
+# FIREWALL (only port 22 is open)
+
+# ----------------------------------------------------------------------
+# class SSHManager:
Review Comment:
The default manager should leverage the `app_init` pattern. So, inside the `AppInitializer`, the actual manager would be instantiated and then would have `init_app(app: Flask)` invoked on it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035419527
##########
superset/databases/ssh_tunnel/commands/create.py:
##########
@@ -45,6 +53,6 @@ def run(self) -> Model:
return tunnel
def validate(self) -> None:
- # TODO(hughhh): check to make sure the server port is not localhost
- # using the config.SSH_TUNNEL_MANAGER
- return
+ if is_feature_enabled("SSH_TUNNELING") and ssh_tunnel_manager:
+ ssh_tunnel_manager.validate(self._properties)
Review Comment:
is this the only validation that we want to do here?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] craig-rueda commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035169108
##########
superset/databases/ssh_tunnel/commands/update.py:
##########
@@ -33,6 +34,9 @@
logger = logging.getLogger(__name__)
+config = app.config
+ssh_tunnel_manager = config["SSH_TUNNEL_MANAGER"]
Review Comment:
Same comment as before. We should be getting the `SSHManager` from the `current_app`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035421585
##########
superset/models/core.py:
##########
@@ -381,25 +383,19 @@ def get_sqla_engine_with_context(
ssh_tunnel.bind_host = url.host
ssh_tunnel.bind_port = url.port
ssh_params = ssh_tunnel.parameters()
- try:
- with sshtunnel.open_tunnel(**ssh_params) as server:
- yield self._get_sqla_engine(
- schema=schema,
- nullpool=nullpool,
- source=source,
- ssh_tunnel_server=server,
- )
- except Exception as ex:
- raise ex
+ if ssh_tunnel_manager and is_feature_enabled("SSH_TUNNELING"):
+ ssh_params = ssh_tunnel_manager.mutate(ssh_params)
- else:
- # do look up in table for using database_id
- try:
+ with sshtunnel.open_tunnel(**ssh_params) as server:
Review Comment:
are there any known errors from the library that we need to catch here?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035418969
##########
superset/config.py:
##########
@@ -471,8 +471,46 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
"DRILL_TO_DETAIL": False,
"DATAPANEL_CLOSED_BY_DEFAULT": False,
"HORIZONTAL_FILTER_BAR": False,
+ # Allow users to enable ssh tunneling when creating a DB.
+ # Users must check whether the DB engine supports SSH Tunnels
+ # otherwise enabling this flag won't have any effect on the DB.
+ "SSH_TUNNELING": False,
}
+# ------------------------------
+# SSH Tunnel
+# ------------------------------
+# Allow users to set the host used when connecting to the SSH Tunnel
+# as localhost and any other alias (0.0.0.0)
+# ----------------------------------------------------------------------
+# |
+# -------------+ | +----------+
+# LOCAL | | | REMOTE | :22 SSH
+# CLIENT | <== SSH ========> | SERVER | :8080 web service
+# -------------+ | +----------+
+# |
+# FIREWALL (only port 22 is open)
+
+# ----------------------------------------------------------------------
+# class SSHManager:
+# def validate(self, ssh_tunnel_params: Dict[str, Any]) -> None:
+# # validation on CREATE + UPDATE on SSHTunnel Model
+# # to block a request this function most raise an exception
+# raise NotImplemented()
+
+# def mutator(self, ssh_tunnel_params: Dict[str, Any]) -> Dict[str, Any]:
+# # override any ssh tunnel configuration object
+# raise NotImplemented()
Review Comment:
do we need to make these two methods required? They seems more like nice to haves.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035419275
##########
superset/databases/ssh_tunnel/commands/create.py:
##########
@@ -20,16 +20,24 @@
from flask_appbuilder.models.sqla import Model
from marshmallow import ValidationError
+
+from superset import app, is_feature_enabled
from superset.commands.base import BaseCommand
from superset.dao.exceptions import DAOCreateFailedError
from superset.databases.dao import DatabaseDAO
from superset.databases.ssh_tunnel.commands.exceptions import SSHTunnelCreateFailedError
from superset.databases.ssh_tunnel.dao import SSHTunnelDAO
+config = app.config
+ssh_tunnel_manager = config["SSH_TUNNEL_MANAGER"]
+
logger = logging.getLogger(__name__)
class CreateSSHTunnelCommand(BaseCommand):
+ def __str__(self) -> str:
+ return super().__str__()
Review Comment:
isn't this redundant?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] craig-rueda commented on a diff in pull request #22201: feat(ssh-tunnel): ssh manager config + feature flag
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on code in PR #22201:
URL: https://github.com/apache/superset/pull/22201#discussion_r1035166277
##########
superset/databases/ssh_tunnel/commands/create.py:
##########
@@ -20,16 +20,24 @@
from flask_appbuilder.models.sqla import Model
from marshmallow import ValidationError
+
+from superset import app, is_feature_enabled
Review Comment:
Don't directly import app here. Instead, use `flask.current_app`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org