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

Add backend provider for default settings #6849

Merged
merged 47 commits into from
May 20, 2016

Conversation

bevacqua
Copy link
Contributor

@bevacqua bevacqua commented Apr 11, 2016

Implementation for #5317

  • GET /api/kibana/settings/:key retrieves current state for key
  • POST /api/kibana/settings/:key sets key to posted value
  • POST /api/kibana/settings sets each key: value in posted changes
  • DELETE /api/kibana/settings/:key changes key to its default value

In front-end, usage over config service API is preferred.

In server-side, usage over server.uiSettings() is preferred.

@bevacqua
Copy link
Contributor Author

The current, client-sided implementation of Advanced Settings will send an entire local copy of current settings to Elasticsearch whenever a setting is deleted, using .index(currentSettings). When a setting is just changed, in contrast, it will use .update({ [field]: value }) on the changed field.

I've opted to use the .update approach in all cases, and then when returning the properties from GET /api/kibana/settings, if the field has a value of null it'll use the default setting. Is this okay or would we rather query the current settings and re-index using that, risking running into timing issues?

.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.`)));
Copy link
Contributor

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

Copy link
Contributor Author

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;
Copy link
Contributor

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

@spalger
Copy link
Contributor

spalger commented Apr 11, 2016

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.

@bevacqua
Copy link
Contributor Author

Questions I have

  • Should we update settings on other tabs (via local storage) when they change on the current tab?
  • Can custom settings be created/used by plugins that I should be aware of? Or what is (Custom setting) in the advanced_row template?

Other than that, and tests, work here is mostly done

@bevacqua bevacqua assigned spalger and unassigned bevacqua Apr 13, 2016
@spalger
Copy link
Contributor

spalger commented Apr 13, 2016

Should we update settings on other tabs (via local storage) when they change on the current tab?

Lets focus on keeping the behavior we have in place, whatever that is

Can custom settings be created/used by plugins

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?

what is (Custom setting) in the advanced_row template?

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

@bevacqua
Copy link
Contributor Author

Great. I'll make sure to account for that.

@bevacqua
Copy link
Contributor Author

The latest commits should address all your concerns. 🎉

@spalger
Copy link
Contributor

spalger commented May 19, 2016

LGTM

@bevacqua bevacqua assigned epixa and unassigned spalger May 19, 2016
@bevacqua
Copy link
Contributor Author

@epixa Wondering if you'd want to take a look at this?

@bevacqua bevacqua assigned panda01 and unassigned epixa May 20, 2016
@@ -17,39 +16,29 @@ uiModules.get('apps/settings')
return {
restrict: 'E',
link: function ($scope) {
const configDefaults = Private(ConfigDefaultsProvider);
const keyCodes = {
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@panda01
Copy link
Contributor

panda01 commented May 20, 2016

LGTM!

@panda01 panda01 assigned bevacqua and unassigned panda01 May 20, 2016
@bevacqua bevacqua merged commit 5df51d5 into elastic:master May 20, 2016
@bevacqua bevacqua removed their assignment May 20, 2016
@epixa epixa added the v5.0.0 label May 21, 2016
@bevacqua bevacqua deleted the feature/restify-config branch May 24, 2016 01:53
combor pushed a commit to alphagov/paas-cf that referenced this pull request Jul 20, 2016
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
benhyland pushed a commit to alphagov/paas-cf that referenced this pull request Jul 21, 2016
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
benhyland pushed a commit to alphagov/paas-cf that referenced this pull request Jul 21, 2016
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
benhyland pushed a commit to alphagov/paas-cf that referenced this pull request Jul 22, 2016
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
henrytk pushed a commit to alphagov/paas-cf that referenced this pull request Jul 25, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants