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

Add new advanced settings for vis augmenter #3961

Merged

Conversation

lezzago
Copy link
Member

@lezzago lezzago commented May 1, 2023

Description

  • Add new advanced settings for vis augmenter
  • Sets default amount of associated plugin resources to a visualization is 10

Issues Resolved

#3361

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

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

codecov-commenter commented May 1, 2023

Codecov Report

Merging #3961 (bb5dc5b) into feature/feature-anywhere (2cc88f3) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

@@                     Coverage Diff                      @@
##           feature/feature-anywhere    #3961      +/-   ##
============================================================
- Coverage                     66.53%   66.52%   -0.02%     
============================================================
  Files                          3245     3246       +1     
  Lines                         62475    62514      +39     
  Branches                       9645     9650       +5     
============================================================
+ Hits                          41569    41585      +16     
- Misses                        18598    18619      +21     
- Partials                       2308     2310       +2     
Flag Coverage Δ
Linux 66.46% <60.00%> (-0.02%) ⬇️
Windows 66.47% <60.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
..._collection/server/collectors/management/schema.ts 100.00% <ø> (ø)
src/plugins/vis_augmenter/public/plugin.ts 0.00% <0.00%> (ø)
...ugmenter/public/saved_augment_vis/utils/helpers.ts 6.66% <0.00%> (-26.67%) ⬇️
...ic/embeddable/create_vis_embeddable_from_object.ts 5.26% <ø> (ø)
...izations/public/embeddable/visualize_embeddable.ts 0.60% <0.00%> (-0.01%) ⬇️
src/plugins/vis_augmenter/public/utils/utils.ts 93.02% <71.42%> (-4.28%) ⬇️
...nter/public/saved_augment_vis/saved_augment_vis.ts 97.43% <97.29%> (-2.57%) ⬇️
src/plugins/vis_augmenter/common/constants.ts 100.00% <100.00%> (ø)
src/plugins/vis_augmenter/public/services.ts 100.00% <100.00%> (ø)
src/plugins/visualizations/public/services.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

lezzago added 2 commits May 12, 2023 16:55
Signed-off-by: Ashish Agrawal <[email protected]>
lezzago added 2 commits May 17, 2023 08:55
Signed-off-by: Ashish Agrawal <[email protected]>
Signed-off-by: Ashish Agrawal <[email protected]>
lezzago added 2 commits May 17, 2023 10:00
Signed-off-by: Ashish Agrawal <[email protected]>
Signed-off-by: Ashish Agrawal <[email protected]>
Comment on lines 217 to 224
const savedAugmentVisLoader = createSavedAugmentVisLoader({
savedObjectsClient: core.savedObjects.client,
indexPatterns: data.indexPatterns,
search: data.search,
chrome: core.chrome,
overlays: core.overlays,
});
setSavedAugmentVisLoader(savedAugmentVisLoader);
Copy link
Member

Choose a reason for hiding this comment

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

We will still need this - why have you removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

brought it back

lezzago added 2 commits May 18, 2023 16:10
Signed-off-by: Ashish Agrawal <[email protected]>
Signed-off-by: Ashish Agrawal <[email protected]>
Copy link
Member

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Cool, it's nice to see this all coming together.

# opensearchDashboards.survey.url: "https://survey.opensearch.org"

# Set the value of this setting to false to disable plugin augmentation on Dashboard
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we're consistent in this file, but I prefer the form of L232-234; because we provide the commented out default value, the instructions should be about setting it to the non-default:

Suggested change
# Set the value of this setting to false to disable plugin augmentation on Dashboard
# Set the value of this setting to true to enable plugin augmentation on Dashboard visualizations

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see the default is intended to be true. In that case, keep the text and change the commented value.

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 followed the standard of the other settings and the commented value should be false based on that.

Comment on lines +60 to +62
if (!isAugmentationEnabled) {
throw new Error(
'Visualization augmentation is disabled, please enable visualization:enablePluginAugmentation.'
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope and unrelated to feature anywhere - but ideally we'd make it easy for these types of configuration errors to directly deep link to the relevant setting. I'll open an issue or add to our existing issue about error handling.

Copy link
Member

Choose a reason for hiding this comment

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

added to #1625 (comment)

Comment on lines +40 to +42
`Cannot associate the plugin resource to the visualization due to the limit of the max
amount of associated plugin resources (${maxAssociatedCount}) with
${savedObjectsForThisVisualization.length} associated to the visualization`
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this error message to indicate that visualization:enablePluginAugmentation.maxPluginObjects is actually a user-configurable setting?

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

The change looks good. The coverage can be added in a followup PR, but calling it out here since it can affect merge to main.

import {
PLUGIN_AUGMENTATION_ENABLE_SETTING,
PLUGIN_AUGMENTATION_MAX_OBJECTS_SETTING,
} from '../../../common/constants';

/**
* Create an augment vis saved object given an object that
* implements the ISavedAugmentVis interface
*/
export const createAugmentVisSavedObject = async (AugmentVis: ISavedAugmentVis): Promise<any> => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add unit tests here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will follow up in another pr

// Checks if the global yaml setting for enabling plugin augmentation is disabled.
// If it is disabled, remove the settings as we would not want to show these to the
// user due to it being disabled at the cluster level.
if (isAugmentationEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

This is actually pretty neat that you are relying on the missing UI settings to disable it in the plugin. Calling it out in the comment here will be really helpful in the future though since it may not be obvious that you are doing it this way and its anon standard pattern in the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will follow up in another pr with the tests

id: this.id,
});
try {
const augmentVisSavedObjs = await getAugmentVisSavedObjs(
Copy link
Member

Choose a reason for hiding this comment

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

There seem to be a bunch of coverage failures here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will follow up in another pr

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