-
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
Add backend provider for default settings #6849
Conversation
The current, client-sided implementation of Advanced Settings will send an entire local copy of current I've opted to use the |
.then(res => res._source) | ||
.then(user => assign(defaults, user, nonEmpty)) | ||
.then(settings => reply(settings).type('application/json')) | ||
.catch(reason => reply(Boom.create(500, `Elasticsearch failure.`))); |
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.
Use Boom.wrap()
here, since you already have an error
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.
Isn't it bad practice to forward errors as-is to the client-side?
method: 'POST', | ||
handler: async function (req, reply) { | ||
const key = req.params.key; | ||
const value = req.query.value; |
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 feel like we should require the value key. Otherwise POST /api/kibana/settings/something
is actually a DELETE
I think that an update with null approach is fine, but keep in mind that the "version" parameter can be used to prevent timing issues. |
Questions I have
Other than that, and tests, work here is mostly done |
Lets focus on keeping the behavior we have in place, whatever that is
Not really, but several plugins are hacking new configuration values into the front-end configuration services. Isn't a "blessed" way of doing this a part of this pr?
This is for the hacks I mentioned earlier. If the configuration value doesn't have a definition in the defaults file it is "custom". See #4924 |
Great. I'll make sure to account for that. |
The latest commits should address all your concerns. 🎉 |
LGTM |
@epixa Wondering if you'd want to take a look at this? |
@@ -17,39 +16,29 @@ uiModules.get('apps/settings') | |||
return { | |||
restrict: 'E', | |||
link: function ($scope) { | |||
const configDefaults = Private(ConfigDefaultsProvider); | |||
const keyCodes = { |
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.
This keyCodes
doesn't appear to be used
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.
Removed
LGTM! |
This script will change default timezone used by Kibana to display timestamps. It is ingesting a proper setting directly into '.kibana' index stored in elasticsearch. It will also crate .kibana index if one doesn't exist with default settings replicated from Kibana: 1 shard with 1 replica only. After release of Kibana 5 we will refactor this script to use Kibana API: elastic/kibana#6849
This script will change the default timezone used by Kibana to display timestamps. It emits a proper setting directly into the `.kibana` index stored in elasticsearch. It will also create a `.kibana` index if one doesn't exist, with the default settings normally used by Kibana: 1 shard with 1 replica only. After the release of Kibana 5 we will refactor this script to use Kibana API: elastic/kibana#6849 @combor & @benhyland
This script will change the default timezone used by Kibana to display timestamps. It emits a proper setting directly into the `.kibana` index stored in elasticsearch. It will also create a `.kibana` index if one doesn't exist, with the default settings normally used by Kibana: 1 shard with 1 replica only. After the release of Kibana 5 we will refactor this script to use Kibana API: elastic/kibana#6849 @combor & @benhyland
This script will change the default timezone used by Kibana to display timestamps. It emits a proper setting directly into the `.kibana` index stored in elasticsearch. It will also create a `.kibana` index if one doesn't exist, with the default settings normally used by Kibana: 1 shard with 1 replica only. After the release of Kibana 5 we will refactor this script to use Kibana API: elastic/kibana#6849 @combor & @benhyland
This script will change the default timezone used by Kibana to display timestamps. It emits a proper setting directly into the `.kibana` index stored in elasticsearch. It will also create a `.kibana` index if one doesn't exist, with the default settings normally used by Kibana: 1 shard with 1 replica only. After the release of Kibana 5 we will refactor this script to use Kibana API: elastic/kibana#6849 Test that the script produces the expected exit codes and hits the expected endpoints. We run the script under test in a separate process since that is closer to reality. This requires us to mock http endpoints using a real http server. We had to upgrade `mimic` and work around a few design issues as the tooling for doing this isn't very good. We might want to reconsider how we do this sort of testing if we have to do a lot of it. @combor & @benhyland
Implementation for #5317
GET /api/kibana/settings/:key
retrieves current state forkey
POST /api/kibana/settings/:key
setskey
to postedvalue
POST /api/kibana/settings
sets eachkey: value
in postedchanges
DELETE /api/kibana/settings/:key
changeskey
to its default valueIn front-end, usage over
config
service API is preferred.In server-side, usage over
server.uiSettings()
is preferred.