Skip to content
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

[uiSettings] Allow setting owners to register migrations #92765

Open
pgayvallet opened this issue Feb 25, 2021 · 11 comments
Open

[uiSettings] Allow setting owners to register migrations #92765

pgayvallet opened this issue Feb 25, 2021 · 11 comments
Labels
enhancement New value added to drive a business result Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@pgayvallet
Copy link
Contributor

We currently have no way to let setting owners register migrations for their settings, forcing to implement such migrations directly into core code

export const migrations = {
'7.9.0': (doc: SavedObjectUnsanitizedDoc<any>): SavedObjectSanitizedDoc<any> => ({
...doc,
...(doc.attributes && {
attributes: Object.keys(doc.attributes).reduce(
(acc, key) =>
key.startsWith('siem:')
? {
...acc,
[key.replace('siem', 'securitySolution')]: doc.attributes[key],
}

We should enhance the uiSettings server-side API to allow such migration without doing it directly in core.

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc enhancement New value added to drive a business result labels Feb 25, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@Bamieh
Copy link
Member

Bamieh commented Feb 25, 2021

I feel that the migrations API is too complicated for this usecase. We can have the setting migrated without requiring a version or all those transformers to be written by developers.

If the deprecated ui settings is being used we can automatically migrate it. Something similar to the rename helper for plugin configs

@afharo
Copy link
Member

afharo commented Mar 4, 2021

Apart from renaming, some additional transformations are also required. There was a recently merged PR that required some internal parsing and transformation.

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Mar 4, 2021

We should enhance the uiSettings server-side API to allow such migration without doing it directly in core.

@pgayvallet ++ to that.
We'll need migrations for the ProfileDataService along with other uiSettings enhancements. If we don't already have a Meta issue for these enhancements, we should think about adding one.

@pgayvallet
Copy link
Contributor Author

If we don't already have a Meta issue for these enhancements, we should think about adding one.

I'm not sure we have, but maybe @restrry could confirm that?

@mshustov
Copy link
Contributor

mshustov commented Mar 10, 2021

If we don't already have a Meta issue for these enhancements, we should think about adding one.

we have an umbrella issue to collect enhancement requests for UiSettings service #48925

However, I agree deserves a dedicated issue to discuss (maybe extending the current one) considering that we will need a migration mechanism for both PUS and PUP from ProfileDataService

Apart from renaming, some additional transformations are also required

I'm not sure that the case with renaming justifies complexity. A key is not a public API, what is the problem to use any name for it?
Maybe a migration mechanism for values is enough?

store.registerMigration({
  // doesn't have access to the whole SO
  'my_key': (value) => newValue;
});

@joshdover
Copy link
Contributor

We'll need migrations for the ProfileDataService along with other uiSettings enhancements.

Do we need it? I'm kind of fine with one-off hacks similar to what we did in #93409 until we have more demand, but if we think we need it to migrate existing settings to PUS and we expect that migration to happen slowly over time, then adding this sooner would make sense.

@pgayvallet
Copy link
Contributor Author

Do we need it?

We'll probably have a more definitive answer to this question once we finish the initial user data service implementation proposal @TinaHeiligers?

@TinaHeiligers
Copy link
Contributor

We'll probably have a more definitive answer to this question once we finish the initial user data service implementation proposal

We can start off with once-offs in the user data service feature POC and see to what extent more 'formal' migrations are needed.

@legrego what's your gut instinct on a migration service for uiSettings?

@legrego
Copy link
Member

legrego commented Mar 15, 2021

My gut instinct says that we can hold off on a migration service until we have a better understanding of what we need.

@lizozom
Copy link
Contributor

lizozom commented Mar 17, 2021

I just worry about what would happen if for some reason there is a problem caused by migration execution order. I agree with @legrego to hold off until we have better understanding and stronger use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

9 participants