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

[Multiple DataSource] Integrate multiple datasource with dev tool console #3754

Merged
merged 3 commits into from
Apr 6, 2023

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Mar 31, 2023

Description

[Multiple DataSource] Integrate multiple datasource with dev tool console

Update:

After #3775 fixes autocomplete issue, I updated this PR to adopt the change, autocomplete also works for datasources now

Video

Screen.Recording.2023-03-31.at.10.34.05.AM.mov

Issues Resolved

#2699
part of #3577

Check List

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

@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

Merging #3754 (076c04c) into main (29008fa) will decrease coverage by 0.02%.
The diff coverage is 45.83%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3754      +/-   ##
==========================================
- Coverage   66.43%   66.42%   -0.02%     
==========================================
  Files        3209     3209              
  Lines       61677    61685       +8     
  Branches     9521     9525       +4     
==========================================
- Hits        40978    40977       -1     
- Misses      18419    18424       +5     
- Partials     2280     2284       +4     
Flag Coverage Δ
Linux 66.37% <45.83%> (-0.02%) ⬇️
Windows 66.38% <45.83%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...equest_to_opensearch/send_request_to_opensearch.ts 70.49% <ø> (ø)
...ugins/console/public/application/stores/request.ts 55.00% <0.00%> (-13.75%) ⬇️
...lugins/console/public/lib/opensearch/opensearch.ts 75.00% <ø> (ø)
...rver/routes/api/console/proxy/validation_config.ts 100.00% <ø> (ø)
.../data_source_management/public/components/utils.ts 100.00% <ø> (ø)
...rc/plugins/console/public/lib/mappings/mappings.js 72.78% <37.50%> (ø)
.../server/routes/api/console/proxy/create_handler.ts 68.29% <40.00%> (-6.07%) ⬇️
...ensearch/use_send_current_request_to_opensearch.ts 68.00% <66.66%> (ø)
...containers/editor/legacy/console_editor/editor.tsx 55.00% <100.00%> (ø)
...ole/server/routes/api/console/proxy/tests/mocks.ts 90.90% <100.00%> (+2.02%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zhongnansu
Copy link
Member Author

cc: @KrooshalUX for UI/UX feedback

@KrooshalUX
Copy link

LGTM from UX perspective - small nit, but it looks like the use of "tabs" means that the horizontal rule is getting truncated in order to accommodate the picker. Not blocking now, but if its an easy fix to remove the Tab, change "console" into a header, and place in a horizontal rule, that would be a nice touch.

@zhongnansu zhongnansu requested a review from KrooshalUX April 4, 2023 23:17
kristenTian
kristenTian previously approved these changes Apr 5, 2023
kristenTian
kristenTian previously approved these changes Apr 5, 2023
@zhongnansu
Copy link
Member Author

LGTM from UX perspective - small nit, but it looks like the use of "tabs" means that the horizontal rule is getting truncated in order to accommodate the picker. Not blocking now, but if its an easy fix to remove the Tab, change "console" into a header, and place in a horizontal rule, that would be a nice touch.

The tricky part is, tab is generated not by console plugin, but by dev_tool plugin. There's a chance that other plugins are relying dev_tool plugin to register themselves, which will generate another tab in dev tool page. If we change this, it will break the contract with existing plugins. I'd suggest we review the blast radius and have the change in 3.0.0 @KrooshalUX

kristenTian
kristenTian previously approved these changes Apr 6, 2023
const mountedTool = useRef<MountedDevToolDescriptor | null>(null);
const [dataSources, setDataSources] = useState<DataSourceOption[]>([]);
const [selectedOptions, setSelected] = useState<DataSourceOption[]>([]);
Copy link
Member

Choose a reason for hiding this comment

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

nit: setSelectedOptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, let me fix this

@@ -62,7 +62,7 @@ export class ConsoleUIPlugin implements Plugin<void, void, AppSetupUIPluginDepen
defaultMessage: 'Console',
}),
enableRouting: false,
mount: async ({ element }) => {
mount: async ({ element, dataSourceId }) => {
const [core] = await getStartServices();

Copy link
Member

Choose a reason for hiding this comment

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

Should we follow the format as other plugins here?

mount: async (params: AppMountParameters) => {
        const { element, dataSourceId } = params;

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather keep it, reducing 1 more line of code~

@KrooshalUX
Copy link

LGTM from UX perspective - small nit, but it looks like the use of "tabs" means that the horizontal rule is getting truncated in order to accommodate the picker. Not blocking now, but if its an easy fix to remove the Tab, change "console" into a header, and place in a horizontal rule, that would be a nice touch.

The tricky part is, tab is generated not by console plugin, but by dev_tool plugin. There's a chance that other plugins are relying dev_tool plugin to register themselves, which will generate another tab in dev tool page. If we change this, it will break the contract with existing plugins. I'd suggest we review the blast radius and have the change in 3.0.0 @KrooshalUX

Gotcha, that is helpful context. I have some ideas that I can take to the feature owners for further exploration.

abbyhu2000
abbyhu2000 previously approved these changes Apr 6, 2023
@zhongnansu zhongnansu dismissed stale reviews from abbyhu2000 and kristenTian via 2fbbaa0 April 6, 2023 23:06
@zhongnansu
Copy link
Member Author

address the comment, CI should be good, since the change is only function naming, merging

@zhongnansu zhongnansu merged commit a6af77d into opensearch-project:main Apr 6, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 6, 2023
…sole (#3754)

* [Multiple DataSource] Integrate multiple datasource with dev tool console
* update to support autocomplete when datasource is selected

Signed-off-by: Su <[email protected]>
(cherry picked from commit a6af77d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
zhongnansu pushed a commit that referenced this pull request Apr 7, 2023
…sole (#3754) (#3802)

* [Multiple DataSource] Integrate multiple datasource with dev tool console
* update to support autocomplete when datasource is selected

Signed-off-by: Su <[email protected]>
(cherry picked from commit a6af77d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
…sole (opensearch-project#3754)

* [Multiple DataSource] Integrate multiple datasource with dev tool console
* update to support autocomplete when datasource is selected

Signed-off-by: Su <[email protected]>
Signed-off-by: David Sinclair <[email protected]>
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.

[MD]Multi data source integration with Dev tool
6 participants