-
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
Upgrade Hapi in legacy platform to v17 #21707
Changes from 52 commits
1ffeea5
efcd412
4145ea9
3598c47
b76feac
bc1b3dc
b29d889
eb333a4
0375953
844d401
2c37a8f
8a6a467
932784a
d4f1529
5d1a45b
a3d4b36
48e5478
1144116
a4a5182
deaff7e
d0979f3
71c08ab
4d58f72
37cfb23
200688c
08dd615
cb40331
511b9fd
aea9afb
80b22ce
f4f5ab6
40c83bc
fbdec15
208c2ef
81a89e9
e99e300
94c6b6a
754c418
59a9f36
4269f42
67141f3
0a6f23e
9b8b379
2f166b0
aa1349c
a7be65d
d3f5cd7
b123769
62ded83
44484d0
9ec768e
84bcd55
d638355
540b11e
3e86acd
9a8ca1d
d514683
9a88ed4
5fdded2
b8bfc87
99f1943
ebb350a
93464fd
d5be0d1
11d6206
bc09fc0
058d949
cab2759
08e51ce
bd75e0e
8409827
459ff04
9f6da50
864125b
8b5c4d9
09db158
30069b1
b913b85
215fb1f
bcaf5c7
e33c6fb
3d6024e
6e178d5
cb03a0e
03877d1
d8f3192
82e09a5
e1358a5
95375dc
aed98ff
efdd3eb
4835ca7
f13abf3
ad14fc1
140f53b
3645fe4
a0ad29c
93e1b11
82f9cd1
ea04f0e
022427b
49df9f1
f28634d
f5e944b
95fe3a4
ae55ff4
56b2ccf
006c557
61d02ea
9cc2ca1
1e1e571
709ba87
4bff36c
6c2c189
729ad76
47706a4
bc60421
cf3ec40
bf7ee19
bb9d98d
9fb482b
fa357b0
b556358
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 |
---|---|---|
|
@@ -84,7 +84,7 @@ | |
"babel-polyfill": "6.20.0", | ||
"babel-register": "6.18.0", | ||
"bluebird": "2.9.34", | ||
"boom": "5.2.0", | ||
"boom": "7.2.0", | ||
"brace": "0.11.1", | ||
"cache-loader": "1.0.3", | ||
"chalk": "^2.4.1", | ||
|
@@ -100,25 +100,24 @@ | |
"elasticsearch": "^14.2.1", | ||
"elasticsearch-browser": "^14.2.1", | ||
"encode-uri-query": "1.0.0", | ||
"even-better": "7.0.2", | ||
"execa": "^0.10.0", | ||
"expiry-js": "0.1.7", | ||
"extract-text-webpack-plugin": "3.0.1", | ||
"file-loader": "1.1.4", | ||
"font-awesome": "4.4.0", | ||
"glob": "^7.1.2", | ||
"glob-all": "^3.1.0", | ||
"good": "github:elastic/good#17.5.3-kibana1", | ||
"good-squeeze": "2.1.0", | ||
"h2o2": "5.1.1", | ||
"h2o2": "8.1.2", | ||
"h2o2-latest": "npm:[email protected]", | ||
"handlebars": "4.0.5", | ||
"hapi": "14.2.0", | ||
"hapi-latest": "npm:[email protected]", | ||
"hapi": "17.5.3", | ||
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 have been moving towards using carets for version matching and relying on the lock file. Lets go ahead and update these packages to use carets unless you see a reason not to. 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. SGTM |
||
"hjson": "3.1.0", | ||
"http-proxy-agent": "^2.1.0", | ||
"https-proxy-agent": "^2.2.1", | ||
"inert": "4.0.2", | ||
"joi": "10.4.1", | ||
"inert": "5.1.0", | ||
"joi": "13.5.2", | ||
"jquery": "^3.3.1", | ||
"js-yaml": "3.4.1", | ||
"json-stringify-pretty-compact": "1.0.4", | ||
|
@@ -192,11 +191,11 @@ | |
"vega-lite": "^2.4.0", | ||
"vega-schema-url-parser": "1.0.0", | ||
"vega-tooltip": "^0.9.14", | ||
"vision": "4.1.0", | ||
"vision": "5.3.3", | ||
"webpack": "3.6.0", | ||
"webpack-merge": "4.1.0", | ||
"whatwg-fetch": "^2.0.3", | ||
"wreck": "12.4.0", | ||
"wreck": "14.0.2", | ||
"x-pack": "link:x-pack", | ||
"yauzl": "2.7.0" | ||
}, | ||
|
@@ -222,10 +221,10 @@ | |
"@types/fetch-mock": "^5.12.2", | ||
"@types/getopts": "^2.0.0", | ||
"@types/glob": "^5.0.35", | ||
"@types/hapi-latest": "npm:@types/hapi@17.0.12", | ||
"@types/hapi": "17.0.18", | ||
"@types/has-ansi": "^3.0.0", | ||
"@types/jest": "^22.2.3", | ||
"@types/joi": "^10.4.4", | ||
"@types/joi": "^13.4.2", | ||
"@types/jquery": "3.3.1", | ||
"@types/js-yaml": "^3.11.1", | ||
"@types/listr": "^0.13.0", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,6 @@ jest.mock('../../server/logging', () => ({ | |
setupLogging: jest.fn(), | ||
})); | ||
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. note: this file will go away soon with #22190 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. Great! |
||
|
||
import { Server } from 'hapi'; | ||
import { createBasePathProxy as createBasePathProxyMock } from '../../core'; | ||
import { setupLogging as setupLoggingMock } from '../../server/logging'; | ||
import { configureBasePathProxy } from './configure_base_path_proxy'; | ||
|
@@ -49,7 +48,7 @@ describe('configureBasePathProxy()', () => { | |
|
||
// Check that logging is configured with the right parameters. | ||
expect(setupLoggingMock).toHaveBeenCalledWith( | ||
expect.any(Server), | ||
expect.anything(), | ||
configMock | ||
); | ||
|
||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,6 @@ import { writeFileSync } from 'fs'; | |
import { relative, resolve } from 'path'; | ||
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. note: I was planning to revive and make these tests stable in the scope of #21140, but haven't started yet. Are you going to handle that instead? If yes, please re-assign this one to yourself. Thanks! 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. Yep already done, will take that issue! |
||
import { safeDump } from 'js-yaml'; | ||
import es from 'event-stream'; | ||
import stripAnsi from 'strip-ansi'; | ||
import { readYamlConfig } from '../read_yaml_config'; | ||
|
||
const testConfigFile = follow('__fixtures__/reload_logging_config/kibana.test.yml'); | ||
|
@@ -42,19 +41,7 @@ function setLoggingJson(enabled) { | |
writeFileSync(testConfigFile, yaml); | ||
} | ||
|
||
const prepareJson = obj => ({ | ||
...obj, | ||
pid: '## PID ##', | ||
'@timestamp': '## @timestamp ##' | ||
}); | ||
|
||
const prepareLogLine = str => | ||
stripAnsi(str.replace( | ||
/\[\d{2}:\d{2}:\d{2}.\d{3}\]/, | ||
'[## timestamp ##]' | ||
)); | ||
|
||
describe.skip('Server logging configuration', function () { | ||
describe('Server logging configuration', function () { | ||
let child; | ||
let isJson; | ||
|
||
|
@@ -80,20 +67,23 @@ describe.skip('Server logging configuration', function () { | |
}); | ||
} else { | ||
it('should be reloadable via SIGHUP process signaling', function (done) { | ||
expect.assertions(1); | ||
expect.assertions(3); | ||
|
||
child = spawn('node', [kibanaPath, '--config', testConfigFile]); | ||
|
||
child.on('error', err => { | ||
done(new Error(`error in child process while attempting to reload config. ${err.stack || err.message || err}`)); | ||
}); | ||
|
||
const lines = []; | ||
let sawJson = false; | ||
let sawNonjson = false; | ||
|
||
child.on('exit', _code => { | ||
const code = _code === null ? 0 : _code; | ||
|
||
expect({ code, lines }).toMatchSnapshot(); | ||
expect(code).toEqual(0); | ||
expect(sawJson).toEqual(true); | ||
expect(sawNonjson).toEqual(true); | ||
done(); | ||
}); | ||
|
||
|
@@ -107,23 +97,18 @@ describe.skip('Server logging configuration', function () { | |
|
||
if (isJson) { | ||
const data = JSON.parse(line); | ||
lines.push(prepareJson(data)); | ||
sawJson = true; | ||
|
||
if (data.tags.includes('listening')) { | ||
switchToPlainTextLog(); | ||
} | ||
} else if (line.startsWith('{')) { | ||
// We have told Kibana to stop logging json, but it hasn't completed | ||
// the switch yet, so we verify the messages that are logged while | ||
// switching over. | ||
|
||
const data = JSON.parse(line); | ||
lines.push(prepareJson(data)); | ||
// the switch yet, so we ignore before switching over. | ||
} else { | ||
// Kibana has successfully stopped logging json, so we verify the | ||
// log line and kill the server. | ||
// Kibana has successfully stopped logging json, so kill the server. | ||
|
||
lines.push(prepareLogLine(line)); | ||
sawNonjson = true; | ||
|
||
child.kill(); | ||
child = undefined; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,12 +98,7 @@ export abstract class Type<V> { | |
return error; | ||
} | ||
|
||
const { context = {}, type, path: rawPath, message } = error; | ||
|
||
// Before v11.0.0 Joi reported paths as `.`-delimited strings, but more | ||
// recent version use arrays instead. Once we upgrade Joi, we should just | ||
// remove this split logic and use `path` provided by Joi directly. | ||
const path = rawPath ? rawPath.split('.') : []; | ||
const { context = {}, type, path, message } = error; | ||
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. note: 👍 |
||
|
||
const errorHandleResult = this.handleError(type, context, path); | ||
if (errorHandleResult instanceof SchemaTypeError) { | ||
|
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.
nit: For examples, I would prefer to keep the long-form variable names.
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.
Hapi convention is to call this
h
now. They've removed thereply
name completely. I agree it's not a great choice, but might be easier for us if we're consistent with hapi documentation?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.
Ok, I think this is fine for the old platform. In the new platform we are not interfacing with the entire router as to not expose any router internals, which is still Hapi in the new platform.