Skip to content

Fix config stacking order - #158827

Merged
delanni merged 7 commits into
elastic:mainfrom
delanni:kib-155154-fix-config-order
Jun 5, 2023
Merged

delanni merged 7 commits into
elastic:mainfrom
delanni:kib-155154-fix-config-order

Conversation

@delanni

@delanni delanni commented Jun 1, 2023 •

Copy link
Copy Markdown
Member

Summary

Fixes: #155154 (introduced in #149878), builds on #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

@delanni
delanni requested a review from a team as a code owner June 1, 2023 13:50
@delanni delanni added Team:Operations Kibana-Operations Team release_note:fix backport:skip This PR does not require backporting labels Jun 1, 2023
@elasticmachine

Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@afharo

afharo commented Jun 1, 2023

Copy link
Copy Markdown
Member

It looks like #158706 supersedes this PR. Doesn't it?

@delanni

delanni commented Jun 2, 2023

Copy link
Copy Markdown
Member Author

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.

@delanni delanni mentioned this pull request Jun 2, 2023
1 task
@delanni

delanni commented Jun 2, 2023

Copy link
Copy Markdown
Member Author

@elasticmachine merge upstream

Comment thread config/README.md
Comment on lines +8 to +13
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cc @afharo given he knows the problem more than I do: Are we fine with changing this ordering?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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.

Comment on lines +82 to +83
// eslint-disable-next-line no-console
console.log('Configurations parsed in this order: ' + env.configs.join(', '));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

@delanni delanni Jun 2, 2023 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it's a good idea, once it's one component, I could do a unit test for it

@pgayvallet
pgayvallet requested a review from afharo June 2, 2023 09:46
@kibana-ci

Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 414 418 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 498 502 +4
total +6

History

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

@delanni

delanni commented Jun 5, 2023

Copy link
Copy Markdown
Member Author

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 delanni added backport:prev-minor and removed backport:skip This PR does not require backporting labels Jun 5, 2023
@delanni
delanni requested a review from pgayvallet June 5, 2023 12:34

@afharo afharo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread config/README.md
Comment on lines +8 to +13
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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.

@delanni
delanni merged commit c57589e into elastic:main Jun 5, 2023
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 5, 2023
## 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)
@kibanamachine

Copy link
Copy Markdown
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jun 5, 2023
# 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":"https://github.com/elastic/kibana/pull/158827","mergeCommit":{"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"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/158827","number":158827,"mergeCommit":{"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"}}]}]
BACKPORT-->

---------

Co-authored-by: Alex Szabo <alex.szabo@elastic.co>
@delanni
delanni deleted the kib-155154-fix-config-order branch May 2, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Override default config when specifying --config CLI option

6 participants