Do not mutate config in place during deprecations #99629
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
mshustov
added
Team:Core
mshustov
commented
May 10, 2021
| deprecations.forEach(({ deprecation, path }) => { | ||
| processed = deprecation(processed, path, createAddDeprecation(path)); | ||
| const commands = deprecation(result, path, createAddDeprecation(path)); |
|
Pinging @elastic/kibana-core (Team:Core) |
mikecote
approved these changes
May 10, 2021
legrego
approved these changes
May 10, 2021
mshustov
commented
May 10, 2021
pgayvallet
approved these changes
May 10, 2021
Comment on lines
31
to
33
| commands.set.forEach(function ({ key, value }) { | ||
| set(result, key, value); | ||
| }); |
There was a problem hiding this comment.
Any reason to not use arrow func here?
There was a problem hiding this comment.
nope, but prefer-arrow-callback eslint is not enabled in the repo
flash1293
approved these changes
May 11, 2021
Bamieh
approved these changes
May 11, 2021
| deprecations.forEach(({ deprecation, path }) => { | ||
| processed = deprecation(processed, path, createAddDeprecation(path)); | ||
| const commands = deprecation(result, path, createAddDeprecation(path)); |
There was a problem hiding this comment.
@mshustov for the telemetry collector we'd only need the deprecated config keys
| processed = deprecation(processed, path, createAddDeprecation(path)); | ||
| const commands = deprecation(result, path, createAddDeprecation(path)); | ||
| if (commands) { | ||
| if (commands.set) { |
There was a problem hiding this comment.
why not use commands?.set instead of the two if statements?
There was a problem hiding this comment.
ok, I will change in a follow-up
mshustov
added
the
auto-backport
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
May 11, 2021
* refactor config deprecations to avoid config mutations * remove dynamic access keys in core deprecations. * refactor custom config deprecations to match the new signature * improve config deprecations fixtures for nested keys * add a test for xpack.banner config deprecations * key --> path, add a test for invalid command
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
kibanamachine
added a commit
that referenced
this pull request
May 11, 2021
) * Do not mutate config in place during deprecations (#99629) * refactor config deprecations to avoid config mutations * remove dynamic access keys in core deprecations. * refactor custom config deprecations to match the new signature * improve config deprecations fixtures for nested keys * add a test for xpack.banner config deprecations * key --> path, add a test for invalid command * remove lodash usage Co-authored-by: Mikhail Shustov <restrry@gmail.com>
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/infra/metrics_anomalies·ts.InfraOps App Metrics UI Metrics UI Anomaly Flyout with anomalies present renders more anomalies on threshold changeStandard OutStack TraceKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/ml/alert_flyout·ts.ML app anomaly detection alert overview page alert flyout controls can create an anomaly detection alertStandard OutStack TraceKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_lifecycle_management/policies·js.apis management index lifecycle management policies list should have a default policy to manage the Watcher history indicesStandard OutStack Traceand 5 more failures, only showing the first 3. Metrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
Summary
Part of #97791
Plugin API Changes
In this PR I changed how deprecations alter
config. When a plugin implements a custom deprecation function, it used to mutate the config object. From now on, it should return eithersetcommand to extendconfigorunsetcommand to removeconfigproperty. see Command patternbefore
after
Note that
@kbn/configdoesn't enforceconfigruntime immutability, but only compile-time check. It's done to prevent cases when a deprecation depends on another deprecation to be executed before, unfortunately, our test coverage is not enough to detect such cases. We have to do that manually in a follow-up if we think it's worth the effort.Checklist
Delete any items that are not applicable to this PR.