feat(ssh_tunnel): Create command & exceptions #22148

hughhhh · 2022-11-17T04:21:37Z

SUMMARY

Create a command to allow users to generate a SSHTunnel object to be saved once a Database is being commited to the metastore.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

…create-command

…e/superset into ssh-create-command

Antonio-RiveroMartnez · 2022-11-17T20:16:25Z

superset/databases/commands/create.py

@@ -71,6 +71,10 @@ def run(self) -> Model:
            database = DatabaseDAO.create(self._properties, commit=False)
            database.set_sqlalchemy_uri(database.sqlalchemy_uri)

+            # create ssh tunnel


Should we remove this comment block?

Antonio-RiveroMartnez · 2022-11-17T20:17:07Z

superset/databases/commands/exceptions.py

@@ -176,3 +176,8 @@ class DatabaseOfflineError(SupersetErrorException):

 class InvalidParametersError(SupersetErrorsException):
    status = 422
+
+
+class SSHTunnelCreateFailedError(CommandException):


This class should be in databases/ssh_tunnel/commands/exceptions.py like the others.

Antonio-RiveroMartnez · 2022-11-17T20:19:06Z

superset/databases/ssh_tunnel/commands/create.py

+class CreateSSHTunnelCommand(BaseCommand):
+    def __init__(self, database_id: int, data: Dict[str, Any]):
+        self._properties = data.copy()
+        self._properties["database_id"] = database_id


Why not pulling it from the data dict instead of having database_id as a separated arg? I mean, even in the create_test.py (dao and command) you have the database_id as part of the properties of the tunnel.

i removed it, i think the pattern is better since the database_id is the first class citizen

Antonio-RiveroMartnez · 2022-11-17T20:20:43Z

tests/unit_tests/databases/dao/dao_tests.py

@@ -50,7 +53,7 @@ def session_with_data(session: Session) -> Iterator[Session]:
    session.rollback()


-def test_database_get_shh_tunnel(session_with_data: Session) -> None:
+def test_database_get_ssh_tunnel(session_with_data: Session) -> None:


Wondering what "changed" here because I see no difference 🤔 👀

test_database_get_shh_tunnel to test_database_get_ssh_tunnel

we misspelt ssh lol

oh boy! yeah! hahaha trust me I had to read this like 10+ times to realize the mistake. kk thanks!

…e/superset into ssh-create-command

Antonio-RiveroMartnez · 2022-11-18T20:08:07Z

tests/unit_tests/databases/ssh_tunnel/dao_test.py

+        "password": "bar",
+    }
+
+    result = SSHTunnelDAO.create(properties, commit=True)


where is it getting the database_id from?

codecov · 2022-11-18T21:35:46Z

Codecov Report

Merging #22148 (ab8cf7b) into create-sshtunnelconfig-tbl (92e41f1) will decrease coverage by 0.71%.
The diff coverage is 92.00%.

@@                      Coverage Diff                       @@
##           create-sshtunnelconfig-tbl   #22148      +/-   ##
==============================================================
- Coverage                       66.97%   66.25%   -0.72%     
==============================================================
  Files                            1840     1839       -1     
  Lines                           70062    70049      -13     
  Branches                         7590     7571      -19     
==============================================================
- Hits                            46922    46413     -509     
- Misses                          21174    21677     +503     
+ Partials                         1966     1959       -7

Flag	Coverage Δ
hive	`52.59% <ø> (ø)`
postgres	`?`
presto	`52.48% <ø> (ø)`
python	`79.79% <92.00%> (-1.51%)`	⬇️
sqlite	`76.57% <ø> (ø)`
unit	`51.05% <92.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/databases/ssh_tunnel/commands/create.py	`91.30% <91.30%> (ø)`
...perset/databases/ssh_tunnel/commands/exceptions.py	`100.00% <100.00%> (ø)`
superset/datasets/commands/create.py	`30.61% <0.00%> (-69.39%)`	⬇️
superset/datasets/commands/update.py	`25.00% <0.00%> (-69.05%)`	⬇️
superset/datasets/commands/bulk_delete.py	`33.33% <0.00%> (-53.34%)`	⬇️
superset/datasets/columns/commands/delete.py	`44.11% <0.00%> (-52.95%)`	⬇️
superset/datasets/metrics/commands/delete.py	`44.11% <0.00%> (-52.95%)`	⬇️
superset/datasets/dao.py	`44.21% <0.00%> (-50.35%)`	⬇️
superset/sql_validators/postgres.py	`50.00% <0.00%> (-50.00%)`	⬇️
superset/datasets/commands/delete.py	`48.57% <0.00%> (-48.58%)`	⬇️
... and 42 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

eschutho · 2022-11-18T22:59:43Z

superset/databases/ssh_tunnel/commands/create.py

+        return tunnel
+
+    def validate(self) -> None:
+        # check to make sure the server port is not localhost


can we fill in the validation for this?

@eschutho going to implement an SSH_MANAGER class to handle this, was talking to @craig-rueda yesterday about the approach and we basically allow users define how the want manage this one their end.

Ok, do we need this validation method then or can we remove it?

…e/superset into ssh-create-command

Antonio-RiveroMartnez

LGTM!

…e/superset into ssh-create-command

hughhhh · 2022-11-22T18:42:15Z

merging to unblock following PRs

@eschutho let me know if there are any concerns with this approach

hughhhh added 3 commits November 16, 2022 14:15

save

1fb44dd

Merge branch 'master' of https://github.com/apache/superset into ssh-…

c85b361

…create-command

change name

1b4e0b3

pull-request-size bot added the size/XL label Nov 17, 2022

hughhhh mentioned this pull request Nov 17, 2022

feat(ssh-tunnelling): Setup SSH Tunneling Commands for Database Connections #21912

Merged

15 tasks

hughhhh changed the title ~~feat: add create command for ssh tunnels~~ feat(ssh_tunnel): Create command & exceptions Nov 17, 2022

hughhhh changed the base branch from master to create-sshtunnelconfig-tbl November 17, 2022 19:06

pull-request-size bot added size/L and removed size/XL labels Nov 17, 2022

hughhhh added 2 commits November 17, 2022 14:07

Merge branch 'create-sshtunnelconfig-tbl' of https://github.com/apach…

5d22a7d

…e/superset into ssh-create-command

update test

fc40a3f

Antonio-RiveroMartnez reviewed Nov 17, 2022

View reviewed changes

hughhhh added 2 commits November 18, 2022 14:42

remove comments

6d073bc

update test

a17333a

hughhhh marked this pull request as ready for review November 18, 2022 19:46

hughhhh added 3 commits November 18, 2022 14:48

Merge branch 'create-sshtunnelconfig-tbl' of https://github.com/apach…

a7ebfbb

…e/superset into ssh-create-command

moved the exceptions

a2b87a6

oops

a36b727

Antonio-RiveroMartnez reviewed Nov 18, 2022

View reviewed changes

dao test

7cbe40e

eschutho reviewed Nov 18, 2022

View reviewed changes

hughhhh added 6 commits November 21, 2022 12:59

add session

ebadf87

fix session

b96af92

Merge branch 'create-sshtunnelconfig-tbl' of https://github.com/apach…

64360df

…e/superset into ssh-create-command

fix testing

bcccfa1

update test

e870cb5

remove commit

ed75a4a

hughhhh force-pushed the ssh-create-command branch 2 times, most recently from 486ac2d to e5de217 Compare November 22, 2022 02:27

hughhhh force-pushed the ssh-create-command branch from e5de217 to 6e32b57 Compare November 22, 2022 02:41

fix test

2b51126

hughhhh force-pushed the ssh-create-command branch from 6e32b57 to 2b51126 Compare November 22, 2022 02:48

add validate spike

b0f2292

hughhhh requested review from eschutho and Antonio-RiveroMartnez November 22, 2022 16:32

Antonio-RiveroMartnez approved these changes Nov 22, 2022

View reviewed changes

hughhhh force-pushed the ssh-create-command branch from c00b151 to 1fbf7a0 Compare November 22, 2022 17:05

remove unneeded fixtures

630b0ad

hughhhh force-pushed the ssh-create-command branch from 1fbf7a0 to 630b0ad Compare November 22, 2022 17:12

Merge branch 'create-sshtunnelconfig-tbl' of https://github.com/apach…

ab8cf7b

…e/superset into ssh-create-command

hughhhh merged commit 6c59663 into create-sshtunnelconfig-tbl Nov 22, 2022

mistercrunch deleted the ssh-create-command branch March 26, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ssh_tunnel): Create command & exceptions #22148

feat(ssh_tunnel): Create command & exceptions #22148

hughhhh commented Nov 17, 2022 •

edited

Loading

Antonio-RiveroMartnez Nov 17, 2022

Antonio-RiveroMartnez Nov 17, 2022

Antonio-RiveroMartnez Nov 17, 2022

hughhhh Nov 18, 2022

Antonio-RiveroMartnez Nov 17, 2022

hughhhh Nov 17, 2022 •

edited

Loading

Antonio-RiveroMartnez Nov 17, 2022

Antonio-RiveroMartnez Nov 18, 2022

codecov bot commented Nov 18, 2022 •

edited

Loading

eschutho Nov 18, 2022

hughhhh Nov 22, 2022

eschutho Nov 22, 2022

Antonio-RiveroMartnez left a comment

hughhhh commented Nov 22, 2022

feat(ssh_tunnel): Create command & exceptions #22148

feat(ssh_tunnel): Create command & exceptions #22148

Conversation

hughhhh commented Nov 17, 2022 • edited Loading

SUMMARY

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughhhh Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

hughhhh commented Nov 22, 2022

hughhhh commented Nov 17, 2022 •

edited

Loading

hughhhh Nov 17, 2022 •

edited

Loading

codecov bot commented Nov 18, 2022 •

edited

Loading