You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org> on 2023/02/02 19:55:23 UTC
[GitHub] [superset] Antonio-RiveroMartnez opened a new pull request, #22967: feat(ssh_tunnel): SSH Tunnel Switch extension
Antonio-RiveroMartnez opened a new pull request, #22967:
URL: https://github.com/apache/superset/pull/22967
### SUMMARY
This PR doesn't change the behavior of our UI. Originally, we had the switch and the rest of the form fields as part of the SSHTunnelForm, however, that stops us from extending the behavior of the switch itself if needed. With these changes we introduce a new registry extension and make use of it if provided. Also we add a new test to cover such scenario.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
### TESTING INSTRUCTIONS
1. Implement a new extension and place it in the new registry
2. Provide such extension to the frontend
3. Your extension of the switch should be displayed and not the default one
### 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
- [X] 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] codecov[bot] commented on pull request #22967: feat(ssh_tunnel): SSH Tunnel Switch extension
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #22967:
URL: https://github.com/apache/superset/pull/22967#issuecomment-1414360287
# [Codecov](https://codecov.io/gh/apache/superset/pull/22967?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 [#22967](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6b8d915) into [master](https://codecov.io/gh/apache/superset/commit/c53c3aa23d9c42e6856617c1db7ea3545c36bade?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c53c3aa) will **increase** coverage by `0.04%`.
> The diff coverage is `72.72%`.
> :exclamation: Current head 6b8d915 differs from pull request most recent head a8a0f0d. Consider uploading reports for the commit a8a0f0d to get more accurate results
```diff
@@ Coverage Diff @@
## master #22967 +/- ##
==========================================
+ Coverage 67.36% 67.41% +0.04%
==========================================
Files 1876 1876
Lines 72087 72096 +9
Branches 7860 7863 +3
==========================================
+ Hits 48564 48602 +38
+ Misses 21509 21479 -30
- Partials 2014 2015 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `53.77% <68.75%> (+<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/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...-ui-chart-controls/src/operators/renameOperator.ts](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL29wZXJhdG9ycy9yZW5hbWVPcGVyYXRvci50cw==) | `100.00% <ø> (ø)` | |
| [...hart-controls/src/operators/timeCompareOperator.ts](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL29wZXJhdG9ycy90aW1lQ29tcGFyZU9wZXJhdG9yLnRz) | `100.00% <ø> (ø)` | |
| [...-chart-controls/src/sections/advancedAnalytics.tsx](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NlY3Rpb25zL2FkdmFuY2VkQW5hbHl0aWNzLnRzeA==) | `14.28% <ø> (ø)` | |
| [...trols/components/ColumnConfigControl/constants.tsx](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9jb21wb25lbnRzL0NvbHVtbkNvbmZpZ0NvbnRyb2wvY29uc3RhbnRzLnRzeA==) | `100.00% <ø> (ø)` | |
| [...d/packages/superset-ui-chart-controls/src/types.ts](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [...i-chart-controls/src/utils/expandControlConfig.tsx](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3V0aWxzL2V4cGFuZENvbnRyb2xDb25maWcudHN4) | `100.00% <ø> (ø)` | |
| [...perset-ui-core/src/chart/components/SuperChart.tsx](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY2hhcnQvY29tcG9uZW50cy9TdXBlckNoYXJ0LnRzeA==) | `97.22% <ø> (ø)` | |
| [...ages/superset-ui-core/src/math-expression/index.ts](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvbWF0aC1leHByZXNzaW9uL2luZGV4LnRz) | `100.00% <ø> (ø)` | |
| [...ges/superset-ui-core/src/query/buildQueryObject.ts](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvYnVpbGRRdWVyeU9iamVjdC50cw==) | `100.00% <ø> (ø)` | |
| [...ges/superset-ui-core/src/query/normalizeOrderBy.ts](https://codecov.io/gh/apache/superset/pull/22967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvbm9ybWFsaXplT3JkZXJCeS50cw==) | `100.00% <ø> (ø)` | |
| ... and [73 more](https://codecov.io/gh/apache/superset/pull/22967?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] Antonio-RiveroMartnez commented on a diff in pull request #22967: feat(ssh_tunnel): SSH Tunnel Switch extension
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #22967:
URL: https://github.com/apache/superset/pull/22967#discussion_r1095247724
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1346,14 +1357,33 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
payload: { login_method: method },
})
}
- removeSSHTunnelConfig={() =>
- setDB({
- type: ActionType.removeSSHTunnelConfig,
- })
- }
/>
);
+ const renderDefaultSSHTunnelSwitch = () => (
Review Comment:
Oh! Yeah, that makes sense. Let me work on that and update the PR. Will come back to you when ready 👍 Thanks!
--
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 merged pull request #22967: feat(ssh_tunnel): SSH Tunnel Switch extension
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #22967:
URL: https://github.com/apache/superset/pull/22967
--
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] eric-briscoe commented on a diff in pull request #22967: feat(ssh_tunnel): SSH Tunnel Switch extension
Posted by "eric-briscoe (via GitHub)" <gi...@apache.org>.
eric-briscoe commented on code in PR #22967:
URL: https://github.com/apache/superset/pull/22967#discussion_r1095201459
##########
superset-frontend/packages/superset-ui-core/src/ui-overrides/ExtensionsRegistry.ts:
##########
@@ -49,6 +49,15 @@ interface MenuObjectChildProps {
disable?: boolean;
}
+export interface SwitchProps {
+ isEditMode: boolean;
+ dbFetched: any;
+ disableSSHTunnelingForEngine?: boolean;
+ useSSHTunneling: boolean;
+ setUseSSHTunneling: React.Dispatch<React.SetStateAction<boolean>>;
+ setDB: React.Dispatch<any>;
Review Comment:
NOTE for other reviewers: @Antonio-RiveroMartnez and I discussed this and this any is a current necessity due to ExtensionsRegistry being in ui-core, and the DatabaseObject Type being in Superset-frontend/src and we do not want to duplicate Types, or create a circular dependency. Moving the DatabaseObject Type or resolving this by moving ExtensionRegistry to superset seemed out of scope for this PR but is being looked at in effort to improve the extensions capability.
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1346,14 +1357,33 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
payload: { login_method: method },
})
}
- removeSSHTunnelConfig={() =>
- setDB({
- type: ActionType.removeSSHTunnelConfig,
- })
- }
/>
);
+ const renderDefaultSSHTunnelSwitch = () => (
Review Comment:
@Antonio-RiveroMartnez one improvement we could consider is to converting renderDefaultSSHTunnelSwitch function into an actual component in separate file. Then import it at top of file. Also add a prop which is `isSSHTunneling` where the switch component just returns undefined so the show / hide logic is internal to the switch. Then we can collapse the conditional rendering blocks into a simple normal component block.
add the isSSHTunneling to interface:
````
export interface SwitchProps {
isEditMode: boolean;
isSSHTunneling: boolean;
dbFetched: any;
disableSSHTunnelingForEngine?: boolean;
useSSHTunneling: boolean;
setUseSSHTunneling: React.Dispatch<React.SetStateAction<boolean>>;
setDB: React.Dispatch<any>;
}
```
Then where we set the SSHTunnelSwitchExtension we could change to something like:
```
import SSHTunnelSwitch from './SSHTTunnelSwitch';
const SSHTunnelSwitchComponent= extensionsRegistry.get(
'ssh_tunnel.form.switch',
) ?? SSHTunnelSwitch;
```
this way lower in the code on lines 1591-1601 and 1877-1897
can look like:
```
<SSHTunnelSwitchComponent
isEditMode={isEditMode}
isSSHTunneling={isSSHTunneling}
dbFetched={dbFetched}
disableSSHTunnelingForEngine={disableSSHTunnelingForEngine}
useSSHTunneling={useSSHTunneling}
setUseSSHTunneling={setUseSSHTunneling}
setDB={setDB}
/>
```
and not need to do all the condition checks if using extension vs base component, etc. what do you think?
##########
superset-frontend/packages/superset-ui-core/src/ui-overrides/ExtensionsRegistry.ts:
##########
@@ -69,6 +78,7 @@ export type Extensions = Partial<{
'welcome.message': React.ComponentType;
'welcome.banner': React.ComponentType;
'welcome.main.replacement': React.ComponentType;
+ 'ssh_tunnel.form.switch': React.ComponentType<SwitchProps>;
Review Comment:
@Antonio-RiveroMartnez this is a great improvement to add props typing for extension points to create a protected contract, well done! I would like to make this a standard pattern to cover all the extension points in the future.
--
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