-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Upgrade Assistant] Fix getFlatSettings() request #89616
[Upgrade Assistant] Fix getFlatSettings() request #89616
Conversation
@elasticmachine merge upstream |
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great finding and thanks for fixing this @alisonelizabeth ! I did not re-test locally, since I tested the previous refactor locally and everything seemed to go OK.
Would it be worth adding a test to confirm that mappings + settings we expect on the reindexed index are present? From the code it looks like the result of this call is being used here:
kibana/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts
Line 313 in 19b1f46
const { settings, mappings } = transformFlatSettings(flatSettings); |
Thanks for the review @jloleysens! I've added a line to the existing reindexing integration test to verify mappings exist on the new index. |
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Alerting API Integration Tests.x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/rbac_legacy·ts.alerting api integration security and spaces enabled Alerts legacy alerts alerts superuser at space1 should schedule actions on legacy alertsStandard Out
Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
This fixes a regression of #89207.
I incorrectly migrated:
to
await esClient.indices.getSettings()
instead ofawait esClient.indices.get()
;indices.getSettings()
will only return the index settings and not the mappings.This was caught in the backport PR (#89435), since we are checking for types there. I've updated the tests as part of this PR, however, note that it passed originally since we were mocking the response correctly.