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.

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

Adds ability to list installed plugins from CLI #5920

Merged
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 2 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
18 changes: 18 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,18 @@
const fs = require('fs');

module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use an es6 export instead of commonjs, so:

export function list(settings, logger) {

instead of module.exports = {...}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will make this change in this file. Should I also make the same change in the other plugin_*.js files in this directory (that aren't in the scope of this PR)?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to modify the other files. We plan to run through the entire codebase before Kibana 5 to update the majority of the existing commonjs module stuff.

list: list
};

function list(settings, logger) {
fs.readdir(settings.pluginDir, function (err, files) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For cli stuff like this, it's generally best to use the *Sync functions so that exceptions will be thrown on error, so this becomes:

const files = fs.readdirSync(settings.pluginDir);
// ...


var pluginFiles = files.filter(function (file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally don't want to use var at all anymore. const will work in this case.

Another option is to skip a new variable entirely and just chain your filter and forEach together:

files
.filter(function (file) {
  // ...
})
.forEach(function (pluginFile) {
  // ...
});

return file[0] !== '.';
});

pluginFiles.forEach(function (pluginFile) {
logger.log(pluginFile);
});
});
}
8 changes: 6 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 @@ -84,8 +84,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 || (options.install && options.remove && options.list)) {
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 conditional needs to get a bit more robust. It seems to be verifying that the user isn't trying to specify conflicting commands, which can be represented with a single conditional when there are only two possibilities. Now that there are three possibilities, I assume we want to verify that the user isn't trying to perform any combination of those commands, which will take more than a simple conditional to verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't quite clear on the original intent here but what you say seems right. Will fix. Thanks!

throw new Error('Please specify either --install, --remove, or --list.');
}

settings.pluginDir = options.pluginDir;
Expand Down