-
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
Migrate defaultIndex attribute for config saved object #133339
Migrate defaultIndex attribute for config saved object #133339
Conversation
) { | ||
defaultIndex = upgradeableConfig.attributes.defaultIndex; // Keep the existing defaultIndex attribute if the data view is not found | ||
try { | ||
// The defaultIndex for this config object was created prior to 8.3, and it might refer to a data view ID that is no longer valid. |
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.
I love all the code comments but it begs the question of "Why do we need to explain what we're doing?". The whole config/settings situation needs an overhaul to avoid similar situations. For now, though, could we also add this information to the dev docs, please?
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 idea.
Looking at the docs I found a page for Advanced Settings, which mentions each setting and what it is for.
https://www.elastic.co/guide/en/kibana/8.3/advanced-options.html#defaultindex
Do you think I should add a "NOTE" admonition there? Or did you have something else in mind?
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.
I don't think we need to concern end-users with the internals of what we're doing here, it's more for internal developer guidance.
I was thinking about adding a note to the tutorial on how to register a uiSetting
migration. The file itself lives here
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.
Updated in c84b7c3, is this what you had in mind?
src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.ts
Outdated
Show resolved
Hide resolved
Cleaned up mocks and removed a few instances of `any` in the process.
Also added tests!
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.
Haven't gotten a chance to test locally yet, but the approach here makes sense overall
src/core/server/ui_settings/create_or_upgrade_saved_config/get_upgradeable_config.ts
Outdated
Show resolved
Hide resolved
src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.ts
Outdated
Show resolved
Hide resolved
* The `config` object type contains many attributes that are defined by consumers. | ||
*/ | ||
export interface ConfigAttributes { | ||
buildNum: number; |
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.
NIT: should we add isDefaultIndexMigrated
here too?
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.
I don't think so, this was to address Luke's feedback in #133339 (comment), we have a separate UpgradeableConfigAttributes
type that extends ConfigAttributes
and specifies other fields such as isDefaultIndexMigrated
.
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.
LGTM, thanks for doing this
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.
LGTM, thanks for all of the edits!
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.
LGTM too!
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit d732ebe) # Conflicts: # src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.ts
(cherry picked from commit d732ebe) # Conflicts: # src/core/server/ui_settings/create_or_upgrade_saved_config/create_or_upgrade_saved_config.ts
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
Pinging @elastic/kibana-core (Team:Core) |
Pinging @elastic/kibana-security (Team:Security) |
Resolves #133241
Overview
This PR adds a new mechanism to apply custom transform functions to the
config
saved object for UI settings. These transform functions are complementary to saved object migrations.The
config
saved objects are versioned and isolated to each space.When a client attempts to fetch or create a
config
saved object, Kibana first checks if there's already an old one that needs to be upgraded. This PR takes advantage of the existing functionality to simply call any necessary transform functions during that process. This way, we can ensure that transforms will be applied to eachconfig
after upgrading, but we don't need to add a complicated "on start" loop to search and update saved objects.Testing
To manually test this end-to-end to verify the migration is applied:
ff959d40-b880-11e8-a6d9-e546fe2bba5f
in the Default space, along with a legacy URL aliasredirect-with-toast
that points to the aforementioned IDsuperduperuser
/changeme
which was created by the script and has access to modify system indicesRelease note: The advanced setting for
defaultIndex
could be broken (not pointing to the correct index pattern) if it was set in a custom space before upgrading to the 8.0 release. The setting is now migrated correctly the first time that space is accessed by any user that hasAll
access to the Advanced Settings feature.