Fixes broken app when management is turned off #4891
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
ashwin-pc
requested review from
ananzh,
kavilla,
seanneumann,
AMoo-Miki,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
kristenTian,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
BSFishy and
curq
as code owners
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
ashwin-pc
changed the title
Codecov Report
@@ Coverage Diff @@
## main #4891 +/- ##
==========================================
- Coverage 66.39% 66.39% -0.01%
==========================================
Files 3397 3397
Lines 64804 64806 +2
Branches 10360 10361 +1
==========================================
+ Hits 43026 43027 +1
- Misses 19218 19219 +1
Partials 2560 2560
Flags with carried forward coverage won't be shown. Click here to find out more.
|
lezzago
approved these changes
Sep 1, 2023
kavilla
approved these changes
Sep 1, 2023
AMoo-Miki
reviewed
Sep 1, 2023
Comment on lines
+78
to
+80
| try { | ||
| uiActions.addTriggerAction(CONTEXT_MENU_TRIGGER, viewEventsOptionAction); | ||
| uiActions.addTriggerAction(SAVED_OBJECT_DELETE_TRIGGER, savedObjectDeleteAction); |
There was a problem hiding this comment.
wouldn't it be safer to wrap them individually? That way, a failure, due to one plugin being disabled, will not prevent other listeners from being added. Maybe even throw the triggers and the callbacks into an array and then loop through them and add the trigger, wrapped in a try-catch.
There was a problem hiding this comment.
Yeah with the current behaviour this is fine. The context menu trigger comes from the embeddables plugin which is not something we disable (Dashboards would not work without it). So in theory the only real action that cannot be registered is the savedObject Management trigger, which is already the second action in the try block.
joshuarrrr
reviewed
Sep 1, 2023
| uiActions.addTriggerAction(PLUGIN_RESOURCE_DELETE_TRIGGER, pluginResourceDeleteAction); | ||
| uiActions.addTriggerAction(EXTERNAL_ACTION_TRIGGER, openEventsFlyoutAction); | ||
|
|
||
| // These triggers are registered by other plugins. If they are disabled can throw an error. |
There was a problem hiding this comment.
Suggested change
| // These triggers are registered by other plugins. If they are disabled can throw an error. | |
| // These triggers are registered by other plugins. If they are disabled, adding an action can throw an error. |
Comment on lines
+78
to
+80
| try { | ||
| uiActions.addTriggerAction(CONTEXT_MENU_TRIGGER, viewEventsOptionAction); | ||
| uiActions.addTriggerAction(SAVED_OBJECT_DELETE_TRIGGER, savedObjectDeleteAction); |
joshuarrrr
approved these changes
Sep 1, 2023
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Sep 1, 2023
* fixes broken app when management is turned off Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * updates changelog Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit 1068939) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
AMoo-Miki
pushed a commit
that referenced
this pull request
Sep 5, 2023
* fixes broken app when management is turned off Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> * updates changelog Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit 1068939) 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Then
management.enabled: falseis set in the config, the app now correctly loads without breaking.Issues Resolved
fixes #4671
Screenshot
Testing the changes
set
management.enabled: falsein the config file and startup OSD. All the pages should be working while the management pages are disabled.Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr