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

Move decideClient and decideLegacyClient to data source plugin util #7079

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Jun 20, 2024

Description

  • Move decideClient and decideLegacyClient from data plugin to data source plugin util, and update the usage
  • added unit tests

Issues Resolved

#6417

Screenshot

Testing the changes

Changelog

  • refactor: Move decideClient and decideLegacy client to data source plugin util

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

@zhongnansu zhongnansu changed the title Move decideClient and decideLegacy client to data source plugin util [not ready for review]Move decideClient and decideLegacy client to data source plugin util Jun 20, 2024
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

@zhongnansu zhongnansu changed the title [not ready for review]Move decideClient and decideLegacy client to data source plugin util Move decideClient and decideLegacy client to data source plugin util Jun 21, 2024
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

@zhongnansu zhongnansu added refactor Tech debt related tasks that need refactoring multiple datasource multiple datasource project failed changeset v2.16.0 labels Jun 21, 2024
@zhongnansu zhongnansu changed the title Move decideClient and decideLegacy client to data source plugin util Move decideClient and decideLegacyClient to data source plugin util Jun 21, 2024
Copy link

codecov bot commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.45%. Comparing base (b85e177) to head (24eb251).

Files Patch % Lines
src/plugins/data/server/index_patterns/routes.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7079   +/-   ##
=======================================
  Coverage   67.45%   67.45%           
=======================================
  Files        3448     3448           
  Lines       67964    67964           
  Branches    11057    11057           
=======================================
+ Hits        45845    45847    +2     
+ Misses      19446    19444    -2     
  Partials     2673     2673           
Flag Coverage Δ
Linux_1 33.10% <22.22%> (ø)
Linux_2 55.06% <22.22%> (ø)
Linux_3 45.24% <77.77%> (+<0.01%) ⬆️
Linux_4 34.83% <22.22%> (ø)
Windows_1 33.12% <22.22%> (ø)
Windows_2 55.01% <22.22%> (ø)
Windows_3 45.26% <77.77%> (+<0.01%) ⬆️
Windows_4 34.83% <22.22%> (ø)

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.

zhongnansu pushed a commit to zhongnansu/OpenSearch-Dashboards that referenced this pull request Jun 24, 2024
@BionIT
Copy link
Collaborator

BionIT commented Jun 24, 2024

@zhongnansu Thanks for refactoring the util! Could you take a look at the failed workflow when having time?

BionIT
BionIT previously approved these changes Jun 24, 2024
zengyan-amazon
zengyan-amazon previously approved these changes Jun 24, 2024
@zhongnansu
Copy link
Member Author

zhongnansu commented Jun 25, 2024

@zhongnansu Thanks for refactoring the util! Could you take a look at the failed workflow when having time?

Unit test in core -> ui_settings -> create_or_upgrade_saved_config.test.ts are failing, which is very weird, because I didn't touch any code there. Need to dive deep on this. Noticed that once I remove my change, this test can pass. And the error message is indicating module mock (doMock) is not working as expected.

Summary of all failing tests
 FAIL  src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.test.ts
  ● uiSettings/createOrUpgradeSavedConfig › nothing is upgradeable › should create config with current version and buildNum

    TypeError: Cannot destructure property 'saved_objects' of '((cov_20o48geyyw(...).s[0]++) , (intermediate value))' as it is undefined.

      47 | }) {
      48 |   // attempt to find a config we can upgrade
    > 49 |   const { saved_objects: savedConfigs } = await savedObjectsClient.find({
         |                          ^
      50 |     type: 'config',
      51 |     page: 1,
      52 |     perPage: 1000,

      at getUpgradeableConfig (src/core/server/ui_settings/create_or_upgrade_saved_config/get_upgradeable_config.ts:49:26)
      at createOrUpgradeSavedConfig (src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.ts:53:29)
      at run (src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.test.ts:62:20)
      at Object.<anonymous> (src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.test.ts:91:7)

Attach the failed CI: https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/9639731291/job/26622985697?pr=7079

@BionIT BionIT merged commit ddf1a41 into opensearch-project:main Jul 1, 2024
66 of 67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 1, 2024
…n util (#7079)

Signed-off-by: Zhongnan Su <[email protected]>
(cherry picked from commit ddf1a41)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
zhongnansu pushed a commit that referenced this pull request Jul 1, 2024
…n util (#7079)

Signed-off-by: Zhongnan Su <[email protected]>
(cherry picked from commit ddf1a41)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
BionIT pushed a commit that referenced this pull request Jul 1, 2024
…n util (#7079) (#7138)

(cherry picked from commit ddf1a41)

Signed-off-by: Zhongnan Su <[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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x distinguished-contributor multiple datasource multiple datasource project refactor Tech debt related tasks that need refactoring v2.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants