-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Plugin pack installer #6402
Plugin pack installer #6402
Conversation
Initial observations...
|
I wonder if we should normalize the version string coming in and cast to lowercase. I also wonder if we should just drop anything after the last number, like we do in the ES version checker. |
import expect from 'expect.js'; | ||
import sinon from 'sinon'; | ||
import nock from 'nock'; | ||
import glob from 'glob'; |
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.
Should be glob-all
here
When running I think it's trying to show the default usage syntax but failing. |
'based analytics and search dashboard for Elasticsearch.' | ||
); | ||
|
||
require('./list')(program); |
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.
I believe with the move to ES6 modules, we've gotten rid of require statements on client and server code now. If I'm right, you should be using import instead of require here, and elsewhere.
The file permissions on |
jenkins, test it |
29cbbd7
to
91cce98
Compare
import removeCommand from './remove'; | ||
|
||
let argv = process.env.kbnWorkerArgv ? JSON.parse(process.env.kbnWorkerArgv) : process.argv.slice(); | ||
let program = new Command('bin/kibana-plugin'); |
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.
I think this should be const
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.
The focus of my changes on this file were removing existing functionality from it. I didn't rewrite this file, so I didn't focus on it as a whole. Despite this, I'll make your suggested change.
One thing I wanted to note: the size of this pull request makes it really difficult to review effectively. You broke up the PR description into 5 different categories of changes - I'd recommend 5 different pull requests in the future. |
2307095
to
d65e7be
Compare
This will require hacking on the Command module we use for the cli. Opened #6577 to address this. |
f9b46b0
to
cbfcd56
Compare
@@ -0,0 +1,43 @@ | |||
import _ from 'lodash'; | |||
import pkg from '../utils/package_json'; | |||
import Command from '../cli/Command'; |
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 has to be lowercase - ../cli/command
With that last fix, the installer is now fully functional. LGTM! |
Is it intentional to allow people to install over existing plugins (without removing them first)? |
Err, nevermind. It failed eventually. |
LGTM |
resolves #5761
Plugin pack installer for kibana
modified existing entry point
Removed the plugin commands from the ./bin/kibana entry point
new entry point
All plugin commands are now executed from the
./bin/kibana-plugin
entry point:List command changes
Remove command changes
Install command changes
<plugin>
specifiedhttps://download.elastic.co/packs/<plugin>/<plugin>-<kibana version>.zip