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.

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

Deprecate kibana.index setting #83988

Merged
merged 10 commits into from
Nov 24, 2020
66 changes: 66 additions & 0 deletions src/core/server/kibana_config.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import { config } from './kibana_config';
import { applyDeprecations, configDeprecationFactory } from '@kbn/config';

const CONFIG_PATH = 'kibana';

const applyKibanaDeprecations = (settings: Record<string, any> = {}) => {
const deprecations = config.deprecations!(configDeprecationFactory);
const deprecationMessages: string[] = [];
const _config: any = {};
_config[CONFIG_PATH] = settings;
const migrated = applyDeprecations(
_config,
deprecations.map((deprecation) => ({
deprecation,
path: CONFIG_PATH,
})),
(msg) => deprecationMessages.push(msg)
);
return {
messages: deprecationMessages,
migrated,
};
};

it('set correct defaults ', () => {
const configValue = config.schema.validate({});
expect(configValue).toMatchInlineSnapshot(`
Object {
"autocompleteTerminateAfter": "PT1M40S",
"autocompleteTimeout": "PT1S",
"enabled": true,
"index": ".kibana",
}
`);
});

describe('deprecations', () => {
['.foo', '.kibana'].forEach((index) => {
it('logs a warning if index is set', () => {
const { messages } = applyKibanaDeprecations({ index });
expect(messages).toMatchInlineSnapshot(`
Array [
"Setting [kibana.index] is deprecated. Multitenancy by changing 'kibana.index' will not be supported starting in 8.0. See https://ela.st/kbn-remove-legacy-multitenancy for more details",
]
`);
});
});
});
14 changes: 14 additions & 0 deletions src/core/server/kibana_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,22 @@
*/

import { schema, TypeOf } from '@kbn/config-schema';
import { ConfigDeprecationProvider } from '@kbn/config';

export type KibanaConfigType = TypeOf<typeof config.schema>;

const deprecations: ConfigDeprecationProvider = () => [
(settings, fromPath, log) => {
const kibana = settings[fromPath];
if (kibana?.index) {
log(
`Setting [kibana.index] is deprecated. Multitenancy by changing 'kibana.index' will not be supported starting in 8.0. See https://ela.st/kbn-remove-legacy-multitenancy for more details`
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remark: There is an inconsistency with the wording of our unused factory:

log(`${fullPath} is deprecated and is no longer used`);

Setting [{config prop}] is deprecated vs {config prop} is deprecated.

I also see that rename is using double quote around the config prop (where unused does not).

log(`"${fullOldPath}" is deprecated and has been replaced by "${fullNewPath}"`);

This is minor, but we might want to be consistent in our deprecation messages.

Also thinking: we may want to adapt ConfigDeprecationFactory.unused (and others) to accept a new optional 'message' parameter. That would also to log custom messages in deprecations without having to recode a custom one just to log the message (as it is done here for kibana.index).

unused('kibana.index', "Multitenancy by changing 'kibana.index' will not be supported starting in 8.0. See https://ela.st/kbn-remove-legacy-multitenancy for more details")

This is probably out of the scope of this PR though. Should I create a separate issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is minor, but we might want to be consistent in our deprecation messages.

Agreed about being consistent here. I was originally replicating the format from the following deprecation log message, because I incorrectly assumed it used the "standard approach": [warning][config][deprecation] Setting [elasticsearch.username] to "kibana" is deprecated. You should use the "kibana_system" user instead.

Do we want to follow the precedent set by unused and not use double-quotes, or should we follow the precedent set by rename and use double-quotes?

Also thinking: we may want to adapt ConfigDeprecationFactory.unused (and others) to accept a new optional 'message' parameter. That would also to log custom messages in deprecations without having to recode a custom one just to log the message (as it is done here for kibana.index).

I think it's fine to support custom messages for unused/rename. However, I don't think we should be using the unused deprecation here, as this setting is still being used and has an effect, it's just deprecated. We use the unused deprecation elsewhere when settings don't have an effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to follow the precedent set by unused and not use double-quotes, or should we follow the precedent set by rename and use double-quotes?

I think using quotes is slightly better, wdyt?

I think it's fine to support custom messages for unused/rename. However, I don't think we should be using the unused deprecation here, as this setting is still being used and has an effect, it's just deprecated.

Yea, you're right. I forgot we are still using the value of this property. Nevermind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double-quotes sound good to me! I just independently decided that also and used it in 9dc7532

}
return settings;
},
];

export const config = {
path: 'kibana',
schema: schema.object({
Expand All @@ -29,4 +42,5 @@ export const config = {
autocompleteTerminateAfter: schema.duration({ defaultValue: 100000 }),
autocompleteTimeout: schema.duration({ defaultValue: 1000 }),
}),
deprecations,
};