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 2023/01/11 18:18:57 UTC
[GitHub] [superset] hughhhh opened a new pull request, #22689: feat: add ssh tunneling to dynamic form
hughhhh opened a new pull request, #22689:
URL: https://github.com/apache/superset/pull/22689
<!---
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] lyndsiWilliams commented on a diff in pull request #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1072761061
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -549,7 +555,10 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
DB.backend === db?.engine || DB.engine === db?.engine,
) as DatabaseObject
)?.engine_information?.allow_ssh_tunneling;
- const sshTunneling = isFeatureEnabled(FeatureFlag.SSH_TUNNELING);
+ const sshTunneling =
+ isFeatureEnabled(FeatureFlag.SSH_TUNNELING) &&
+ engineAllowsSSHTunneling !== undefined &&
+ engineAllowsSSHTunneling;
Review Comment:
```suggestion
isFeatureEnabled(FeatureFlag.SSH_TUNNELING) &&
engineAllowsSSHTunneling;
```
Could this be simplified if just checking for truthiness? Or is there something specific that needs the `!== undefined` check?
--
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 #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1072830286
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1287,6 +1296,37 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
});
};
+ const renderSSHTunnelForm = () => (
+ <SSHTunnelForm
+ isEditMode={isEditMode}
+ sshTunneling={sshTunneling}
+ db={db as DatabaseObject}
Review Comment:
Might be wrong but, isn't this:
```
const [db, setDB] = useReducer<
Reducer<Partial<DatabaseObject> | null, DBReducerActionType>
>(dbReducer, null);
```
forcing `db` to be `Partial<DatabaseObject> | null` ? thus would match the type in `SSHTunnelForm`?
--
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 #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
hughhhh merged PR #22689:
URL: https://github.com/apache/superset/pull/22689
--
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 #22689: feat: add ssh tunneling to dynamic form
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1067453441
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1553,49 +1564,59 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
)}
</StyledAlignment>
) : (
- <DatabaseConnectionForm
- isEditMode
- sslForced={sslForced}
- dbModel={dbModel}
- db={db as DatabaseObject}
- onParametersChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.parametersChange, {
- type: target.type,
- name: target.name,
- checked: target.checked,
- value: target.value,
- })
- }
- onExtraInputChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.extraInputChange, {
- name: target.name,
- value: target.value,
- })
- }
- onChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.textChange, {
- name: target.name,
- value: target.value,
- })
- }
- onQueryChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.queryChange, {
- name: target.name,
- value: target.value,
- })
- }
- onAddTableCatalog={() =>
- setDB({ type: ActionType.addTableCatalogSheet })
- }
- onRemoveTableCatalog={(idx: number) =>
- setDB({
- type: ActionType.removeTableCatalogSheet,
- payload: { indexToDelete: idx },
- })
- }
- getValidation={() => getValidation(db)}
- validationErrors={validationErrors}
- />
+ <div>
+ <DatabaseConnectionForm
Review Comment:
Are there any tests for this? :)
--
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] lyndsiWilliams commented on a diff in pull request #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1072955765
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -549,7 +555,10 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
DB.backend === db?.engine || DB.engine === db?.engine,
) as DatabaseObject
)?.engine_information?.allow_ssh_tunneling;
- const sshTunneling = isFeatureEnabled(FeatureFlag.SSH_TUNNELING);
+ const sshTunneling =
+ isFeatureEnabled(FeatureFlag.SSH_TUNNELING) &&
+ engineAllowsSSHTunneling !== undefined &&
+ engineAllowsSSHTunneling;
Review Comment:
```suggestion
isFeatureEnabled(FeatureFlag.SSH_TUNNELING) &&
engineAllowsSSHTunneling !== undefined;
```
That makes sense. In that case, would `engineAllowsSSHTunneling` be needed at the end? I think `engineAllowsSSHTunneling !== undefined` covers what it is checking for.
--
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 #22689: feat: add ssh tunneling to dynamic form
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1067393060
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1553,49 +1562,60 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
)}
</StyledAlignment>
) : (
- <DatabaseConnectionForm
- isEditMode
- sslForced={sslForced}
- dbModel={dbModel}
- db={db as DatabaseObject}
- onParametersChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.parametersChange, {
- type: target.type,
- name: target.name,
- checked: target.checked,
- value: target.value,
- })
- }
- onExtraInputChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.extraInputChange, {
- name: target.name,
- value: target.value,
- })
- }
- onChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.textChange, {
- name: target.name,
- value: target.value,
- })
- }
- onQueryChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.queryChange, {
- name: target.name,
- value: target.value,
- })
- }
- onAddTableCatalog={() =>
- setDB({ type: ActionType.addTableCatalogSheet })
- }
- onRemoveTableCatalog={(idx: number) =>
- setDB({
- type: ActionType.removeTableCatalogSheet,
- payload: { indexToDelete: idx },
- })
- }
- getValidation={() => getValidation(db)}
- validationErrors={validationErrors}
- />
+ <div>
+ hello1
Review Comment:
leftover?
--
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 #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1072830286
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1287,6 +1296,37 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
});
};
+ const renderSSHTunnelForm = () => (
+ <SSHTunnelForm
+ isEditMode={isEditMode}
+ sshTunneling={sshTunneling}
+ db={db as DatabaseObject}
Review Comment:
Might be wrong but, isn't this:
```
const [db, setDB] = useReducer<
Reducer<Partial<DatabaseObject> | null, DBReducerActionType>
>(dbReducer, null);
```
forcing db to be `Partial<DatabaseObject> | null` ? thus would match the type in `SSHTunnelForm`?
--
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 commented on a diff in pull request #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1072919190
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -549,7 +555,10 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
DB.backend === db?.engine || DB.engine === db?.engine,
) as DatabaseObject
)?.engine_information?.allow_ssh_tunneling;
- const sshTunneling = isFeatureEnabled(FeatureFlag.SSH_TUNNELING);
+ const sshTunneling =
+ isFeatureEnabled(FeatureFlag.SSH_TUNNELING) &&
+ engineAllowsSSHTunneling !== undefined &&
+ engineAllowsSSHTunneling;
Review Comment:
i had to explicitly add undefined for typing issues to go away since `engineAllowsSSHTunneling` key isn't always set for every engine coming back from the `available` endpoint
--
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] lyndsiWilliams commented on a diff in pull request #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1072703546
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx:
##########
@@ -1198,6 +1198,42 @@ describe('DatabaseModal', () => {
});
describe('SSH Tunnel Form interaction', () => {
+ test('properly interacts with SSH Tunnel form textboxes for dynamic form', async () => {
+ userEvent.click(
+ screen.getByRole('button', {
+ name: /postgresql/i,
+ }),
+ );
+ screen.logTestingPlaygroundURL();
Review Comment:
```suggestion
```
This should be removed
--
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 #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1067565937
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -549,7 +555,10 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
DB.backend === db?.engine || DB.engine === db?.engine,
) as DatabaseObject
)?.engine_information?.allow_ssh_tunneling;
- const sshTunneling = isFeatureEnabled(FeatureFlag.SSH_TUNNELING);
+ const sshTunneling =
Review Comment:
nit: would be more clear to use name like: `sshTunnelingEnabled` or `isSshTunnelingEnabled`
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1287,6 +1296,37 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
});
};
+ const renderSSHTunnelForm = () => (
+ <SSHTunnelForm
+ isEditMode={isEditMode}
+ sshTunneling={sshTunneling}
+ db={db as DatabaseObject}
Review Comment:
Not new to this PR so maybe not in scope, but is there any validation on `db` to ensure it is a `DatabaseObject`? Forcing a coercion here and on line 1304 basically removes any useful type checking. We may want to add a type guard or other validation that db does have the structure SSHTunnelForm expects.
--
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 #22689: feat: add ssh tunneling to dynamic form
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22689:
URL: https://github.com/apache/superset/pull/22689#issuecomment-1379310305
# [Codecov](https://codecov.io/gh/apache/superset/pull/22689?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 [#22689](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bd141e0) into [master](https://codecov.io/gh/apache/superset/commit/0908fd291bedfab51ccd7d67a959e0137d45b9c4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0908fd2) will **decrease** coverage by `11.23%`.
> The diff coverage is `43.54%`.
> :exclamation: Current head bd141e0 differs from pull request most recent head ec9788d. Consider uploading reports for the commit ec9788d to get more accurate results
```diff
@@ Coverage Diff @@
## master #22689 +/- ##
===========================================
- Coverage 67.08% 55.85% -11.24%
===========================================
Files 1869 1869
Lines 71585 71616 +31
Branches 7806 7806
===========================================
- Hits 48025 40001 -8024
- Misses 21532 29587 +8055
Partials 2028 2028
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| postgres | `?` | |
| presto | `52.49% <30.30%> (-0.02%)` | :arrow_down: |
| python | `57.89% <57.57%> (-23.40%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `51.49% <57.57%> (+<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/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ackages/superset-ui-core/src/utils/featureFlags.ts](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZmVhdHVyZUZsYWdzLnRz) | `100.00% <ø> (ø)` | |
| [...ase/DatabaseModal/DatabaseConnectionForm/index.tsx](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL0RhdGFiYXNlQ29ubmVjdGlvbkZvcm0vaW5kZXgudHN4) | `90.90% <ø> (ø)` | |
| [...RUD/data/database/DatabaseModal/SqlAlchemyForm.tsx](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL1NxbEFsY2hlbXlGb3JtLnRzeA==) | `61.53% <ø> (ø)` | |
| [superset/databases/ssh\_tunnel/models.py](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NzaF90dW5uZWwvbW9kZWxzLnB5) | `74.19% <11.11%> (-21.46%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `43.76% <27.58%> (ø)` | |
| [superset/databases/commands/test\_connection.py](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3Rlc3RfY29ubmVjdGlvbi5weQ==) | `62.65% <40.00%> (-34.79%)` | :arrow_down: |
| [superset/utils/ssh\_tunnel.py](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvc3NoX3R1bm5lbC5weQ==) | `47.36% <66.66%> (-32.64%)` | :arrow_down: |
| [superset/databases/schemas.py](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | `85.43% <100.00%> (-12.91%)` | :arrow_down: |
| [superset/databases/ssh\_tunnel/commands/update.py](https://codecov.io/gh/apache/superset/pull/22689?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) | `90.62% <100.00%> (-2.93%)` | :arrow_down: |
| [superset/databases/ssh\_tunnel/dao.py](https://codecov.io/gh/apache/superset/pull/22689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NzaF90dW5uZWwvZGFvLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [303 more](https://codecov.io/gh/apache/superset/pull/22689?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 #22689: feat: add ssh tunneling to dynamic form
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1067403710
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1553,49 +1562,59 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
)}
</StyledAlignment>
) : (
- <DatabaseConnectionForm
- isEditMode
- sslForced={sslForced}
- dbModel={dbModel}
- db={db as DatabaseObject}
- onParametersChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.parametersChange, {
- type: target.type,
- name: target.name,
- checked: target.checked,
- value: target.value,
- })
- }
- onExtraInputChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.extraInputChange, {
- name: target.name,
- value: target.value,
- })
- }
- onChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.textChange, {
- name: target.name,
- value: target.value,
- })
- }
- onQueryChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.queryChange, {
- name: target.name,
- value: target.value,
- })
- }
- onAddTableCatalog={() =>
- setDB({ type: ActionType.addTableCatalogSheet })
- }
- onRemoveTableCatalog={(idx: number) =>
- setDB({
- type: ActionType.removeTableCatalogSheet,
- payload: { indexToDelete: idx },
- })
- }
- getValidation={() => getValidation(db)}
- validationErrors={validationErrors}
- />
+ <div>
Review Comment:
There's no need to wrap it with a `div`
--
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 #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1068645634
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx:
##########
@@ -1198,6 +1198,41 @@ describe('DatabaseModal', () => {
});
describe('SSH Tunnel Form interaction', () => {
+ test('properly interacts with SSH Tunnel form textboxes for dynamic form', async () => {
Review Comment:
It seems the CI it's failing:
```
Functional: Create new database › SQL Alchemy form flow › SSH Tunnel Form interaction › properly interacts with SSH Tunnel form textboxes for dynamic form
TestingLibraryElementError: Unable to find an element with the text: /step 2 of 2/i. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.
```
More likely because you need the `allow_ssh_tunneling` set to `true` for Postgres engine, otherwise it wont render the form.
--
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 #22689: feat: add ssh tunneling to dynamic form
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1067403710
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1553,49 +1562,59 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
)}
</StyledAlignment>
) : (
- <DatabaseConnectionForm
- isEditMode
- sslForced={sslForced}
- dbModel={dbModel}
- db={db as DatabaseObject}
- onParametersChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.parametersChange, {
- type: target.type,
- name: target.name,
- checked: target.checked,
- value: target.value,
- })
- }
- onExtraInputChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.extraInputChange, {
- name: target.name,
- value: target.value,
- })
- }
- onChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.textChange, {
- name: target.name,
- value: target.value,
- })
- }
- onQueryChange={({ target }: { target: HTMLInputElement }) =>
- onChange(ActionType.queryChange, {
- name: target.name,
- value: target.value,
- })
- }
- onAddTableCatalog={() =>
- setDB({ type: ActionType.addTableCatalogSheet })
- }
- onRemoveTableCatalog={(idx: number) =>
- setDB({
- type: ActionType.removeTableCatalogSheet,
- payload: { indexToDelete: idx },
- })
- }
- getValidation={() => getValidation(db)}
- validationErrors={validationErrors}
- />
+ <div>
Review Comment:
There's no need to wrap it with a `div`, is 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] Antonio-RiveroMartnez commented on a diff in pull request #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1068645634
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx:
##########
@@ -1198,6 +1198,41 @@ describe('DatabaseModal', () => {
});
describe('SSH Tunnel Form interaction', () => {
+ test('properly interacts with SSH Tunnel form textboxes for dynamic form', async () => {
Review Comment:
It seems the CI it's failing:
```
Functional: Create new database › SQL Alchemy form flow › SSH Tunnel Form interaction › properly interacts with SSH Tunnel form textboxes for dynamic form
TestingLibraryElementError: Unable to find an element with the text: /step 2 of 2/i. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.
```
More likely because you need the `allow_ssh_tunneling` set to `true` for Postgres engine, otherwise it wont render the form.
--
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 commented on a diff in pull request #22689: feat: add ssh tunneling to dynamic form for Database Connection UI
Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1073490146
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1287,6 +1296,37 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
});
};
+ const renderSSHTunnelForm = () => (
+ <SSHTunnelForm
+ isEditMode={isEditMode}
+ sshTunneling={sshTunneling}
+ db={db as DatabaseObject}
Review Comment:
@eric-briscoe i say we cut a ticket for this under tech debt so we can revisit doing a proper refactor of this component
--
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