[cli/serve] accept mulitple --config flags #6825

spalger · 2016-04-08T07:12:12Z

When parsing the CLI arguments we should allow passing the --config flag multiple times so that multiple configuration files can be used. This allows enabling use of the kibana.dev.yml file with the production build using:

# mulitple files are merged in order, last one "wins" conflicts
./bin/kibana --config=config/kibana.yml --config=config/kibana.dev.yml

epixa · 2016-04-08T16:24:16Z

src/cli/serve/read_yaml_config.js


-  _.each(deprecatedSettings, function (message, setting) {
-    if (_.has(file, setting)) console.error(message);
+  const files = [].concat(paths).map(path => {


Are you doing the .concat() here just to create a shallow copy? If so, it's not necessary because the .map() creates a new array.

I'm doing it to support receiving both an array of paths or a single path.

…/multipleConfigFlags

spalger · 2016-04-11T21:29:57Z

jenkins, test it

epixa · 2016-04-13T15:00:42Z

src/cli/serve/deprecated_config.js

+};
+
+// transform legeacy options into new namespaced versions
+export function rewriteDeprecatedConfig(object) {


Can you write some unit tests for this? I know the unit tests from read_yaml_config are at least partially covering this, but it really should have some tests of its own. Since it's not used anywhere else except in read_yaml_config at the moment and this PR is blocking other pull requests, I'm OK with not blocking on this, but can you at least follow up with another PR to add tests for it?

(I'd love to know how many times I've needed you to tell me to write more tests. Sorry man)

…/multipleConfigFlags

epixa · 2016-04-13T17:23:54Z

src/cli/serve/serve.js

-    'Path to the config file, can be changed with the CONFIG_PATH environment variable as well',
-    process.env.CONFIG_PATH || fromRoot('config/kibana.yml'))
+    'Path to the config file, can be changed with the CONFIG_PATH environment variable as well. ' +
+    'Use mulitple --config flags to include multiple config files.',


Since these aren't really flags, perhaps "multiple --config arguments" would be better?

epixa · 2016-04-13T19:41:15Z

jenkins, test it

epixa · 2016-04-13T19:41:18Z

So much fun.

spalger · 2016-04-13T19:50:47Z

jenkins, test it

epixa · 2016-04-13T20:20:35Z

src/cli/serve/deprecated_config.js

+  // join them with simple string concatenation so that
+  // compound keys don't get considered a single path segment
+  (function recurse(obj, parent = []) {
+    forOwn(obj, (val, leaf) => {


This is new, right? I didn't just miss it in the old implementation?

Well, this is new because we are now checking for deprecation before merging... Which now that I say it makes me think we should check for deprecation after merging...

spalger · 2016-04-13T21:50:03Z

jenkins, test it

epixa · 2016-04-13T22:01:18Z

src/cli/serve/deprecated_config.js

+// are checked for after the config object is prepared and known, so legacySettings
+// will have already been transformed.
+export const deprecatedSettings = new Map([
+  [['server', 'xsrf', 'token'], 'server.xsrf.token is deprecated. It is no longer used when providing xsrf protection.']


Given how this data will be used, I think a simple hash would be better here.

The inputs are private and won't be changing at runtime.

You never need to do things like access the "size" of the collection.

The key/value pairs are entered by humans and are likely to be the thing changing most frequently in this file over time.

So basically, the Map here seems to be optimizing for CPU time rather than for humans, which I don't think is the right priority in this file.

Of course, objects can't have arrays as keys, but that brings me to my next point: I think these keys should be strings instead of arrays. Again, optimizing for humans.

I sort of think about these key/values as configuration data rather than programmatic values. For the sake of fewer moving parts and a simpler and less-bug-prone implementation, I don't suggest that we pull them out into an actual configuration file or anything, but treating them as if they could have come from a configuration file seems appropriate.

Well, I'm using Map because _.has() starts by doing a hasOwnProperty() check, and therefore this test fails when I use string keys. It seems really weird to me that the function would work for direct properties and expanded properties, but not partially expanded properties considering the way that the rest of the configuration works.

If you think that it's worth it to prevent the use of Map() and have this weird behavior I can live with it, but it's not really a cost to me. Map() is no longer foreign to me.

Ah, I didn't see that test. Can you explain what "ignoring" does in this context? Maybe my brain is just fried this late in the day, but I'm struggling to make sense of it. Why would the "compoundedness" of the key determine whether a configuration should be considered deprecated?

It just means that these would warn:

checkForDeprecatedConfig({ 'server.xsrf.token': 1 }) checkForDeprecatedConfig({ server: { xsrf: { token: 1 } } })

But these would not log a warning (all though they are valid ways to specify config):

checkForDeprecatedConfig({ 'server.xsrf': { token: 1 } }) checkForDeprecatedConfig({ server: { 'xsrf.token': 1 } })

So instead it just doesn't work with compound properties.

But... why would we want to ignore valid ways to specify a config?

This function only runs on the fully expanded version of the config, the version that comes out of merge(), so this discussion is just about what this function will do if someone includes this module outside of where it's currently used. I could also revert to the previous recursive flattening that worked for all key types if you prefer.

What are the drawbacks to that? I do think this should work for any valid way to configure a key.

If that means that you must pass a fully normalized/merged config, then I think that's completely fine, but it should probably throw an exception if it were given anything else rather than silently ignoring them.

spalger · 2016-04-14T22:04:40Z

Talked with @epixa about this offline, and we both agree that this discussion has been off in the weeds for a little while. In the interest of not blocking other work this is going in.

spalger added 2 commits April 8, 2016 00:08

[cli/serve] accept mulitple --config flags

fe7abcb

[cli/serve] tests for readYamlConfig

3410b09

spalger added review v5.0.0 labels Apr 8, 2016

spalger assigned epixa Apr 8, 2016

epixa reviewed Apr 8, 2016
View reviewed changes

epixa assigned spalger and unassigned epixa Apr 11, 2016

epixa added updates_needed and removed review labels Apr 11, 2016

spalger added 2 commits April 11, 2016 13:24

[cli/server] split up file merging and deprecated settings logic

b7738f1

[cli/serve] add note to --config help text

9a6379f

spalger force-pushed the implement/cli/multipleConfigFlags branch from d58fa8f to 9a6379f Compare April 11, 2016 20:24

Merge branch 'master' of github.com:elastic/kibana into implement/cli…

62f1401

…/multipleConfigFlags

spalger assigned epixa and unassigned spalger Apr 12, 2016

spalger added review and removed updates_needed labels Apr 12, 2016

epixa reviewed Apr 13, 2016
View reviewed changes

spalger added 2 commits April 13, 2016 10:20

Merge branch 'master' of github.com:elastic/kibana into implement/cli…

65d3ddd

…/multipleConfigFlags

[cli/serve] fix typo

b217938

epixa reviewed Apr 13, 2016
View reviewed changes

epixa assigned spalger and unassigned epixa Apr 13, 2016

epixa added the updates_needed label Apr 13, 2016

[cli/serve/deprecated_config] added tests

23e4902

spalger force-pushed the implement/cli/multipleConfigFlags branch from 274d917 to 23e4902 Compare April 13, 2016 19:05

epixa reviewed Apr 13, 2016
View reviewed changes

spalger assigned epixa and unassigned spalger Apr 13, 2016

spalger added review and removed updates_needed labels Apr 13, 2016

[cli/serve] simplify deprecated config check by doing it after merge()

5a100b3

spalger force-pushed the implement/cli/multipleConfigFlags branch from d05c099 to 5a100b3 Compare April 13, 2016 21:39

epixa reviewed Apr 13, 2016
View reviewed changes

epixa assigned spalger and unassigned epixa Apr 13, 2016

spalger assigned epixa and unassigned spalger Apr 13, 2016

epixa assigned spalger and unassigned epixa Apr 14, 2016

spalger force-pushed the implement/cli/multipleConfigFlags branch 2 times, most recently from 491f640 to 5a100b3 Compare April 14, 2016 21:29

spalger merged commit 9ba255e into elastic:master Apr 14, 2016

spalger mentioned this pull request Apr 15, 2016

Only proxy whitelisted request headers to ES server upstream #6896

Merged

epixa added the v5.0.0-alpha2 label Apr 29, 2016

jbudz mentioned this pull request Nov 1, 2016

Display warning when using deprecated kibana configs #6646

Closed

spalger deleted the implement/cli/multipleConfigFlags branch October 18, 2019 17:37

legrego mentioned this pull request Apr 27, 2021

Interactive setup mode #89287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cli/serve] accept mulitple --config flags #6825

[cli/serve] accept mulitple --config flags #6825

spalger commented Apr 8, 2016

epixa Apr 8, 2016

spalger Apr 8, 2016

spalger commented Apr 11, 2016

epixa Apr 13, 2016

spalger Apr 13, 2016

epixa Apr 13, 2016

epixa commented Apr 13, 2016

epixa commented Apr 13, 2016

spalger commented Apr 13, 2016

epixa Apr 13, 2016

spalger Apr 13, 2016

spalger commented Apr 13, 2016

epixa Apr 13, 2016

spalger Apr 13, 2016

epixa Apr 14, 2016

spalger Apr 14, 2016

epixa Apr 14, 2016

spalger Apr 14, 2016

epixa Apr 14, 2016

spalger commented Apr 14, 2016

[cli/serve] accept mulitple --config flags #6825

[cli/serve] accept mulitple --config flags #6825

Conversation

spalger commented Apr 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epixa commented Apr 13, 2016

epixa commented Apr 13, 2016

spalger commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Apr 14, 2016