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

Setting savedAugmentVisLoader in Visualizations plugin #3616

Conversation

amitgalitz
Copy link
Member

Description

Currently when utilizing the latest commit in feature-anywhere branch while trying to use it as a dependency in the ad plugin I get an error displaying any visualizations across all of dashboards since the visAugment loader needs to be set before calling get in visualizations plugin. Related comment here: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3131/files#r1137998192

Screenshot 2023-03-15 at 8 26 37 PM

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

@amitgalitz amitgalitz marked this pull request as ready for review March 16, 2023 17:29
@amitgalitz amitgalitz requested a review from a team as a code owner March 16, 2023 17:29
@ohltyler
Copy link
Member

We have a circular dependency issue if we add visAugmenter as a required plugin in visualizations plugin. Instead, since visAugmenter is already a required bundle, we can use the exported function to re-create the loader in start() ourselves in the visualizations plugin - see this commit as an example.

@amitgalitz
Copy link
Member Author

We have a circular dependency issue if we add visAugmenter as a required plugin in visualizations plugin. Instead, since visAugmenter is already a required bundle, we can use the exported function to re-create the loader in start() ourselves in the visualizations plugin - see this commit as an example.

Applied fix

Signed-off-by: Amit Galitzky <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #3616 (a587361) into feature/feature-anywhere (236f49f) will decrease coverage by 0.17%.
The diff coverage is 50.00%.

📣 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                      @@
##           feature/feature-anywhere    #3616      +/-   ##
============================================================
- Coverage                     66.58%   66.42%   -0.17%     
============================================================
  Files                          3216     3218       +2     
  Lines                         61506    61748     +242     
  Branches                       9476     9520      +44     
============================================================
+ Hits                          40954    41015      +61     
- Misses                        18294    18454     +160     
- Partials                       2258     2279      +21     
Flag Coverage Δ
Linux ?
Windows 66.42% <50.00%> (-0.12%) ⬇️

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

Impacted Files Coverage Δ
src/dev/build/tasks/patch_native_modules_task.ts 86.20% <ø> (ø)
src/dev/notice/generate_notice_from_source.ts 14.28% <ø> (ø)
src/plugins/data/server/plugin.ts 0.00% <0.00%> (ø)
src/plugins/data/server/search/collectors/usage.ts 22.72% <ø> (-3.36%) ⬇️
.../plugins/data_source/server/data_source_service.ts 71.42% <ø> (ø)
.../data_source_management/public/components/utils.ts 100.00% <ø> (+4.34%) ⬆️
...omponents/validation/datasource_form_validation.ts 75.86% <0.00%> (-24.14%) ⬇️
src/plugins/data_source_management/public/types.ts 100.00% <ø> (ø)
...egion_map/public/components/map_choice_options.tsx 53.33% <ø> (ø)
.../public/application/components/aggs/serial_diff.js 18.18% <0.00%> (ø)
... and 17 more

... and 4 files 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.

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

I'm not quite sure if this is the correct final dependency model, but it's something we can revisit in context for the final review. Will approve to unblock branch development.

@abbyhu2000 abbyhu2000 merged commit 79b9f50 into opensearch-project:feature/feature-anywhere Mar 22, 2023
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.

6 participants