Install plugin #4406
Install plugin #4406
Conversation
Conflicts: package.json
| expect(errorStub.called).to.be(true); | ||
| expect(errorStub.lastCall.args[0].message).to.match(/This is unexpected./); | ||
|
|
||
| fs.statSync.restore(); |
There was a problem hiding this comment.
I have a feeling this is causing issues. If you put .restore() calls within a test they are likely to not run
85c8edd to
6a59781
Compare
| @@ -9,6 +9,7 @@ set NODE=%DIR%\node\node.exe | |||
| set SERVER=%DIR%\src\bin\kibana.js | |||
| set NODE_ENV="production" | |||
| set CONFIG_PATH=%DIR%\config\kibana.yml | |||
| set NPM=npm | |||
There was a problem hiding this comment.
Looks like you are signaling to the installer what command to use for npm install. Seems like the default is already npm, but anyway many of the machines where this will run won't have npm or node in the path.
We should use the npm bundled with the node instance we are current running in, or depend on it and use the version installed locally. To find the npm bundled with the current node executable use:
resolve(dirname(process.execPath), '../lib/node_modules/npm')
PS: I learned that resolve properly handles / conversion. Exciting!!
There was a problem hiding this comment.
I applied this change, however the path needed to be slightly different for me. Please review and let me know if that path will work for all instances, or if that is a windows-specific nuance.
|
Should we be seeing this "Error extracting the plugin archive" error at the end of the output? I would have expected that before "Plugin installation was unsuccessful". Perhaps it's a timing issue with the errors emitted from the streams?
Just checked and I think it's just the error message that ends up in the log. I like that bug maybe it should be formatted to describe that. Maybe |
|
|
||
| rimraf.sync(settings.pluginPath); | ||
| } catch (ex) { | ||
| logger.error(ex.message); |
There was a problem hiding this comment.
Can we log something more informative here, maybe:
Unable to remove plugin "${settings.package}" because of error: "${ex.message}"
|
We should include some more information about how the |
| var cmd = (process.env.NPM) ? process.env.NPM : 'npm'; | ||
| cmd += ' install'; | ||
|
|
||
| var child = exec(cmd, { cwd: dest }); |
There was a problem hiding this comment.
The output of this command needs to be ignored when the --silent option is specified.
There was a problem hiding this comment.
Modified the --silent option so that it applies to both log and error messages per a conversation with @rashidkpc.
|
Functionality is looking great! For posterity, here are the things I checked manually:
|
3b0c0bf to
7a37dcf
Compare
|
Couple more comments, but looking great! I'm 99% ready to merge. Time for someone else to give it a spin. @jbudz? |
| .description('Maintain Plugins') | ||
| .option('-i, --install <org>/<plugin>/<version>', installDesc) | ||
| .option('-r, --remove <plugin>', 'The plugin to remove') | ||
| .option('-s, --silent', 'Disable process messaging') |
There was a problem hiding this comment.
Thoughts on --silent vs --quiet without the plugin command?
There was a problem hiding this comment.
Good catch @jbudz, I like the idea of supporting both on both commands at some point.
--silentwould block absolutely all output--quietwould block everything but errors
|
Works well, code looks good. I made a minor comment on an option name. The grunt tasks seem to be processing plugins//node_modules/, running grunt test after installing a plugin can fail on jshint/jscs. |
…ser to clean up scope
Addresses #4204
Creates the Kibana plugin install cli.
To use:
from the src/server/bin folder, type: