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

Plugin pack installer #6402

Merged
merged 19 commits into from
Mar 24, 2016
Merged

Conversation

BigFunger
Copy link
Contributor

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  [options]                 List installed plugins
install  [options] <plugin/url> Install a plugin
remove  [options] <plugin>      Remove a plugin
help  <command>                 Get the help for a specific command

List command changes

  • Refactored to use its own settings object/parser
  • Now ignores folders that begin with a period.
  • Added tests

Remove command changes

  • Refactored to use its own settings object/parser

Install command changes

  • Refactored to use its own settings object/parser
  • No longer supports tar.gz format. Only supports zip format
  • Attempts to download the specified plugin from:
    • exact <plugin> specified
    • https://download.elastic.co/packs/<plugin>/<plugin>-<kibana version>.zip
  • Expects the pack zip file if the plugin name is 'xpack' to be structured:
any-named-zip-archive.zip
|__kibana
| |__xpack
| | |__package.json
| | |__...
  • Expects the package.json file to contain a name (xpack) that matches the containing folder in the archive exactly and a version that matches the version of kibana exactly.
  • Currently there is no platform-specific logic implemented
  • Currently will only install the first plugin that it finds in the kibana folder if more are in the archive.
  • Logic has been added to make implement platform specific and multiple plugins easier for later versions.

@w33ble
Copy link
Contributor

w33ble commented Mar 3, 2016

Initial observations...

  • remove and list commands should be checking to ensure that they're dealing with directories, currently they list and remove files
  • remove doesn't work when folder names have a space
  • related: install should probably validate the name in the package.json and fail if it contains a space - npm packages can't have a space in their name anyway

@w33ble
Copy link
Contributor

w33ble commented Mar 3, 2016

Plugin installation was unsuccessful due to error "Incorrect version in plugin [shield]. Expected [5.0.0-snapshot]; found [5.0.0-SNAPSHOT]"

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

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

@w33ble
Copy link
Contributor

w33ble commented Mar 3, 2016

When running help on a command that doesn't exist (ie. kibana-plugin help funger), there's a stray undefined in the output that should be removed.

I think it's trying to show the default usage syntax but failing.

'based analytics and search dashboard for Elasticsearch.'
);

require('./list')(program);
Copy link
Contributor

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.

@w33ble
Copy link
Contributor

w33ble commented Mar 3, 2016

The file permissions on bin/kibana-plugin need to include the +x flag. I have no idea how to achieve that on Windows though...

@w33ble w33ble assigned BigFunger and unassigned w33ble Mar 4, 2016
@BigFunger
Copy link
Contributor Author

jenkins, test it

import removeCommand from './remove';

let argv = process.env.kbnWorkerArgv ? JSON.parse(process.env.kbnWorkerArgv) : process.argv.slice();
let program = new Command('bin/kibana-plugin');
Copy link
Contributor

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

Copy link
Contributor Author

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.

@epixa
Copy link
Contributor

epixa commented Mar 10, 2016

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.

@BigFunger
Copy link
Contributor Author

@epixa said

Rather than error if the user doesn't specify a command in help, I think we should render the default text. Basically, I'd expect both of these to be valid ways to retrieve help text about the cli:

This will require hacking on the Command module we use for the cli. Opened #6577 to address this.

@@ -0,0 +1,43 @@
import _ from 'lodash';
import pkg from '../utils/package_json';
import Command from '../cli/Command';
Copy link
Contributor

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

@w33ble
Copy link
Contributor

w33ble commented Mar 23, 2016

With that last fix, the installer is now fully functional. LGTM!

@epixa
Copy link
Contributor

epixa commented Mar 23, 2016

Is it intentional to allow people to install over existing plugins (without removing them first)?

@epixa
Copy link
Contributor

epixa commented Mar 23, 2016

Err, nevermind. It failed eventually.

@epixa
Copy link
Contributor

epixa commented Mar 23, 2016

LGTM

@epixa epixa assigned BigFunger and unassigned epixa Mar 23, 2016
@BigFunger BigFunger merged commit c649b49 into elastic:master Mar 24, 2016
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.

[plugin installer] cross stack plugin installing
5 participants