fix: SSH Tunnel creation with dynamic form #24196

hughhhh · 2023-05-23T21:17:39Z

SUMMARY

Fixing issue with SSH tunnel creation with dynamic forms. Main issue was the ssh toggle would lose state, and validate_parameters endpoints would fail immediately since no tunnel is created on every onBlur event

To fix it I refactored the toggle state to the reducer, and stop validation on any ssh tunnel requests to connect to a DB

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

codecov · 2023-05-23T21:27:30Z

Codecov Report

Merging #24196 (0bb9c6f) into master (d5f88c1) will increase coverage by 0.04%.
The diff coverage is 71.68%.

❗ Current head 0bb9c6f differs from pull request most recent head 5fb76e7. Consider uploading reports for the commit 5fb76e7 to get more accurate results

@@            Coverage Diff             @@
##           master   #24196      +/-   ##
==========================================
+ Coverage   69.03%   69.07%   +0.04%     
==========================================
  Files        1901     1906       +5     
  Lines       74002    74123     +121     
  Branches     8116     8158      +42     
==========================================
+ Hits        51086    51204     +118     
+ Misses      20805    20797       -8     
- Partials     2111     2122      +11

Flag	Coverage Δ
hive	`53.92% <67.20%> (-0.01%)`	⬇️
javascript	`55.78% <61.53%> (+0.13%)`	⬆️
mysql	`79.40% <88.00%> (-0.03%)`	⬇️
postgres	`79.48% <88.00%> (-0.03%)`	⬇️
presto	`53.82% <64.80%> (-0.02%)`	⬇️
python	`83.48% <92.80%> (-0.01%)`	⬇️
sqlite	`78.04% <83.20%> (+0.03%)`	⬆️
unit	`54.69% <68.80%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...ackages/superset-ui-chart-controls/src/fixtures.ts	`100.00% <ø> (ø)`
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
.../packages/superset-ui-core/src/chart/types/Base.ts	`100.00% <ø> (ø)`
...ges/superset-ui-core/src/query/types/Datasource.ts	`100.00% <ø> (ø)`
...ackages/superset-ui-core/src/query/types/Metric.ts	`100.00% <ø> (ø)`
...rts/src/BigNumber/BigNumberTotal/transformProps.ts	`0.00% <0.00%> (ø)`
.../plugins/plugin-chart-echarts/src/BoxPlot/index.ts	`50.00% <ø> (ø)`
...d/plugins/plugin-chart-echarts/src/Funnel/index.ts	`50.00% <ø> (ø)`
...nd/plugins/plugin-chart-echarts/src/Gauge/index.ts	`50.00% <ø> (ø)`
...nd/plugins/plugin-chart-echarts/src/Graph/index.ts	`50.00% <ø> (ø)`
... and 122 more

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

Antonio-RiveroMartnez · 2023-06-13T17:07:36Z

superset-frontend/src/features/databases/DatabaseModal/index.tsx

-      const errors = await getValidation(dbToUpdate, true);
-      if ((validationErrors && !isEmpty(validationErrors)) || errors) {
+      // only do validation for non ssh tunnel connections
+      console.log(dbToUpdate);


…sh-dynamic

This reverts commit 75f6ddf.

betodealmeida · 2023-06-22T18:42:26Z

superset-frontend/src/features/databases/DatabaseModal/index.tsx

-      const errors = await getValidation(dbToUpdate, true);
-      if ((validationErrors && !isEmpty(validationErrors)) || errors) {
+      // only do validation for non ssh tunnel connections
+      if (!dbToUpdate?.ssh_tunnel) {


@hughhhh why can't we run the validation for SSH tunnel connections, assuming the user has provided that info?

I think the logic should be: if the user has enabled SSH, we don't perform any validation until SSH credentials are entered.

for the initial implementation, we were okay with doing validation on just connect. Right now users are not able to connect at all via dynamic forms.

I'm open to this fix but that's going to take longer to implement vs. fixing the issue at hand.

Let me know what you think

betodealmeida · 2023-06-22T19:03:03Z

...et-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/CommonParameters.tsx

+  changeMethods,
+  db,
+}: FieldPropTypes) =>
+  true ? (


Why the ternary operator here if the condition is always true?

betodealmeida · 2023-06-22T19:10:23Z

superset-frontend/src/views/CRUD/hooks.ts

@@ -753,6 +753,11 @@ export function useDatabaseValidation() {
        .catch(e => {
          if (typeof e.json === 'function') {
            return e.json().then(({ errors = [] }: JsonObject) => {
+              if (database?.parameters?.ssh) {


I don't think this belongs here... if we don't want to show validation errors for some reason, then we shouldn't be running the validation API calls.

betodealmeida

Awesome!

(cherry picked from commit 226c7f8)

sadpandajoe · 2023-07-05T18:37:44Z

🏷️ preset:2023.27

(cherry picked from commit 226c7f8)

hughhhh added 2 commits May 23, 2023 01:29

save for now

97b436b

this works

1afb7cd

pull-request-size bot added the size/M label May 23, 2023

hughhhh added 6 commits May 24, 2023 19:05

w

79af985

ok

eb7683a

fix test

9b99dd2

fix test

eff48f9

add ssh

b4bab39

update test

c1ccff2

pull-request-size bot added size/XXL and removed size/M labels May 30, 2023

hughhhh force-pushed the ssh-dynamic branch from e558892 to c1ccff2 Compare June 13, 2023 16:51

pull-request-size bot added size/M and removed size/XXL labels Jun 13, 2023

Antonio-RiveroMartnez reviewed Jun 13, 2023

View reviewed changes

hughhhh added 2 commits June 21, 2023 01:37

Merge branch 'master' of https://github.com/preset-io/superset into s…

7b01352

…sh-dynamic

Merge branch 'master' of https://github.com/preset-io/superset into s…

97cddfe

…sh-dynamic

hughhhh force-pushed the ssh-dynamic branch 3 times, most recently from a27ed0a to fdde2f8 Compare June 21, 2023 16:08

remove console log

f4fb07b

hughhhh force-pushed the ssh-dynamic branch from fdde2f8 to f4fb07b Compare June 21, 2023 16:34

fix cypress

3fc9269

hughhhh requested review from jinghua-qa, geido, eschutho, rusackas and betodealmeida as code owners June 21, 2023 19:24

pull-request-size bot removed the size/M label Jun 21, 2023

pull-request-size bot added the size/L label Jun 21, 2023

hughhhh added 3 commits June 22, 2023 00:41

cleanup

75f6ddf

Revert "cleanup"

66f6024

This reverts commit 75f6ddf.

update types for now

9e3760a

betodealmeida requested changes Jun 22, 2023

View reviewed changes

hughhhh added 2 commits June 28, 2023 22:34

remove unneeded code

5e843f9

dnt make network calls with ssh enabled

5fb76e7

betodealmeida approved these changes Jun 29, 2023

View reviewed changes

hughhhh merged commit 226c7f8 into apache:master Jul 3, 2023

hughhhh deleted the ssh-dynamic branch July 3, 2023 03:48

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 5, 2023

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Jul 5, 2023

fix: SSH Tunnel creation with dynamic form (apache#24196)

747c2bd

(cherry picked from commit 226c7f8)

michael-s-molina pushed a commit that referenced this pull request Jul 26, 2023

fix: SSH Tunnel creation with dynamic form (#24196)

1b41653

(cherry picked from commit 226c7f8)

eschutho added v2.1 2.1.2 labels Oct 20, 2023

eschutho pushed a commit that referenced this pull request Oct 20, 2023

fix: SSH Tunnel creation with dynamic form (#24196)

2a49cda

(cherry picked from commit 226c7f8)

eschutho pushed a commit that referenced this pull request Oct 20, 2023

fix: SSH Tunnel creation with dynamic form (#24196)

2867f05

(cherry picked from commit 226c7f8)

eschutho pushed a commit that referenced this pull request Oct 20, 2023

fix: SSH Tunnel creation with dynamic form (#24196)

f3bb63b

(cherry picked from commit 226c7f8)

mistercrunch added 🍒 2.1.2 🍒 2.1.3 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix: SSH Tunnel creation with dynamic form (apache#24196)

46d86a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: SSH Tunnel creation with dynamic form #24196

fix: SSH Tunnel creation with dynamic form #24196

hughhhh commented May 23, 2023 •

edited

Loading

codecov bot commented May 23, 2023 •

edited

Loading

Antonio-RiveroMartnez Jun 13, 2023

betodealmeida Jun 22, 2023

hughhhh Jun 22, 2023

betodealmeida Jun 22, 2023

betodealmeida Jun 22, 2023

betodealmeida left a comment

sadpandajoe commented Jul 5, 2023

fix: SSH Tunnel creation with dynamic form #24196

fix: SSH Tunnel creation with dynamic form #24196

Conversation

hughhhh commented May 23, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented May 23, 2023 • edited Loading

Codecov Report

Antonio-RiveroMartnez Jun 13, 2023

Choose a reason for hiding this comment

betodealmeida Jun 22, 2023

Choose a reason for hiding this comment

hughhhh Jun 22, 2023

Choose a reason for hiding this comment

betodealmeida Jun 22, 2023

Choose a reason for hiding this comment

betodealmeida Jun 22, 2023

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

sadpandajoe commented Jul 5, 2023

hughhhh commented May 23, 2023 •

edited

Loading

codecov bot commented May 23, 2023 •

edited

Loading