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.

Sign up for GitHub

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

Jump to bottom

Hides unknown uiSettings from the advanced settings page #128030

Merged
merged 7 commits into from
Mar 21, 2022
Merged

Hides unknown uiSettings from the advanced settings page #128030

merged 7 commits into from
Mar 21, 2022

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Mar 18, 2022 •
edited
Loading

Resolves #94876

Settings that aren't registered by plugins are visible and editable in Stack Management > Kibana > Advanced Settings > General. We need to hide these from the UI without removing them from the underlying saved object. Hiding custom settings will also hide any settings registered by a plugin that's been disabled (e.g Reporting).

Rather than introduce another flag, we reuse the isCustom property.

In this PR,

  • this PR filters out settings for disabled plugins by using the ui_settings client isCustom flag to decide on when to show a setting or not.
  • we’re effectively ignoring previously set items that were for a plugin that is now disabled by filtering these out from the AdvancedSettings page.

This PR doesn't modify the underlying uiSettings saved object directly, because that should be done using an explicit migration.

How to test this:

  • Run Kibana with a trial license with all plugins enabled
  • Navigate to Advanced Settings and adjust the settings for any of the plugins that may still be disabled. For example, add a custom pdf logo for reporting.
  • Stop Kibana but keep elasticsearch running (retains previous settings in the config saved object)
  • Disable any of the plugins that can be disabled, for example, to disable reporting:
xpack.reporting.enabled: false
xpack.reporting.queue.pollEnabled: false
  • Start Kibana again
  • Navigate to Advanced Settings and search for any setting applicable to the plugin(s) you’ve disabled.
  • Observe that the UI doesn’t render unregistered settings (even though they are in the config).
  • You can confirm that the saved object still exists and is persisted by checking that settings for the disabled plugin are still in the config document. For example, with the reporting test setup:
GET .kibana/_search
{
  "query": {
    "bool": {
      "must": [
        {
          "exists": {
            "field": "config"
          }
        }
      ]
    }
  }
}

Should return a document similar to:

"config" : {
   "buildNum" : 9007199254740991,
   "xpackReporting:customPdfLogo" : data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAUAAAAFACA……
}

The following plugins can still be disabled:

  • interactive_setup
  • newsfeed
  • reporting
  • telemetry
  • vis_type_* plugins

Checklist

  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

For maintainers

TinaHeiligers
TinaHeiligers commented Mar 18, 2022
View reviewed changes
src/plugins/advanced_settings/public/management_app/advanced_settings.tsx
@@ -198,6 +198,7 @@ export class AdvancedSettings extends Component<AdvancedSettingsProps, AdvancedS
});
})
.filter((c) => !c.readOnly)
.filter((c) => !c.isCustom) // hide any settings that aren't explicitly registered by enabled plugins.
Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We already have a flag that we can use to filter out any settings that aren't explicitly registered during Kibana setup and start lifecycles and don't need to necessarily add another one.

TinaHeiligers
TinaHeiligers commented Mar 19, 2022
View reviewed changes
Copy link
Contributor Author

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

self review

src/plugins/advanced_settings/public/management_app/advanced_settings.test.tsx Outdated
@@ -268,6 +268,33 @@ describe('AdvancedSettings', () => {
).toHaveLength(1);
});

it('should should not render a custom setting', async () => {
// For various complicated reasons the mock returns false for isConfig, override that
Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@pgayvallet the whole advanced_settings.test.ts needs some serious rework. The test suite hasn't seen much "love" for a while and hacks around implementation within the AdvancedSettings component.
I thought of refactoring this test but we will need to do that later as part of the Kibana profile initiative and chose to leave it as is right now to get this bug fix in.

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers changed the title WIP Hides unregistered uiSettings in the AS page Hides unknown uiSettings from the advanced settings page Mar 19, 2022
@TinaHeiligers TinaHeiligers added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.2.0 labels Mar 19, 2022
@TinaHeiligers TinaHeiligers marked this pull request as ready for review March 19, 2022 16:53
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner March 19, 2022 16:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Bamieh
Bamieh approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@Bamieh Bamieh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I believe reusing the isCustom property makes sense

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers enabled auto-merge (squash) March 21, 2022 14:53
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
advancedSettings 40.6KB 40.6KB +25.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers merged commit 35e3dd0 into elastic:main Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bamieh Bamieh Bamieh approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Advanced Settings] hide unknown uiSettings from the UI
5 participants
@TinaHeiligers @elasticmachine @kibana-ci @Bamieh @kibanamachine