opendev
/
gerrit
Code Issues Proposed changes

Disentangle ListPlugins and PluginLsCommand 

ListPlugins has a lot of code that is only used for printing the
output to stdout, which is only used by PluginLsCommand.

Simplify ListPlugins to only return the map of plugins, and move the
logic for displaying the output into PluginLsCommand.

PluginLsCommand does not have access to the Plugin objects, but only
to what is returned in PluginInfo instances. Extend that to include
the filename, which was previously accessible to ListPlugins.

This change removes support for the format option from the list
plugins REST API endpoint.

Change-Id: I0c352e587d5f0e8d524ae5b7322e1feec9434d58
This commit is contained in:
David Pursehouse 2017-08-04 21:03:28 +02:00
parent c1eb61280d
commit 4a64821338
6 changed files with 73 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
Documentation/rest-api-plugins.txt
View File

@ -47,6 +47,7 @@ by plugin ID.
"delete-project": {
"id": "delete-project",
"index_url": "plugins/delete-project/",
"filename": "delete-project.jar",
"version": "2.9-SNAPSHOT"
}
}
@ -73,11 +74,13 @@ List all plugins including those that are disabled.
"delete-project": {
"id": "delete-project",
"index_url": "plugins/delete-project/",
"filename": "delete-project.jar",
"version": "2.9-SNAPSHOT"
},
"reviewers-by-blame": {
"id": "reviewers-by-blame",
"index_url": "plugins/reviewers-by-blame/",
"filename": "reviewers-by-blame.jar",
"version": "2.9-SNAPSHOT",
"disabled": true
}
@ -105,6 +108,7 @@ Query the first plugin in the plugin list:
"delete-project": {
"id": "delete-project",
"index_url": "plugins/delete-project/",
"filename": "delete-project.jar",
"version": "2.9-SNAPSHOT"
}
}
@ -134,6 +138,7 @@ List all plugins that start with `delete`:
"delete-project": {
"id": "delete-project",
"index_url": "plugins/delete-project/",
"filename": "delete-project.jar",
"version": "2.9-SNAPSHOT"
}
}
@ -168,11 +173,13 @@ List all plugins that match regex `some.*plugin`:
"some-plugin": {
"id": "some-plugin",
"index_url": "plugins/some-plugin/",
"filename": "some-plugin.jar",
"version": "2.9-SNAPSHOT"
},
"some-other-plugin": {
"id": "some-other-plugin",
"index_url": "plugins/some-other-plugin/",
"filename": "some-other-plugin.jar",
"version": "2.9-SNAPSHOT"
}
}
@ -200,6 +207,7 @@ Query the second plugin in the plugin list:
"reviewers-by-blame": {
"id": "reviewers-by-blame",
"index_url": "plugins/reviewers-by-blame/",
"filename": "reviewers-by-blame.jar",
"version": "2.9-SNAPSHOT",
"disabled": true
}
@ -229,6 +237,7 @@ List all plugins that match substring `project`:
"delete-project": {
"id": "delete-project",
"index_url": "plugins/delete-project/",
"filename": "delete-project.jar",
"version": "2.9-SNAPSHOT"
}
}
@ -429,6 +438,7 @@ The `PluginInfo` entity describes a plugin.
|`id` ||The ID of the plugin.
|`version` ||The version of the plugin.
|`index_url`|optional|URL of the plugin's default page.
|`filename` |optional|The plugin's filename.
|`disabled` |not set if `false`|Whether the plugin is disabled.
|=======================

1
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/plugin/PluginIT.java
View File

@ -68,6 +68,7 @@ public class PluginIT extends AbstractDaemonTest {
assertThat(info.id).isEqualTo(name);
assertThat(info.version).isEqualTo(pluginVersion(plugin));
assertThat(info.indexUrl).isEqualTo(String.format("plugins/%s/", name));
assertThat(info.filename).isEqualTo(plugin);
assertThat(info.disabled).isNull();
}
assertPlugins(list().get(), PLUGINS);

4
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/PluginInfo.java
View File

@ -18,12 +18,14 @@ public class PluginInfo {
public final String id;
public final String version;
public final String indexUrl;
public final String filename;
public final Boolean disabled;
public PluginInfo(String id, String version, String indexUrl, Boolean disabled) {
public PluginInfo(String id, String version, String indexUrl, String filename, Boolean disabled) {
this.id = id;
this.version = version;
this.indexUrl = indexUrl;
this.filename = filename;
this.disabled = disabled;
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/api/plugins/PluginsImpl.java
View File

@ -66,7 +66,7 @@ public class PluginsImpl implements Plugins {
list.setMatchPrefix(this.getPrefix());
list.setMatchSubstring(this.getSubstring());
list.setMatchRegex(this.getRegex());
return list.apply();
return list.apply(TopLevelResource.INSTANCE);
}
};
}

80
gerrit-server/src/main/java/com/google/gerrit/server/plugins/ListPlugins.java
View File

@ -15,11 +15,8 @@
package com.google.gerrit.server.plugins;
import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.toList;
import com.google.common.base.Strings;
import com.google.common.collect.Streams;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.extensions.annotations.RequiresCapability;
import com.google.gerrit.extensions.common.PluginInfo;
@ -27,16 +24,12 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.server.OutputFormat;
import com.google.gson.reflect.TypeToken;
import com.google.inject.Inject;
import java.io.PrintWriter;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.kohsuke.args4j.Option;
@ -52,10 +45,6 @@ public class ListPlugins implements RestReadView<TopLevelResource> {
private String matchSubstring;
private String matchRegex;
@Deprecated
@Option(name = "--format", usage = "(deprecated) output format")
private OutputFormat format = OutputFormat.TEXT;
@Option(
name = "--all",
aliases = {"-a"},
@ -115,29 +104,8 @@ public class ListPlugins implements RestReadView<TopLevelResource> {
this.pluginLoader = pluginLoader;
}
public OutputFormat getFormat() {
return format;
}
public ListPlugins setFormat(OutputFormat fmt) {
this.format = fmt;
return this;
}
@Override
public Object apply(TopLevelResource resource) throws BadRequestException {
format = OutputFormat.JSON;
return display(null);
}
public SortedMap<String, PluginInfo> apply() throws BadRequestException {
format = OutputFormat.JSON;
return display(null);
}
public SortedMap<String, PluginInfo> display(@Nullable PrintWriter stdout)
throws BadRequestException {
SortedMap<String, PluginInfo> output = new TreeMap<>();
public SortedMap<String, PluginInfo> apply(TopLevelResource resource) throws BadRequestException {
Stream<Plugin> s = Streams.stream(pluginLoader.getPlugins(all));
if (matchPrefix != null) {
checkMatchOptions(matchSubstring == null && matchRegex == null);
@ -158,38 +126,7 @@ public class ListPlugins implements RestReadView<TopLevelResource> {
if (limit > 0) {
s = s.limit(limit);
}
List<Plugin> plugins = s.collect(toList());
if (!format.isJson()) {
stdout.format("%-30s %-10s %-8s %s\n", "Name", "Version", "Status", "File");
stdout.print(
"-------------------------------------------------------------------------------\n");
}
for (Plugin p : plugins) {
PluginInfo info = toPluginInfo(p);
if (format.isJson()) {
output.put(p.getName(), info);
} else {
stdout.format(
"%-30s %-10s %-8s %s\n",
p.getName(),
Strings.nullToEmpty(info.version),
p.isDisabled() ? "DISABLED" : "ENABLED",
p.getSrcFile().getFileName());
}
}
if (stdout == null) {
return output;
} else if (format.isJson()) {
format
.newGson()
.toJson(output, new TypeToken<Map<String, PluginInfo>>() {}.getType(), stdout);
stdout.print('\n');
}
stdout.flush();
return null;
return new TreeMap<>(s.collect(Collectors.toMap(p -> p.getName(), p -> toPluginInfo(p))));
}
private void checkMatchOptions(boolean cond) throws BadRequestException {
@ -202,13 +139,20 @@ public class ListPlugins implements RestReadView<TopLevelResource> {
String id;
String version;
String indexUrl;
String filename;
Boolean disabled;
id = Url.encode(p.getName());
version = p.getVersion();
disabled = p.isDisabled() ? true : null;
indexUrl = p.getSrcFile() != null ? String.format("plugins/%s/", p.getName()) : null;
if (p.getSrcFile() != null) {
indexUrl = String.format("plugins/%s/", p.getName());
filename = p.getSrcFile().getFileName().toString();
} else {
indexUrl = null;
filename = null;
}
return new PluginInfo(id, version, indexUrl, disabled);
return new PluginInfo(id, version, indexUrl, filename, disabled);
}
}

49
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PluginLsCommand.java
View File

@ -16,25 +16,68 @@ package com.google.gerrit.sshd.commands;
import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE;
import com.google.common.base.Strings;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.extensions.annotations.RequiresCapability;
import com.google.gerrit.extensions.common.PluginInfo;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.server.OutputFormat;
import com.google.gerrit.server.plugins.ListPlugins;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.gson.reflect.TypeToken;
import com.google.inject.Inject;
import java.util.Map;
import org.kohsuke.args4j.Option;
@RequiresCapability(GlobalCapability.VIEW_PLUGINS)
@CommandMetaData(name = "ls", description = "List the installed plugins", runsAt = MASTER_OR_SLAVE)
final class PluginLsCommand extends SshCommand {
@Inject private ListPlugins impl;
@Inject private ListPlugins list;
@Option(
name = "--all",
aliases = {"-a"},
usage = "List all plugins, including disabled plugins"
)
private boolean all;
@Option(name = "--format", usage = "output format")
private OutputFormat format = OutputFormat.TEXT;
@Override
public void run() throws Exception {
impl.display(stdout);
list.setAll(all);
Map<String, PluginInfo> output = list.apply(TopLevelResource.INSTANCE);
if (format.isJson()) {
format
.newGson()
.toJson(output, new TypeToken<Map<String, PluginInfo>>() {}.getType(), stdout);
stdout.print('\n');
} else {
stdout.format("%-30s %-10s %-8s %s\n", "Name", "Version", "Status", "File");
stdout.print(
"-------------------------------------------------------------------------------\n");
for (Map.Entry<String, PluginInfo> p : output.entrySet()) {
PluginInfo info = p.getValue();
stdout.format(
"%-30s %-10s %-8s %s\n",
p.getKey(),
Strings.nullToEmpty(info.version),
status(info.disabled),
Strings.nullToEmpty(info.filename));
}
}
stdout.flush();
}
private String status(Boolean disabled) {
return disabled != null && disabled.booleanValue() ? "DISABLED" : "ENABLED";
}
@Override
protected void parseCommandLine() throws UnloggedFailure {
parseCommandLine(impl);
parseCommandLine(this);
}
}