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

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

Merged
merged 12 commits into from
Apr 14, 2016

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Apr 8, 2016

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 


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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@epixa epixa assigned spalger and unassigned epixa Apr 11, 2016
@spalger spalger force-pushed the implement/cli/multipleConfigFlags branch from d58fa8f to 9a6379f Compare April 11, 2016 20:24
@spalger
Copy link
Contributor Author

spalger commented Apr 11, 2016

jenkins, test it

@spalger spalger assigned epixa and unassigned spalger Apr 12, 2016
};

// transform legeacy options into new namespaced versions
export function rewriteDeprecatedConfig(object) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

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

'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.',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@epixa epixa assigned spalger and unassigned epixa Apr 13, 2016
@spalger spalger force-pushed the implement/cli/multipleConfigFlags branch from 274d917 to 23e4902 Compare April 13, 2016 19:05
@epixa
Copy link
Contributor

epixa commented Apr 13, 2016

jenkins, test it

@epixa
Copy link
Contributor

epixa commented Apr 13, 2016

So much fun.

@spalger
Copy link
Contributor Author

spalger commented Apr 13, 2016

jenkins, test it

// 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) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 spalger assigned epixa and unassigned spalger Apr 13, 2016
@spalger spalger force-pushed the implement/cli/multipleConfigFlags branch from d05c099 to 5a100b3 Compare April 13, 2016 21:39
@spalger
Copy link
Contributor Author

spalger commented Apr 13, 2016

jenkins, test it

// 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.']
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. The inputs are private and won't be changing at runtime.
  2. You never need to do things like access the "size" of the collection.
  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@epixa epixa assigned spalger and unassigned epixa Apr 13, 2016
@spalger spalger assigned epixa and unassigned spalger Apr 13, 2016
@epixa epixa assigned spalger and unassigned epixa Apr 14, 2016
@spalger spalger force-pushed the implement/cli/multipleConfigFlags branch 2 times, most recently from 491f640 to 5a100b3 Compare April 14, 2016 21:29
@spalger
Copy link
Contributor Author

spalger commented Apr 14, 2016

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 spalger merged commit 9ba255e into elastic:master Apr 14, 2016
@spalger spalger deleted the implement/cli/multipleConfigFlags branch October 18, 2019 17:37
@legrego legrego mentioned this pull request Apr 27, 2021
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.

2 participants