Fix config stacking order #158827
Fix config stacking order #158827
Conversation
delanni
added
Team:Operations
|
Pinging @elastic/kibana-operations (Team:Operations) |
|
It looks like #158706 supersedes this PR. Doesn't it? |
@afharo - yes, good question. I think I should close #158706, as it's split into two separate PRs:
Both benefit from having the integration tests nearby, but ultimately different issues. I think now they can be merged in either order, I'll just need to resolve conflicts on the tests expectations. |
|
@elasticmachine merge upstream |
| 1. serverless.yml (serverless configs go first) | ||
| 2. serverless.{mode}.yml (serverless configs go first) | ||
| 3. base config, in this preference order: | ||
| - my-config.yml(s) (set by --config) | ||
| - env-config.yml (described by `env.KBN_CONFIG_PATHS`) | ||
| - kibana.yml (default @ `env.KBN_PATH_CONF`/kibana.yml) |
There was a problem hiding this comment.
cc @afharo given he knows the problem more than I do: Are we fine with changing this ordering?
There was a problem hiding this comment.
Just to add, this change reflects the status quo as it is on main now for (1, 2), and (3) is the one whose order is currently in regression, trying to be fixed.
main now:
/Users/alex/Git/kibana/config/serverless.yml
/Users/alex/Git/kibana/config/serverless.es.yml
/Users/alex/Git/kibana/config/kibana.yml
/Users/alex/Git/kibana/config/my-config.yml
/Users/alex/Git/kibana/config/serverless.recent.yml
/Users/alex/Git/kibana/config/kibana.dev.yml
/Users/alex/Git/kibana/config/serverless.recent.dev.yml
on this branch:
/Users/alex/Git/kibana/config/serverless.yml
/Users/alex/Git/kibana/config/serverless.es.yml
/Users/alex/Git/kibana/config/my-config.yml
/Users/alex/Git/kibana/config/serverless.recent.yml
/Users/alex/Git/kibana/config/kibana.dev.yml
/Users/alex/Git/kibana/config/serverless.recent.dev.yml
There was a problem hiding this comment.
@pgayvallet, thanks for the ping! Yes, @delanni and I discussed the correct order on Slack.
The reasoning for serverless.*yml files being loaded before kibana.yml is that serverless config files define the defaults for the Serverless offering. kibana.yml should be able to override those defaults if it's decided that way by the user/operator.
I would consider it a bad UX if the user sets my-config.choice: value and it's not reflected in the product (in case serverless was loaded after kibana.yml and overrode it) without any warning.
| // eslint-disable-next-line no-console | ||
| console.log('Configurations parsed in this order: ' + env.configs.join(', ')); |
There was a problem hiding this comment.
For the tests, I needed to output something to the logs.
I would expect this to be replaceable with unit tests of the proper sub component(s).
There was a problem hiding this comment.
it's a good idea, once it's one component, I could do a unit test for it
delanni
force-pushed
the
kib-155154-fix-config-order
branch
from
c46d9dc
to
1002538
Compare
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
|
Since this is a fix to a regression, we'd prefer not to include the refactors with this PR. This is going in first, fixing the regression, and getting backported. (if we want to include tests with this PR, they can only be integration for now) The followup is #158750 - it moves the config ordering to a module, so it can be tested with unit-tests (~1s instead of ~1m). |
delanni
added
backport:prev-minor
| 1. serverless.yml (serverless configs go first) | ||
| 2. serverless.{mode}.yml (serverless configs go first) | ||
| 3. base config, in this preference order: | ||
| - my-config.yml(s) (set by --config) | ||
| - env-config.yml (described by `env.KBN_CONFIG_PATHS`) | ||
| - kibana.yml (default @ `env.KBN_PATH_CONF`/kibana.yml) |
There was a problem hiding this comment.
@pgayvallet, thanks for the ping! Yes, @delanni and I discussed the correct order on Slack.
The reasoning for serverless.*yml files being loaded before kibana.yml is that serverless config files define the defaults for the Serverless offering. kibana.yml should be able to override those defaults if it's decided that way by the user/operator.
I would consider it a bad UX if the user sets my-config.choice: value and it's not reflected in the product (in case serverless was loaded after kibana.yml and overrode it) without any warning.
## Summary Fixes: elastic#155154 (introduced in elastic#149878), builds on elastic#155436 . - Adds tests to ensure the configuration merging order, check those for reference. - Updates the README to explain the intention For the tests, I needed to output something to the logs. I hope it's not a big issue to log it. If needed, I might hide that behind a verbose- or feature flag. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit c57589e)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.8`: - [Fix config stacking order (#158827)](#158827) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Alex Szabo","email":"alex.szabo@elastic.co"},"sourceCommit":{"committedDate":"2023-06-05T13:15:07Z","message":"Fix config stacking order (#158827)\n\n## Summary\r\nFixes: #155154 (introduced in #149878), builds on #155436 .\r\n\r\n- Adds tests to ensure the configuration merging order, check those for\r\nreference.\r\n- Updates the README to explain the intention\r\n \r\nFor the tests, I needed to output something to the logs. I hope it's not\r\na big issue to log it. If needed, I might hide that behind a verbose- or\r\nfeature flag.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"c57589ec57e5e8265a66cd9c8c2102005736f6d8","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Operations","release_note:fix","backport:prev-minor","v8.9.0"],"number":158827,"url":"#158827 config stacking order (#158827)\n\n## Summary\r\nFixes: #155154 (introduced in #149878), builds on #155436 .\r\n\r\n- Adds tests to ensure the configuration merging order, check those for\r\nreference.\r\n- Updates the README to explain the intention\r\n \r\nFor the tests, I needed to output something to the logs. I hope it's not\r\na big issue to log it. If needed, I might hide that behind a verbose- or\r\nfeature flag.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"c57589ec57e5e8265a66cd9c8c2102005736f6d8"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"#158827 config stacking order (#158827)\n\n## Summary\r\nFixes: #155154 (introduced in #149878), builds on #155436 .\r\n\r\n- Adds tests to ensure the configuration merging order, check those for\r\nreference.\r\n- Updates the README to explain the intention\r\n \r\nFor the tests, I needed to output something to the logs. I hope it's not\r\na big issue to log it. If needed, I might hide that behind a verbose- or\r\nfeature flag.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"c57589ec57e5e8265a66cd9c8c2102005736f6d8"}}]}] BACKPORT--> --------- Co-authored-by: Alex Szabo <alex.szabo@elastic.co>
## Summary Fixes: elastic#155154 (introduced in elastic#149878), builds on elastic#155436 . - Adds tests to ensure the configuration merging order, check those for reference. - Updates the README to explain the intention For the tests, I needed to output something to the logs. I hope it's not a big issue to log it. If needed, I might hide that behind a verbose- or feature flag. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
## Summary Fixes: elastic#155154 (introduced in elastic#149878), builds on elastic#155436 . - Adds tests to ensure the configuration merging order, check those for reference. - Updates the README to explain the intention For the tests, I needed to output something to the logs. I hope it's not a big issue to log it. If needed, I might hide that behind a verbose- or feature flag. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Fixes: #155154 (introduced in #149878), builds on #155436 .
For the tests, I needed to output something to the logs. I hope it's not a big issue to log it. If needed, I might hide that behind a verbose- or feature flag.
Checklist