-
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
[cli/serve] accept mulitple --config flags #6825
Changes from all commits
fe7abcb
3410b09
b7738f1
9a6379f
62f1401
65d3ddd
b217938
869fa10
448848c
a02bbd5
23e4902
5a100b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import expect from 'expect.js'; | ||
import { set } from 'lodash'; | ||
import { checkForDeprecatedConfig } from '../deprecated_config'; | ||
import sinon from 'auto-release-sinon'; | ||
|
||
describe('cli/serve/deprecated_config', function () { | ||
it('passes original config through', function () { | ||
const config = {}; | ||
set(config, 'server.xsrf.token', 'xxtokenxx'); | ||
const output = checkForDeprecatedConfig(config); | ||
expect(output).to.be(config); | ||
expect(output.server).to.be(config.server); | ||
expect(output.server.xsrf).to.be(config.server.xsrf); | ||
expect(output.server.xsrf.token).to.be(config.server.xsrf.token); | ||
}); | ||
|
||
it('logs warnings about deprecated config values', function () { | ||
const log = sinon.stub(); | ||
const config = {}; | ||
set(config, 'server.xsrf.token', 'xxtokenxx'); | ||
checkForDeprecatedConfig(config, log); | ||
sinon.assert.calledOnce(log); | ||
expect(log.firstCall.args[0]).to.match(/server\.xsrf\.token.+deprecated/); | ||
}); | ||
|
||
describe('does not support compound.keys', function () { | ||
it('ignores fully compound keys', function () { | ||
const log = sinon.stub(); | ||
const config = { 'server.xsrf.token': 'xxtokenxx' }; | ||
checkForDeprecatedConfig(config, log); | ||
sinon.assert.notCalled(log); | ||
}); | ||
|
||
it('ignores partially compound keys', function () { | ||
const log = sinon.stub(); | ||
const config = { server: { 'xsrf.token': 'xxtokenxx' } }; | ||
checkForDeprecatedConfig(config, log); | ||
sinon.assert.notCalled(log); | ||
}); | ||
|
||
it('ignores partially compound keys', function () { | ||
const log = sinon.stub(); | ||
const config = { 'server.xsrf': { token: 'xxtokenxx' } }; | ||
checkForDeprecatedConfig(config, log); | ||
sinon.assert.notCalled(log); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
server.xsrf.token: token |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
kibana_index: indexname |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
foo: 1 | ||
bar: true |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
foo: 2 | ||
baz: bonkers |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import expect from 'expect.js'; | ||
import { rewriteLegacyConfig } from '../legacy_config'; | ||
import sinon from 'auto-release-sinon'; | ||
|
||
describe('cli/serve/legacy_config', function () { | ||
it('returns a clone of the input', function () { | ||
const file = {}; | ||
const output = rewriteLegacyConfig(file); | ||
expect(output).to.not.be(file); | ||
}); | ||
|
||
it('rewrites legacy config values with literal path replacement', function () { | ||
const file = { port: 4000, host: 'kibana.com' }; | ||
const output = rewriteLegacyConfig(file); | ||
expect(output).to.not.be(file); | ||
expect(output).to.eql({ | ||
'server.port': 4000, | ||
'server.host': 'kibana.com', | ||
}); | ||
}); | ||
|
||
it('logs warnings when legacy config properties are encountered', function () { | ||
const log = sinon.stub(); | ||
rewriteLegacyConfig({ port: 5555 }, log); | ||
sinon.assert.calledOnce(log); | ||
expect(log.firstCall.args[0]).to.match(/port.+deprecated.+server\.port/); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
import expect from 'expect.js'; | ||
import { join, relative, resolve } from 'path'; | ||
import readYamlConfig from '../read_yaml_config'; | ||
import sinon from 'auto-release-sinon'; | ||
|
||
function fixture(name) { | ||
return resolve(__dirname, 'fixtures', name); | ||
} | ||
|
||
describe('cli/serve/read_yaml_config', function () { | ||
it('reads a single config file', function () { | ||
const config = readYamlConfig(fixture('one.yml')); | ||
|
||
expect(readYamlConfig(fixture('one.yml'))).to.eql({ | ||
foo: 1, | ||
bar: true, | ||
}); | ||
}); | ||
|
||
it('reads and merged mulitple config file', function () { | ||
const config = readYamlConfig([ | ||
fixture('one.yml'), | ||
fixture('two.yml') | ||
]); | ||
|
||
expect(config).to.eql({ | ||
foo: 2, | ||
bar: true, | ||
baz: 'bonkers' | ||
}); | ||
}); | ||
|
||
context('different cwd()', function () { | ||
const oldCwd = process.cwd(); | ||
const newCwd = join(oldCwd, '..'); | ||
|
||
before(function () { | ||
process.chdir(newCwd); | ||
}); | ||
|
||
it('resolves relative files based on the cwd', function () { | ||
const relativePath = relative(newCwd, fixture('one.yml')); | ||
const config = readYamlConfig(relativePath); | ||
expect(config).to.eql({ | ||
foo: 1, | ||
bar: true, | ||
}); | ||
}); | ||
|
||
it('fails to load relative paths, not found because of the cwd', function () { | ||
expect(function () { | ||
readYamlConfig(relative(oldCwd, fixture('one.yml'))); | ||
}).to.throwException(/ENOENT/); | ||
}); | ||
|
||
after(function () { | ||
process.chdir(oldCwd); | ||
}); | ||
}); | ||
|
||
context('stubbed stdout', function () { | ||
let stub; | ||
|
||
beforeEach(function () { | ||
stub = sinon.stub(process.stdout, 'write'); | ||
}); | ||
|
||
context('deprecated settings', function () { | ||
it('warns about deprecated settings', function () { | ||
readYamlConfig(fixture('deprecated.yml')); | ||
sinon.assert.calledOnce(stub); | ||
expect(stub.firstCall.args[0]).to.match(/deprecated/); | ||
stub.restore(); | ||
}); | ||
|
||
it('only warns once about deprecated settings', function () { | ||
readYamlConfig(fixture('deprecated.yml')); | ||
readYamlConfig(fixture('deprecated.yml')); | ||
readYamlConfig(fixture('deprecated.yml')); | ||
sinon.assert.notCalled(stub); // already logged in previous test | ||
stub.restore(); | ||
}); | ||
}); | ||
|
||
context('legacy settings', function () { | ||
it('warns about deprecated settings', function () { | ||
readYamlConfig(fixture('legacy.yml')); | ||
sinon.assert.calledOnce(stub); | ||
expect(stub.firstCall.args[0]).to.match(/has been replaced/); | ||
stub.restore(); | ||
}); | ||
|
||
it('only warns once about legacy settings', function () { | ||
readYamlConfig(fixture('legacy.yml')); | ||
readYamlConfig(fixture('legacy.yml')); | ||
readYamlConfig(fixture('legacy.yml')); | ||
sinon.assert.notCalled(stub); // already logged in previous test | ||
stub.restore(); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import { forOwn, has, noop } from 'lodash'; | ||
|
||
// deprecated settings are still allowed, but will be removed at a later time. They | ||
// 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.'] | ||
]); | ||
|
||
// check for and warn about deprecated settings | ||
export function checkForDeprecatedConfig(object, log = noop) { | ||
for (const [key, msg] of deprecatedSettings.entries()) { | ||
if (has(object, key)) log(msg); | ||
} | ||
return object; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import { noop, transform } from 'lodash'; | ||
|
||
// legacySettings allow kibana 4.2+ to accept the same config file that people | ||
// used for kibana 4.0 and 4.1. These settings are transformed to their modern | ||
// equivalents at the very begining of the process | ||
export const legacySettings = { | ||
// server | ||
port: 'server.port', | ||
host: 'server.host', | ||
pid_file: 'pid.file', | ||
ssl_cert_file: 'server.ssl.cert', | ||
ssl_key_file: 'server.ssl.key', | ||
|
||
// logging | ||
log_file: 'logging.dest', | ||
|
||
// kibana | ||
kibana_index: 'kibana.index', | ||
default_app_id: 'kibana.defaultAppId', | ||
|
||
// es | ||
ca: 'elasticsearch.ssl.ca', | ||
elasticsearch_preserve_host: 'elasticsearch.preserveHost', | ||
elasticsearch_url: 'elasticsearch.url', | ||
kibana_elasticsearch_client_crt: 'elasticsearch.ssl.cert', | ||
kibana_elasticsearch_client_key: 'elasticsearch.ssl.key', | ||
kibana_elasticsearch_password: 'elasticsearch.password', | ||
kibana_elasticsearch_username: 'elasticsearch.username', | ||
ping_timeout: 'elasticsearch.pingTimeout', | ||
request_timeout: 'elasticsearch.requestTimeout', | ||
shard_timeout: 'elasticsearch.shardTimeout', | ||
startup_timeout: 'elasticsearch.startupTimeout', | ||
verify_ssl: 'elasticsearch.ssl.verify', | ||
}; | ||
|
||
// transform legacy options into new namespaced versions | ||
export function rewriteLegacyConfig(object, log = noop) { | ||
return transform(object, (clone, val, key) => { | ||
if (legacySettings.hasOwnProperty(key)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you even Not that you'd actually do that on the same line. And this is a general remark about code style/expressiveness rather than something that should be addressed in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a bad idea, hadn't thought about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anytime I'm creating a set of values that I will regularly be checking for membership I use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that I'm implementing this, and tracking the keys in their own variable, I'm really confused how this is beneficial? Is If this is about aesthetics then I think that If it's about complexity, then I would counter that tracking the key list of an outside of the object just so you can reimplement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're so far into the weeds of personal preferences here, so I'd say we just move on :P |
||
const replacement = legacySettings[key]; | ||
log(`Config key "${key}" is deprecated. It has been replaced with "${replacement}"`); | ||
clone[replacement] = val; | ||
} else { | ||
clone[key] = val; | ||
} | ||
}, {}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,75 +1,40 @@ | ||
import _ from 'lodash'; | ||
import fs from 'fs'; | ||
import yaml from 'js-yaml'; | ||
import { chain, isArray, isPlainObject, forOwn, memoize, set, transform } from 'lodash'; | ||
import { readFileSync as read } from 'fs'; | ||
import { safeLoad } from 'js-yaml'; | ||
import { red } from 'ansicolors'; | ||
|
||
import { fromRoot } from '../../utils'; | ||
|
||
let legacySettingMap = { | ||
// server | ||
port: 'server.port', | ||
host: 'server.host', | ||
pid_file: 'pid.file', | ||
ssl_cert_file: 'server.ssl.cert', | ||
ssl_key_file: 'server.ssl.key', | ||
|
||
// logging | ||
log_file: 'logging.dest', | ||
|
||
// kibana | ||
kibana_index: 'kibana.index', | ||
default_app_id: 'kibana.defaultAppId', | ||
|
||
// es | ||
ca: 'elasticsearch.ssl.ca', | ||
elasticsearch_preserve_host: 'elasticsearch.preserveHost', | ||
elasticsearch_url: 'elasticsearch.url', | ||
kibana_elasticsearch_client_crt: 'elasticsearch.ssl.cert', | ||
kibana_elasticsearch_client_key: 'elasticsearch.ssl.key', | ||
kibana_elasticsearch_password: 'elasticsearch.password', | ||
kibana_elasticsearch_username: 'elasticsearch.username', | ||
ping_timeout: 'elasticsearch.pingTimeout', | ||
request_timeout: 'elasticsearch.requestTimeout', | ||
shard_timeout: 'elasticsearch.shardTimeout', | ||
startup_timeout: 'elasticsearch.startupTimeout', | ||
verify_ssl: 'elasticsearch.ssl.verify', | ||
}; | ||
|
||
const deprecatedSettings = { | ||
'server.xsrf.token': 'server.xsrf.token is deprecated. It is no longer used when providing xsrf protection.' | ||
}; | ||
|
||
module.exports = function (path) { | ||
if (!path) return {}; | ||
|
||
let file = yaml.safeLoad(fs.readFileSync(path, 'utf8')); | ||
|
||
function apply(config, val, key) { | ||
if (_.isPlainObject(val)) { | ||
_.forOwn(val, function (subVal, subKey) { | ||
apply(config, subVal, key + '.' + subKey); | ||
}); | ||
} | ||
else if (_.isArray(val)) { | ||
config[key] = []; | ||
val.forEach((subVal, i) => { | ||
apply(config, subVal, key + '.' + i); | ||
}); | ||
} | ||
else { | ||
_.set(config, key, val); | ||
} | ||
} | ||
|
||
_.each(deprecatedSettings, function (message, setting) { | ||
if (_.has(file, setting)) console.error(message); | ||
}); | ||
|
||
// transform legeacy options into new namespaced versions | ||
return _.transform(file, function (config, val, key) { | ||
if (legacySettingMap.hasOwnProperty(key)) { | ||
key = legacySettingMap[key]; | ||
} | ||
|
||
apply(config, val, key); | ||
import { rewriteLegacyConfig } from './legacy_config'; | ||
import { checkForDeprecatedConfig } from './deprecated_config'; | ||
|
||
const log = memoize(function (message) { | ||
console.log(red('WARNING:'), message); | ||
}); | ||
|
||
export function merge(sources) { | ||
return transform(sources, (merged, source) => { | ||
forOwn(source, function apply(val, key) { | ||
if (isPlainObject(val)) { | ||
forOwn(val, function (subVal, subKey) { | ||
apply(subVal, key + '.' + subKey); | ||
}); | ||
return; | ||
} | ||
|
||
if (isArray(val)) { | ||
set(merged, key, []); | ||
val.forEach((subVal, i) => apply(subVal, key + '.' + i)); | ||
return; | ||
} | ||
|
||
set(merged, key, val); | ||
}); | ||
}, {}); | ||
}; | ||
} | ||
|
||
export default function (paths) { | ||
const files = [].concat(paths || []); | ||
const yamls = files.map(path => safeLoad(read(path, 'utf8'))); | ||
const config = merge(yamls.map(file => rewriteLegacyConfig(file, log))); | ||
return checkForDeprecatedConfig(config, log); | ||
} |
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.
Given how this data will be used, I think a simple hash would be better here.
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.
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.
Well, I'm using
Map
because_.has()
starts by doing ahasOwnProperty()
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.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.
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?
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.
It just means that these would warn:
But these would not log a warning (all though they are valid ways to specify config):
So instead it just doesn't work with compound properties.
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.
But... why would we want to ignore valid ways to specify a config?
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 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.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.
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.