fix bug: management section id not match the key defined in capabilities #6421

ruanyl · 2024-04-12T02:28:14Z

Description

The Dashboards management section id(opensearch-dashboards) of Management category doesn't match the id(opensearchDashboards) defined in capabilities, the consequence is the following code will never work for apps of Dashboards management. Even set the capability of the app to false, the nav link still displayed on the UI(it does redirect to home page when accessing the link).

    this.getAllSections().forEach((section) => {
      if (capabilities.management.hasOwnProperty(section.id)) {
        const sectionCapabilities = capabilities.management[section.id];
        section.apps.forEach((app) => {
          if (sectionCapabilities.hasOwnProperty(app.id) && sectionCapabilities[app.id] !== true) {
            app.disable();
          }
        });
      }
    });

The original PR introduced the id change is #260
@kavilla do you have more context of changing id from opensearchDashboards to opensearch-dashboards?

Changelog

fix: management section id not match the key defined in capabilities

Issues Resolved

Screenshot

Testing the changes

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

codecov · 2024-04-15T02:03:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.96%. Comparing base (376ead0) to head (da608d3).
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6421      +/-   ##
==========================================
- Coverage   67.72%   63.96%   -3.77%     
==========================================
  Files        3518     3598      +80     
  Lines       69638    78266    +8628     
  Branches    11365    12340     +975     
==========================================
+ Hits        47165    50061    +2896     
- Misses      19684    25185    +5501     
- Partials     2789     3020     +231

Flag	Coverage Δ
Linux_1	`31.07% <ø> (-2.09%)`	⬇️
Linux_2	`55.53% <ø> (+0.06%)`	⬆️
Linux_3	`41.30% <ø> (-2.02%)`	⬇️
Linux_4	`31.54% <100.00%> (-3.15%)`	⬇️
Windows_1	`31.09% <ø> (-2.12%)`	⬇️
Windows_2	`55.48% <ø> (+0.06%)`	⬆️
Windows_3	`41.31% <ø> (-2.03%)`	⬇️
Windows_4	`31.54% <100.00%> (-3.15%)`	⬇️

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.

manasvinibs · 2024-05-22T18:42:58Z

src/plugins/management/public/management_sections_service.ts

@@ -94,8 +98,9 @@ export class ManagementSectionsService {

  start({ capabilities }: SectionsServiceStartDeps) {
    this.getAllSections().forEach((section) => {
-      if (capabilities.management.hasOwnProperty(section.id)) {
-        const sectionCapabilities = capabilities.management[section.id];
+      const capabilityId = MANAGEMENT_ID_TO_CAPABILITIES[section.id] || section.id;


@ruanyl Just curious to know why don't we instead update the Id in the Capabilities to opensearch-dashboards instead of mapping it here?

@manasvinibs Good question, that's for compatibility, opensearch-dashboards maybe deep linked, for example, in function test repo.

@kavilla Is there a way to estimate the impact of changing capabilities key from opensearchDashboards to opensearch-dashboards in src/plugins/management/server/capabilities_provider.ts?

I've seen some places in OSD core uses capabilities.management.opensearchDashboards, also there might other plugins are already using this capability key, changing this key would be a breaking change, so I think it would be better to do key mapping. What do you think? @manasvinibs

virajsanghvi · 2024-05-24T16:37:05Z

src/plugins/management/public/management_sections_service.ts

@@ -94,8 +98,9 @@ export class ManagementSectionsService {

  start({ capabilities }: SectionsServiceStartDeps) {
    this.getAllSections().forEach((section) => {
-      if (capabilities.management.hasOwnProperty(section.id)) {
-        const sectionCapabilities = capabilities.management[section.id];
+      const capabilityId = MANAGEMENT_ID_TO_CAPABILITIES[section.id] || section.id;


Is there an associated test to validate this is working correctly?

@virajsanghvi unit test added

BionIT · 2024-06-05T03:29:04Z

Hi @ruanyl , are we targeting this PR for 2.15 or 2.16?

ruanyl · 2024-06-05T16:16:54Z

Hi @ruanyl , are we targeting this PR for 2.15 or 2.16?

Maybe 2.16, haven't got a chance to add unit tests

Signed-off-by: Yulong Ruan <[email protected]>

ruanyl · 2024-07-21T12:42:01Z

@virajsanghvi @manasvinibs Could you please take another look at the PR?

virajsanghvi · 2024-07-22T23:40:17Z

src/plugins/management/public/management_sections_service.test.ts

+    const originalDataSourcesCapability =
+      DEFAULT_MANAGEMENT_CAPABILITIES.management.opensearchDashboards.dataSources;


nit: ideally we could mock this over setting it globally, but you do clean it up so not blocking.

virajsanghvi · 2024-07-22T23:43:41Z

src/plugins/management/public/management_sections_service.test.ts

@@ -105,4 +106,29 @@ describe('ManagementService', () => {
      ]
    `);
  });
+
+  it('should disable apps register in opensearchDashboards section', () => {


Could you help explain how this is testing the "opensearch-dashboards" translation code?

In the capability provider, the capability key is opensearchDashboards, when registering the default section opensearchDashboards: this.registerSection(OpenSearchDashboardsSection), the section id is defined as opensearch-dashboards and this id was used to retrieve the app status register with capability key opensearchDashboards.

So we need to do the mapping from opensearch-dashboards -> opensearchDashboards

Signed-off-by: Yulong Ruan <[email protected]>

…ies (#6421) * fix management section id not match the key defined in capabilities Signed-off-by: Yulong Ruan <[email protected]> * add descriptions on how the PR solved the problme Signed-off-by: Yulong Ruan <[email protected]> --------- Signed-off-by: Yulong Ruan <[email protected]> (cherry picked from commit a65a8aa) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…ies (#6421) (#7634) * fix management section id not match the key defined in capabilities * add descriptions on how the PR solved the problme --------- (cherry picked from commit a65a8aa) Signed-off-by: Yulong Ruan <[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>

ruanyl requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, BionIT and xinruiba as code owners April 12, 2024 02:28

github-actions bot added the repeat-contributor label Apr 12, 2024

ruanyl added bug Something isn't working and removed bug Something isn't working labels Apr 12, 2024

ruanyl force-pushed the fix-management-section-id-not-match-capabilities branch from 81ce8d3 to ca218bb Compare April 12, 2024 10:14

manasvinibs reviewed May 22, 2024

View reviewed changes

ruanyl requested review from zhyuanqi and mengweieric as code owners May 24, 2024 07:34

opensearch-changeset-bot bot added a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request May 24, 2024

Changeset file for PR opensearch-project#6421 created/updated

6e8d9ad

virajsanghvi reviewed May 24, 2024

View reviewed changes

yujin-emma assigned manasvinibs May 28, 2024

BionIT added the v2.16.0 label Jun 5, 2024

fix management section id not match the key defined in capabilities

ea9f4ea

Signed-off-by: Yulong Ruan <[email protected]>

ruanyl force-pushed the fix-management-section-id-not-match-capabilities branch from 374a6dc to ea9f4ea Compare July 21, 2024 12:31

ruanyl requested a review from LDrago27 as a code owner July 21, 2024 12:31

ruanyl added the backport 2.x label Jul 21, 2024

virajsanghvi reviewed Jul 22, 2024

View reviewed changes

add descriptions on how the PR solved the problme

da608d3

Signed-off-by: Yulong Ruan <[email protected]>

virajsanghvi approved these changes Jul 23, 2024

View reviewed changes

ruanyl added backport 2.16 v2.17.0 and removed v2.16.0 backport 2.16 labels Jul 23, 2024

SuZhou-Joe approved these changes Aug 7, 2024

View reviewed changes

ruanyl merged commit a65a8aa into opensearch-project:main Aug 7, 2024
70 of 71 checks passed

opensearch-trigger-bot bot mentioned this pull request Aug 7, 2024

[Backport 2.x] fix bug: management section id not match the key defined in capabilities #7634

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bug: management section id not match the key defined in capabilities #6421

fix bug: management section id not match the key defined in capabilities #6421

ruanyl commented Apr 12, 2024 •

edited

Loading

codecov bot commented Apr 15, 2024 •

edited

Loading

manasvinibs May 22, 2024

ruanyl May 23, 2024

ruanyl Jul 8, 2024

ruanyl Jul 21, 2024 •

edited

Loading

virajsanghvi May 24, 2024

manasvinibs Jun 4, 2024

ruanyl Jul 21, 2024

BionIT commented Jun 5, 2024

ruanyl commented Jun 5, 2024

ruanyl commented Jul 21, 2024

virajsanghvi Jul 22, 2024

virajsanghvi Jul 22, 2024

ruanyl Jul 23, 2024

		const originalDataSourcesCapability =
		DEFAULT_MANAGEMENT_CAPABILITIES.management.opensearchDashboards.dataSources;

fix bug: management section id not match the key defined in capabilities #6421

fix bug: management section id not match the key defined in capabilities #6421

Conversation

ruanyl commented Apr 12, 2024 • edited Loading

Description

Changelog

Issues Resolved

Screenshot

Testing the changes

Check List

codecov bot commented Apr 15, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BionIT commented Jun 5, 2024

ruanyl commented Jun 5, 2024

ruanyl commented Jul 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl commented Apr 12, 2024 •

edited

Loading

codecov bot commented Apr 15, 2024 •

edited

Loading

ruanyl Jul 21, 2024 •

edited

Loading