Skip to content

Adds ability to list installed plugins from CLI #5920

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.

Sign up for GitHub

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

Merged
epixa merged 14 commits into from
Jan 16, 2016
Merged

Adds ability to list installed plugins from CLI #5920

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8fda0f9
Adding CLI option for listing plugins
ycombinator Jan 15, 2016
0e14e41
Using switch-case instead of ifs
ycombinator Jan 15, 2016
ea0f2ef
Getting rid of var and chaining calls instead
ycombinator Jan 15, 2016
d09f83b
Using es6 export
ycombinator Jan 15, 2016
2e9f00f
Using synch flavor of readdir to bubble up exception
ycombinator Jan 15, 2016
5bfccda
Fixing logic error
ycombinator Jan 15, 2016
42334e3
Removing unnecessary filter
ycombinator Jan 15, 2016
cac0f06
Another variable bites the dust
ycombinator Jan 15, 2016
ff15794
Simplifying using intersection from lodash
ycombinator Jan 15, 2016
220fc8b
Using import with proper semantics
ycombinator Jan 15, 2016
b9a8098
Fixing description of test
ycombinator Jan 16, 2016
cbc3350
Adding test for list option
ycombinator Jan 16, 2016
f8ba6ca
Updating error message tests to account for new "list" option
ycombinator Jan 16, 2016
13c0ccc
Better removal of options set by previous tests
ycombinator Jan 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/cli/plugin/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const fromRoot = utils('fromRoot');
const settingParser = require('./setting_parser');
const installer = require('./plugin_installer');
const remover = require('./plugin_remover');
const lister = require('./plugin_lister');
const pluginLogger = require('./plugin_logger');

export default function pluginCli(program) {
Expand All @@ -18,18 +19,24 @@ export default function pluginCli(program) {

const logger = pluginLogger(settings);

if (settings.action === 'install') {
installer.install(settings, logger);
}
if (settings.action === 'remove') {
remover.remove(settings, logger);
switch (settings.action) {
case 'install':
installer.install(settings, logger);
break;
case 'remove':
remover.remove(settings, logger);
break;
case 'list':
lister.list(settings, logger);
break;
}
}

program
.command('plugin')
.option('-i, --install <org>/<plugin>/<version>', 'The plugin to install')
.option('-r, --remove <plugin>', 'The plugin to remove')
.option('-l, --list', 'List installed plugins')
.option('-q, --quiet', 'Disable all process messaging except errors')
.option('-s, --silent', 'Disable all process messaging')
.option('-u, --url <url>', 'Specify download url')
Expand Down
12 changes: 12 additions & 0 deletions src/cli/plugin/plugin_lister.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const fs = require('fs');

export function list(settings, logger) {
const files = fs.readdirSync(settings.pluginDir);
files

@epixa epixa Jan 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for not catching this in the first go around, but I didn't notice it until you chained these together. If you're chaining anyway, you might as well ditch the local variable entirely:

fs.readdirSync(settings.pluginDir)
.filter(...)
.forEach(...)

@ycombinator ycombinator Jan 15, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okie doke. Changing.

.filter(function (file) {
return file[0] !== '.';

@epixa epixa Jan 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you only need the first value of the file array, I'd recommend destructuring the array argument so this conditional becomes a bit more expressive:

.filter(function([ fileName ]) {
  return fileName !== '.';
});

@ycombinator ycombinator Jan 15, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think I can remove this filter altogether. It was meant to check if the first character of a filename is . because in the installedPlugins source directory there is a .empty file (presumably so the directory is preserved in git). From what I can tell this .empty file does not actually get deployed as part of Kibana.

@epixa epixa Jan 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I totally misunderstood what this filter was even doing. Remove it if you think that's OK, but you definitely should not try to take my advice on this.

If you do need to keep the check, I'd recommend using lodash's startsWith function instead, which would just make it a bit more clear that we're dealing with the first character of a string rather than the first element of an array.

})
.forEach(function (pluginFile) {
logger.log(pluginFile);
});
}
18 changes: 16 additions & 2 deletions src/cli/plugin/setting_parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ export default function createSettingParser(options) {
return 'https://download.elastic.co/' + settings.organization + '/' + settings.package + '/' + filename;
}

function areMultipleOptionsChosen(options, choices) {
let numChosen = 0;
choices.forEach(function (choice) {
if (options[choice]) {
++numChosen;
}
});
return (numChosen > 1);

@epixa epixa Jan 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend doing this by checking the length of the intersection rather than imperatively counting the duplicates. You can import intersection from the lodash module, and then this function becomes:

function areMultipleOptionsChosen(options, choices) {
  return intersection(Object.keys(options), choices).length > 1;
}

@ycombinator ycombinator Jan 15, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful. Changing. Thanks!

}

function parse() {
let parts;
let settings = {
Expand Down Expand Up @@ -84,8 +94,12 @@ export default function createSettingParser(options) {
settings.package = parts.shift();
}

if (!settings.action || (options.install && options.remove)) {
throw new Error('Please specify either --install or --remove.');
if (options.list) {
settings.action = 'list';
}

if (!settings.action || areMultipleOptionsChosen(options, [ 'install', 'remove', 'list' ])) {
throw new Error('Please specify either --install, --remove, or --list.');
}

settings.pluginDir = options.pluginDir;
Expand Down