-
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 'core' ui settings to core #75544
migrate 'core' ui settings to core #75544
Conversation
'</a>', | ||
}, | ||
}), | ||
schema: schema.string(), |
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.
AFAIK there is no way to validate a moment date format. We can't do better than just string
here unfortunately...
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.
'</a>', | ||
}, | ||
}), | ||
schema: schema.string(), |
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.
Same here, no obvious way to validate the format. Parsing the json value to check if it's an array of array[2] felt overkill (and is not enough)
buildNum: { | ||
readonly: true, | ||
schema: schema.maybe(schema.string()), | ||
}, |
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.
buildNum
was not on our list, but I guess it still belongs here.
@@ -60,6 +61,8 @@ export class UiSettingsService | |||
|
|||
savedObjects.registerType(uiSettingsType); | |||
registerRoutes(http.createRouter('')); | |||
this.register(getCoreSettings()); |
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 kept the registration internal to the service. Tell me if it should be done from elsewhere.
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.
In the core app, maybe? https://github.com/restrry/kibana/blob/e43efca3097859d72be2e8a745287f7d7eb335a0/src/core/server/core_app/core_app.ts not to mix different layers
Pinging @elastic/kibana-platform (Team:Platform) |
`"Must be a relative URL."` | ||
); | ||
expect(() => validate(125)).toThrowErrorMatchingInlineSnapshot( | ||
`"expected value of type [string] but got [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.
can we remove this validation since you added this to the core?
}, | ||
}) | ||
), | ||
defaultRoute: schema.maybe(schema.string()), |
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.
Shouldn't we remove it completely? We validate overrides on the start
this.validatesOverrides(); |
That's not the same thing entirely, but I would avoid spreading logic to different places.
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* migrate ui settings to core * add basic test on service * add unit tests * adapt buildNum schema * use any for buildNum... * move i18n keys to core prefix * translate added validation messages * using number for schema for buildNum * move state:storeInSessionStorage setting to core * remove overrides config validation * remove defaultRoute from config schema
* migrate ui settings to core * add basic test on service * add unit tests * adapt buildNum schema * use any for buildNum... * move i18n keys to core prefix * translate added validation messages * using number for schema for buildNum * move state:storeInSessionStorage setting to core * remove overrides config validation * remove defaultRoute from config schema
Summary
Fix #59755
src/core/server/ui_settings/settings
Checklist