-
Notifications
You must be signed in to change notification settings - Fork 917
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
Changes from 10 commits
d19a257
d2e2a48
282a7c6
e179ef5
42bd042
fdaf819
66f7f1f
67276c8
6c5cb9e
7f553f3
0b31c87
d60caa2
bb5dc5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
export const PLUGIN_AUGMENTATION_ENABLE_SETTING = 'visualization:enablePluginAugmentation'; | ||
export const PLUGIN_AUGMENTATION_MAX_OBJECTS_SETTING = | ||
'visualization:enablePluginAugmentation.maxPluginObjects'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { schema, TypeOf } from '@osd/config-schema'; | ||
|
||
export const configSchema = schema.object({ | ||
pluginAugmentationEnabled: schema.boolean({ defaultValue: true }), | ||
}); | ||
|
||
export type VisAugmenterPluginConfigType = TypeOf<typeof configSchema>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,47 @@ | |
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { getSavedAugmentVisLoader } from '../../services'; | ||
import { ISavedAugmentVis } from '../../types'; | ||
import { get } from 'lodash'; | ||
import { getSavedAugmentVisLoader, getUISettings } from '../../services'; | ||
import { ISavedAugmentVis } from '../types'; | ||
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> => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add unit tests here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will follow up in another pr |
||
const loader = getSavedAugmentVisLoader(); | ||
const config = getUISettings(); | ||
|
||
const isAugmentationEnabled = config.get(PLUGIN_AUGMENTATION_ENABLE_SETTING); | ||
if (!isAugmentationEnabled) { | ||
throw new Error( | ||
'Visualization augmentation is disabled, please enable visualization:enablePluginAugmentation.' | ||
); | ||
} | ||
const maxAssociatedCount = config.get(PLUGIN_AUGMENTATION_MAX_OBJECTS_SETTING); | ||
|
||
await loader.findAll().then(async (resp) => { | ||
if (resp !== undefined) { | ||
const savedAugmentObjects = get(resp, 'hits', []); | ||
// gets all the saved object for this visualization | ||
const savedObjectsForThisVisualization = savedAugmentObjects.filter( | ||
(savedObj) => get(savedObj, 'visId', '') === AugmentVis.visId | ||
); | ||
|
||
if (maxAssociatedCount <= savedObjectsForThisVisualization.length) { | ||
throw new Error( | ||
`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` | ||
Comment on lines
+40
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want this error message to indicate that |
||
); | ||
} | ||
} | ||
}); | ||
|
||
return await loader.get((AugmentVis as any) as Record<string, unknown>); | ||
}; |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.