Kibana property config migrations #55937

nickofthyme · 2020-01-24T23:56:03Z

Summary

Related issue #54497

Move global kibana config properties/remove `injectedMetadata` usage

Move kibana.defaultAppId to kibana_legacy plugin
Move kibana.disableWelcomeScreen to home.disableWelcomeScreen in `Home plugin.

Core server API changes Updates

ConfigDeprecationFactory#renameFromRoot now allows silent as the third argument to mute depreciation logging.

Checklist

This was checked for cross-browser compatibility, including a check against IE11
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

nickofthyme · 2020-01-25T00:05:42Z

@flash1293 I noticed that the configService prevents duplicate schema configs per path.

Since kibana is already defined as a global config here 👇

kibana/src/core/server/kibana_config.ts

Lines 24 to 34 in 703a3c2

    
           export const config = { 
        
             path: 'kibana', 
        
             schema: schema.object({ 
        
               enabled: schema.boolean({ defaultValue: true }), 
        
               defaultAppId: schema.string({ defaultValue: 'home' }), 
        
               index: schema.string({ defaultValue: '.kibana' }), 
        
               disableWelcomeScreen: schema.boolean({ defaultValue: false }), 
        
               autocompleteTerminateAfter: schema.duration({ defaultValue: 100000 }), 
        
               autocompleteTimeout: schema.duration({ defaultValue: 1000 }), 
        
             }), 
        
           };

I can't define the "configPath": "kibana", in kibana_legacy/kibana.json and there doesn't appear to be a way to alias the name. So should I remove all the kibana.* config and put them in kibana_legacy?

I also wasn't sure how this config below was different than the one above. Seem to like like a duplicate schema.

kibana/src/legacy/core_plugins/kibana/index.js

Lines 41 to 59 in 703a3c2

    
           return new kibana.Plugin({ 
        
             id: 'kibana', 
        
             config: function(Joi) { 
        
               return Joi.object({ 
        
                 enabled: Joi.boolean().default(true), 
        
                 defaultAppId: Joi.string().default('home'), 
        
                 index: Joi.string().default('.kibana'), 
        
                 disableWelcomeScreen: Joi.boolean().default(false), 
        
                 autocompleteTerminateAfter: Joi.number() 
        
                   .integer() 
        
                   .min(1) 
        
                   .default(100000), 
        
                 // TODO Also allow units here like in elasticsearch config once this is moved to the new platform 
        
                 autocompleteTimeout: Joi.number() 
        
                   .integer() 
        
                   .min(1) 
        
                   .default(1000), 
        
               }).default(); 
        
             },

flash1293 · 2020-01-27T13:40:03Z

@nickofthyme

I noticed that the configService prevents duplicate schema configs per path.

Very good point, I think you can use a deprecation helper though to rename the config key, right? I think you can't use the default renameRoot one because it will emit a log message which we should avoid in this case, but you can also write your own.

I also wasn't sure how this config below was different than the one above. Seem to like like a duplicate schema.

Good catch, didn't notice that. They should definitely stay in sync (so if you are moving out something to the new platform, remove it from both)

Questionable tasks
Move kibana.autocompleteTerminateAfter to kibana_legacy plugin
Move kibana. autocompleteTimeout to kibana_legacy plugin
Remove all kibana global config properties. (i.e. defaultAppId and index)

Those can be omitted for this PR. There is also some discussion about those going on in #46705

flash1293

LGTM, just left a minor comment.

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

flash1293 · 2020-01-31T15:34:29Z

src/plugins/kibana_legacy/server/index.ts

+  schema: configSchema,
+  deprecations: ({ renameFromRoot }) => [
+    // TODO: Remove deprecation once defaultAppId is deleted
+    renameFromRoot('kibana.defaultAppId', 'kibana_legacy.defaultAppId'),


This triggers a warning in the server logs when kibana.defaultAppId is used:

log [16:23:34.047] [warning][config][deprecation] "kibana.defaultAppId" is deprecated and has been replaced by "kibana_legacy.defaultAppId"

Since we are going to sunset the whole config key soon it doesn't really help when users are prompted to change the setting now.

It seems like an easy way to do this is to no pass down the logger function to the helper:

[ // TODO: Remove deprecation once defaultAppId is deleted (config, rootPath) => renameFromRoot('kibana.defaultAppId', 'kibana_legacy.defaultAppId')(config, rootPath, /* don't log this deprecation because it won't stay long*/ () => {}),

I was suggesting to copy the renameFromRoot code to ignore the logger, as this was temporary. but the suggestion may be better

flash1293 · 2020-01-31T15:41:16Z

@joshdover To give a bit more context on this change - defaultAppId will probably be removed with 8.0 anyway, but this change moves it into the new platform and out of the global injected vars to

Make it explicit who owns this setting
Use regular NP apis to not block migration
In case we decide to keep this alive for backwards compatibility even after removing legacy platform in 7.8

joshdover

Platform changes and config deprecations LGTM

pgayvallet

LGTM when the kibana_legacy pluginId is renamed to kibanaLegacy

pgayvallet · 2020-02-03T06:58:56Z

src/plugins/kibana_legacy/kibana.json

@@ -1,6 +1,6 @@
 {
  "id": "kibana_legacy",


Even if we did not migrate existing plugin's ids when doing #52190, pluginId should be camelCased. This should be kibanaLegacy

This also means changing calls such as
const { config } = npSetup.plugins.kibana_legacy;
to
const { config } = npSetup.plugins.kibanaLegacy;

Note: this will not change the config path, which default to snakeCase(id) for convention with the stack's naming, so the property access / deprecations don't need any changes

We have a separate issue tracking this (#55363) - @maryia-lapata is already working on it.

Ok, as long as it's tracked, LGTM.

nickofthyme · 2020-02-03T14:28:52Z

@pgayvallet are you ok with changes in e42ac5e like we discussed?

pgayvallet · 2020-02-03T14:55:49Z

@nickofthyme lgtm

* Move defaultAppId config param into kibanaLegacy * Move disableWelcomeScreen config param into Home plugin * Update api and docs with silent option for renameFromRoot

kibanamachine · 2020-02-04T05:36:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b47b428

History

💚 Build #24054 succeeded b47b428
💔 Build #24018 failed b5aeb34
💚 Build #23646 succeeded eeac1da
💔 Build #23519 failed c528e4a
💔 Build #23426 failed b31a688

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

* Move defaultAppId config param into kibanaLegacy * Move disableWelcomeScreen config param into Home plugin * Update api and docs with silent option for renameFromRoot

* master: (42 commits) Move kuery_autocomplete ⇒ NP (elastic#56607) [ML] Functional tests - stabilize job row and analytics result view assertions (elastic#56595) [Discover] Inline angular directives only used in this plugin (elastic#56119) [Discover] Migrate get_sort.js test from mocha to TypeScript (elastic#56011) [SIEM] Enable flow_target_select_connected unit tests (elastic#55618) Start consuming np logging config (elastic#56480) [SIEM] Add eslint-plugin-react-perf (elastic#55960) Mention changed SAML ACS endpoint URL in breaking changes doc. (elastic#56613) Add `getServerInfo` API to http setup contract (elastic#56636) Updates Monitoring alert Jest snapshots Kibana property config migrations (elastic#55937) Vislib replacement toggle (elastic#56439) [Uptime] Add unit tests for QueryContext time calculation (elastic#56671) [SIEM][Detection Engine] Critical blocker, fixes pre-packaged rule miscounts Upgrade EUI to v18.3.0 (elastic#56228) [Maps] Fix server log (elastic#56679) [SIEM] Fixes FTUE when APM node is present (elastic#56574) [Reporting/FieldFormats] expose `setFieldFormats` and call from ReportingPlugin.start (elastic#56563) Update EMS API urls for production (elastic#56657) Ability to delete alerts even when AAD is out of sync (elastic#56543) ...

* Move defaultAppId config param into kibanaLegacy * Move disableWelcomeScreen config param into Home plugin * Update api and docs with silent option for renameFromRoot

nickofthyme added 2 commits January 24, 2020 15:16

Move defaultAppId config param into kibanaLegacy

d21a532

Move disableWelcomeScreen config param into Home plugin

82256a8

flash1293 mentioned this pull request Jan 27, 2020

[New Platform] 8.0 Refactor kibana.yml #46705

Closed

nickofthyme changed the title ~~Kpm/config change~~ Kibana config migration Jan 27, 2020

nickofthyme added 10 commits January 28, 2020 14:43

Update kibana config with renaming deprecation

0da7b43

Fix type errors

ea27169

Fix karma test failures

4f8b305

Merge branch 'master' into kpm/config-change

3483f2c

Add karma config mock for home

d0675a9

Fix jest test error

c706f2f

Merge branch 'master' into kpm/config-change

3d70732

Merge branch 'master' into kpm/config-change

b31a688

Fix config.get errors in functional tests

c528e4a

Merge branch 'master' into kpm/config-change

eeac1da

nickofthyme requested review from a team January 30, 2020 21:50

nickofthyme assigned flash1293 and joshdover and unassigned flash1293 and joshdover Jan 30, 2020

nickofthyme requested review from joshdover and flash1293 January 30, 2020 21:50

nickofthyme added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Jan 30, 2020

nickofthyme marked this pull request as ready for review January 30, 2020 23:14

flash1293 approved these changes Jan 31, 2020

View reviewed changes

joshdover approved these changes Jan 31, 2020

View reviewed changes

pgayvallet approved these changes Feb 3, 2020

View reviewed changes

nickofthyme added 2 commits February 3, 2020 08:25

Address PR comments

e42ac5e

Merge branch 'master' into kpm/config-change

b5aeb34

nickofthyme added 2 commits February 3, 2020 09:33

Update api docs

d66c4e0

Merge branch 'master' into kpm/config-change

b47b428

nickofthyme requested a review from a team as a code owner February 3, 2020 15:59

nickofthyme merged commit 186a826 into elastic:master Feb 4, 2020

nickofthyme deleted the kpm/config-change branch February 4, 2020 04:17

nickofthyme changed the title ~~Kibana config migration~~ Kibana property config migrations Feb 4, 2020

flash1293 mentioned this pull request Feb 4, 2020

Replace injected vars with exposeToBrowser #54497

Closed

nickofthyme mentioned this pull request Feb 4, 2020

[7.x] Kibana property config migrations (#55937) #56720

Merged

mistic mentioned this pull request Feb 7, 2020

fix(NA): MaxListenersExceededWarning on getLoggerStream #57133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kibana property config migrations #55937

Kibana property config migrations #55937

nickofthyme commented Jan 24, 2020 •

edited

Loading

nickofthyme commented Jan 25, 2020

flash1293 commented Jan 27, 2020 •

edited

Loading

flash1293 left a comment

flash1293 Jan 31, 2020

pgayvallet Feb 3, 2020

flash1293 commented Jan 31, 2020 •

edited

Loading

joshdover left a comment

pgayvallet left a comment

pgayvallet Feb 3, 2020

flash1293 Feb 3, 2020

pgayvallet Feb 3, 2020

nickofthyme commented Feb 3, 2020

pgayvallet commented Feb 3, 2020

kibanamachine commented Feb 4, 2020

Kibana property config migrations #55937

Kibana property config migrations #55937

Conversation

nickofthyme commented Jan 24, 2020 • edited Loading

Summary

Move global kibana config properties/remove injectedMetadata usage

Core server API changes Updates

Checklist

For maintainers

nickofthyme commented Jan 25, 2020

flash1293 commented Jan 27, 2020 • edited Loading

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Jan 31, 2020

Choose a reason for hiding this comment

pgayvallet Feb 3, 2020

Choose a reason for hiding this comment

flash1293 commented Jan 31, 2020 • edited Loading

joshdover left a comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet Feb 3, 2020

Choose a reason for hiding this comment

flash1293 Feb 3, 2020

Choose a reason for hiding this comment

pgayvallet Feb 3, 2020

Choose a reason for hiding this comment

nickofthyme commented Feb 3, 2020

pgayvallet commented Feb 3, 2020

kibanamachine commented Feb 4, 2020

💚 Build Succeeded

History

nickofthyme commented Jan 24, 2020 •

edited

Loading

Move global kibana config properties/remove `injectedMetadata` usage

flash1293 commented Jan 27, 2020 •

edited

Loading

flash1293 commented Jan 31, 2020 •

edited

Loading