Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Workspace] Refactor 'Associate data sources' in create page to support direct query connections #7961

Merged
merged 29 commits into from
Sep 5, 2024

Conversation

Kapian1234
Copy link
Contributor

@Kapian1234 Kapian1234 commented Sep 2, 2024

Description

Refactor 'Associate data sources' in create page to support direct query connections

Issues Resolved

Screenshot

Screenshot 2024-08-13 at 17 18 24
Screenshot 2024-09-02 at 16 13 12

Testing the changes

Changelog

  • feat: Support DQCs in create page

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 91.36691% with 12 lines in your changes missing coverage. Please review.

Project coverage is 60.55%. Comparing base (7e8612f) to head (b4b1ac7).
Report is 91 commits behind head on main.

Files with missing lines Patch % Lines
src/plugins/workspace/public/utils.ts 60.00% 8 Missing ⚠️
...ents/workspace_detail/select_data_source_panel.tsx 88.23% 2 Missing ⚠️
...components/workspace_creator/workspace_creator.tsx 75.00% 0 Missing and 1 partial ⚠️
...onents/workspace_form/select_data_source_panel.tsx 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7961      +/-   ##
==========================================
+ Coverage   60.52%   60.55%   +0.02%     
==========================================
  Files        3728     3730       +2     
  Lines       88295    88365      +70     
  Branches    13661    13681      +20     
==========================================
+ Hits        53441    53507      +66     
- Misses      31600    31606       +6     
+ Partials     3254     3252       -2     
Flag Coverage Δ
Linux_1 28.76% <91.36%> (+0.04%) ⬆️
Linux_2 56.25% <ø> (+<0.01%) ⬆️
Linux_3 37.40% <ø> (-0.02%) ⬇️
Linux_4 29.62% <ø> (+0.01%) ⬆️
Windows_1 28.78% <91.36%> (+0.04%) ⬆️
Windows_2 56.20% <ø> (+<0.01%) ⬆️
Windows_3 37.40% <ø> (-0.02%) ⬇️
Windows_4 29.62% <ø> (+0.02%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -174,27 +182,26 @@ export const DataSourceConnectionTable = ({
},
},
{
width: '10%',
width: '15%',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kapian1234 Seems we are re-use the same component with workspace detail side. Could you help check with @yubonluo if this width updates can be applied in workspace detail? I'm prefer to add flag here to determine if we need to update the width.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The width updates will indeed be applied in workspace detail, but I've checked with @yubonluo and the new width still presents well there. Perhaps the flag isn't necessary?

wanglam and others added 2 commits September 4, 2024 16:08
Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Kapian1234 <[email protected]>
Comment on lines 125 to 128
url = `${origin}${basePath}/app/dataSources/manage/${name}?dataSourceMDSId=${record.parentId}`;
url = http.basePath.prepend(`/app/dataSources/${record.id}`, {
withoutClientBasePath: true,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
url = `${origin}${basePath}/app/dataSources/manage/${name}?dataSourceMDSId=${record.parentId}`;
url = http.basePath.prepend(`/app/dataSources/${record.id}`, {
withoutClientBasePath: true,
});
url = http.basePath.prepend(`/app/dataSources/manage/${name}?dataSourceMDSId=${record.parentId}`, {
withoutClientBasePath: true,
});

Comment on lines 121 to 123
url = http.basePath.prepend(`/app/dataSources/${record.id}`, {
withoutClientBasePath: true,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
url = http.basePath.prepend(`/app/dataSources/${record.id}`, {
withoutClientBasePath: true,
});
url = http.basePath.prepend(`/app/dataSources/${record.id}`);

If it's opensearch connection, shall we just go to the data source page within the workspace?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go to the data source management page within the workspace. @yubonluo , could you help clarify this?

Comment on lines 31 to 40
type DataSourceConnectionTableProps = Omit<
EuiInMemoryTableProps<DataSourceConnection>,
| 'columns'
| 'itemsId'
| 'isSelectable'
| 'itemIdToExpandedRowMap'
| 'isExpandable'
| 'selection'
| 'pagination'
> & {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type DataSourceConnectionTableProps = Omit<
EuiInMemoryTableProps<DataSourceConnection>,
| 'columns'
| 'itemsId'
| 'isSelectable'
| 'itemIdToExpandedRowMap'
| 'isExpandable'
| 'selection'
| 'pagination'
> & {
type DataSourceConnectionTableProps = {
tableProps: Partial<EuiInMemoryTableProps<DataSourceConnection>>

What about we moving all the table related props to a standalone entry so that it won't bother other props. And to make the props simple by using Partial.

)}
</>
<EuiInMemoryTable
{...(restProps as EuiInMemoryTableProps<DataSourceConnection>)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{...(restProps as EuiInMemoryTableProps<DataSourceConnection>)}
{...restProps}

We'd better avoid type assertion.

ruanyl
ruanyl previously approved these changes Sep 5, 2024
@SuZhou-Joe
Copy link
Member

Run cypress tests (osd:ciGroup6) should be irrelevant to the code changes.

@SuZhou-Joe SuZhou-Joe merged commit 16d160a into opensearch-project:main Sep 5, 2024
66 of 67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 5, 2024
…rt direct query connections (#7961)

* support DQC

Signed-off-by: Kapian1234 <[email protected]>

* Fix UTs in workspace form select data source panel

Signed-off-by: Lin Wang <[email protected]>

* Remove no need IntlProvider

Signed-off-by: Lin Wang <[email protected]>

* Add aria-labelledby for permission inputs

Signed-off-by: Lin Wang <[email protected]>

* Modify UTs

Signed-off-by: Kapian1234 <[email protected]>

* Changeset file for PR #7961 created/updated

* Modify UTs

Signed-off-by: Kapian1234 <[email protected]>

* Resolve some issues

Signed-off-by: Kapian1234 <[email protected]>

* Modify UTs

Signed-off-by: Kapian1234 <[email protected]>

* Fix UT errror

Signed-off-by: Lin Wang <[email protected]>

* update button text

Signed-off-by: Kapian1234 <[email protected]>

* rename onSelectItems()

Signed-off-by: Kapian1234 <[email protected]>

* Fix an error

Signed-off-by: Kapian1234 <[email protected]>

* Refactor data source connection table

Signed-off-by: Lin Wang <[email protected]>

* resolve some issues

Signed-off-by: Kapian1234 <[email protected]>

* resolve some issues

Signed-off-by: Kapian1234 <[email protected]>

* Fix the data source URL reference

Signed-off-by: Kapian1234 <[email protected]>

* Move restProps to tableProps

Signed-off-by: Lin Wang <[email protected]>

* Fix table not unmont after connection type changed

Signed-off-by: Lin Wang <[email protected]>

* Refactor selectedDataSources to selectedDataSourceConnections

Signed-off-by: Lin Wang <[email protected]>

* Load direct query connections after data source tab selected

Signed-off-by: Lin Wang <[email protected]>

---------

Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Co-authored-by: Lin Wang <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit 16d160a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SuZhou-Joe pushed a commit that referenced this pull request Sep 5, 2024
…rt direct query connections (#7961) (#8024)

* support DQC



* Fix UTs in workspace form select data source panel



* Remove no need IntlProvider



* Add aria-labelledby for permission inputs



* Modify UTs



* Changeset file for PR #7961 created/updated

* Modify UTs



* Resolve some issues



* Modify UTs



* Fix UT errror



* update button text



* rename onSelectItems()



* Fix an error



* Refactor data source connection table



* resolve some issues



* resolve some issues



* Fix the data source URL reference



* Move restProps to tableProps



* Fix table not unmont after connection type changed



* Refactor selectedDataSources to selectedDataSourceConnections



* Load direct query connections after data source tab selected



---------





(cherry picked from commit 16d160a)

Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Lin Wang <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 6, 2024
…rt direct query connections (#7961) (#8024)

* support DQC

* Fix UTs in workspace form select data source panel

* Remove no need IntlProvider

* Add aria-labelledby for permission inputs

* Modify UTs

* Changeset file for PR #7961 created/updated

* Modify UTs

* Resolve some issues

* Modify UTs

* Fix UT errror

* update button text

* rename onSelectItems()

* Fix an error

* Refactor data source connection table

* resolve some issues

* resolve some issues

* Fix the data source URL reference

* Move restProps to tableProps

* Fix table not unmont after connection type changed

* Refactor selectedDataSources to selectedDataSourceConnections

* Load direct query connections after data source tab selected

---------

(cherry picked from commit 16d160a)

Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Lin Wang <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit 3f2d867)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SuZhou-Joe pushed a commit that referenced this pull request Sep 6, 2024
…rt direct query connections (#7961) (#8024) (#8055)

* support DQC

* Fix UTs in workspace form select data source panel

* Remove no need IntlProvider

* Add aria-labelledby for permission inputs

* Modify UTs

* Changeset file for PR #7961 created/updated

* Modify UTs

* Resolve some issues

* Modify UTs

* Fix UT errror

* update button text

* rename onSelectItems()

* Fix an error

* Refactor data source connection table

* resolve some issues

* resolve some issues

* Fix the data source URL reference

* Move restProps to tableProps

* Fix table not unmont after connection type changed

* Refactor selectedDataSources to selectedDataSourceConnections

* Load direct query connections after data source tab selected

---------

(cherry picked from commit 16d160a)







(cherry picked from commit 3f2d867)

Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Lin Wang <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants