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

Gradle plugin support for dependencies in plugin descriptor properties #17178

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+165 −9
Open

Gradle plugin support for dependencies in plugin descriptor properties #17178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion buildSrc/src/main/groovy/org/opensearch/gradle/plugin/PluginBuildPlugin.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,15 @@
'customFolderName' : extension1.customFolderName,
'extendedPlugins' : extension1.extendedPlugins.join(','),
'hasNativeController' : extension1.hasNativeController,
'requiresKeystore' : extension1.requiresKeystore
'requiresKeystore' : extension1.requiresKeystore,
'dependencies' : extension1.dependencies
]

// Clear opensearch version if dependencies are specified
if (extension1.dependencies != null && !extension1.dependencies.trim().isEmpty()) {
properties.put('opensearchVersion', '');
}

Check warning on line 123 in buildSrc/src/main/groovy/org/opensearch/gradle/plugin/PluginBuildPlugin.groovy

View check run for this annotation

Codecov / codecov/patch

buildSrc/src/main/groovy/org/opensearch/gradle/plugin/PluginBuildPlugin.groovy#L122-L123 

Added lines #L122 - L123 were not covered by tests

project.tasks.named('pluginProperties').configure {
expand(properties)
inputs.properties(properties)
Expand Down
10 changes: 10 additions & 0 deletions buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@

private String customFolderName = "";

private String dependencies = "";

/** Other plugins this plugin extends through SPI */
private List<String> extendedPlugins = new ArrayList<>();

Expand Down Expand Up @@ -86,6 +88,14 @@
this.customFolderName = customFolderName;
}

public String getDependencies() {
return dependencies;
}

public void setDependencies(String dependencies) {
this.dependencies = dependencies;
}

Check warning on line 97 in buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java

View check run for this annotation

Codecov / codecov/patch

buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java#L96-L97 

Added lines #L96 - L97 were not covered by tests

public String getName() {
return name == null ? project.getName() : name;
}
Expand Down
3 changes: 3 additions & 0 deletions buildSrc/src/main/resources/plugin-descriptor.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,6 @@ extended.plugins=${extendedPlugins}
#
# 'has.native.controller': whether or not the plugin has a native controller
has.native.controller=${hasNativeController}
#
# 'dependencies': any dependencies specified by the plugin
dependencies=${dependencies}
Copy link
Member

Choose a reason for hiding this comment

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

I see we are adding dependencies as another parameter to plugin-descriptor.properties. Why not re-use opensearch.version ?
PluginInfo.java[1] already processes semver range. May be we might have to change opensearch-plugin install cli but thats pretty much it.

[1] https://github.com/opensearch-project/OpenSearch/blame/e93791bc97b9791c422fa21e365df340cb8e2aa2/server/src/main/java/org/opensearch/plugins/PluginInfo.java#L291

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Support for 'dependencies' as another parameter was already introduced in #11441. After that change, Opensearch expects either 'opensearch.version' or 'dependencies' parameter in the plugin descriptor properties file.
https://github.com/opensearch-project/OpenSearch/blame/e93791bc97b9791c422fa21e365df340cb8e2aa2/server/src/main/java/org/opensearch/plugins/PluginInfo.java#L291

Today, the semver support only works if plugin descriptor file has a 'dependencies' section with 'opensearch' as the only supported dependency.
Example test showing a sample plugin descriptor properties file - https://github.com/opensearch-project/OpenSearch/pull/11441/files#diff-1311b3459a8afb363dd9ff9d584421c3a8292fed4b0d25535f10a2b4a9c98bc7R94

With this commit (17178), we are adding capability for the opensearch-plugin to add this new parameter to the plugin-descriptor.properties file that it generates, so that users can leverage semver feature.

Here is the discussion from one of the discarded PRs regarding adding support for new 'dependencies' section - #6837 (review)

14 changes: 12 additions & 2 deletions buildSrc/src/test/java/org/opensearch/gradle/plugin/PluginBuildPluginTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@
import org.gradle.api.tasks.bundling.AbstractArchiveTask;
import org.gradle.testfixtures.ProjectBuilder;
import org.junit.Before;
import org.junit.Ignore;

import java.util.Map;
import java.util.stream.Collectors;

import org.mockito.Mockito;
Expand Down Expand Up @@ -72,7 +72,6 @@ public void testApply() {
});
}

@Ignore("https://github.com/elastic/elasticsearch/issues/47123")
public void testApplyWithAfterEvaluate() {
project.getExtensions().getExtraProperties().set("bwcVersions", Mockito.mock(BwcVersions.class));
project.getPlugins().apply(PluginBuildPlugin.class);
Expand All @@ -88,5 +87,16 @@ public void testApplyWithAfterEvaluate() {
"Task to generate notice not created: " + project.getTasks().stream().map(Task::getPath).collect(Collectors.joining(", ")),
project.getTasks().findByName("generateNotice")
);
Map<String, Object> taskProperties = project.getTasks().findByName("pluginProperties").getInputs().getProperties();
assertTrue(taskProperties.containsKey("description"));
assertTrue(taskProperties.containsKey("version"));
assertTrue(taskProperties.containsKey("name"));
assertTrue(taskProperties.containsKey("classname"));
assertTrue(taskProperties.containsKey("javaVersion"));
assertTrue(taskProperties.containsKey("opensearchVersion"));
assertTrue(taskProperties.containsKey("customFolderName"));
assertTrue(taskProperties.containsKey("extendedPlugins"));
assertTrue(taskProperties.containsKey("hasNativeController"));
assertTrue(taskProperties.containsKey("dependencies"));
}
}
8 changes: 6 additions & 2 deletions ...tion/tools/plugin-cli/src/test/java/org/opensearch/plugins/InstallPluginCommandTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,9 @@ static void writePlugin(String name, Path structure, String... additionalProps)
"java.version",
System.getProperty("java.specification.version"),
"classname",
"FakePlugin"
"FakePlugin",
"dependencies",
""
),
Arrays.stream(additionalProps)
).toArray(String[]::new);
Expand All @@ -300,7 +302,9 @@ static void writePlugin(String name, Path structure, SemverRange opensearchVersi
"java.version",
System.getProperty("java.specification.version"),
"classname",
"FakePlugin"
"FakePlugin",
"opensearch.version",
""
),
Arrays.stream(additionalProps)
).toArray(String[]::new);
Expand Down
10 changes: 7 additions & 3 deletions server/src/main/java/org/opensearch/plugins/PluginInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,19 +289,19 @@ public static PluginInfo readFromProperties(final Path path) throws IOException

final String opensearchVersionString = propsMap.remove("opensearch.version");
final String dependenciesValue = propsMap.remove("dependencies");
if (opensearchVersionString == null && dependenciesValue == null) {
if (isBlank(opensearchVersionString) && isBlank(dependenciesValue)) {
throw new IllegalArgumentException(
"Either [opensearch.version] or [dependencies] property must be specified for the plugin [" + name + "]"
);
}
if (opensearchVersionString != null && dependenciesValue != null) {
if (!isBlank(opensearchVersionString) && !isBlank(dependenciesValue)) {
throw new IllegalArgumentException(
"Only one of [opensearch.version] or [dependencies] property can be specified for the plugin [" + name + "]"
);
}

final List<SemverRange> opensearchVersionRanges = new ArrayList<>();
if (opensearchVersionString != null) {
if (!isBlank(opensearchVersionString)) {
opensearchVersionRanges.add(SemverRange.fromString(opensearchVersionString));
} else {
Map<String, String> dependenciesMap;
Expand Down Expand Up @@ -592,4 +592,8 @@ public String toString(String prefix) {
.append(customFolderName);
return information.toString();
}

private static boolean isBlank(String s) {
return s == null || s.trim().isEmpty();
}
}
120 changes: 119 additions & 1 deletion server/src/test/java/org/opensearch/plugins/PluginInfoTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,68 @@ public void testReadFromPropertiesOpenSearchVersionAndDependenciesMissing() thro
);
}

public void testReadFromPropertiesOpenSearchVersionEmpty() throws Exception {
Path pluginDir = createTempDir().resolve("fake-plugin");
PluginTestUtil.writePluginProperties(
pluginDir,
"description",
"fake desc",
"opensearch.version",
"",
"name",
"my_plugin",
"version",
"1.0"
);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> PluginInfo.readFromProperties(pluginDir));
assertThat(
e.getMessage(),
containsString("Either [opensearch.version] or [dependencies] property must be specified for the plugin ")
);
}

public void testReadFromPropertiesDependenciesEmpty() throws Exception {
Path pluginDir = createTempDir().resolve("fake-plugin");
PluginTestUtil.writePluginProperties(
pluginDir,
"description",
"fake desc",
"dependencies",
"",
"name",
"my_plugin",
"version",
"1.0"
);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> PluginInfo.readFromProperties(pluginDir));
assertThat(
e.getMessage(),
containsString("Either [opensearch.version] or [dependencies] property must be specified for the plugin ")
);
}

public void testReadFromPropertiesOpenSearchVersionAndDependenciesEmpty() throws Exception {
Path pluginDir = createTempDir().resolve("fake-plugin");
PluginTestUtil.writePluginProperties(
pluginDir,
"description",
"fake desc",
"opensearch.version",
"",
"dependencies",
"",
"name",
"my_plugin",
"version",
"1.0"
);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> PluginInfo.readFromProperties(pluginDir));
assertThat(
e.getMessage(),
containsString("Either [opensearch.version] or [dependencies] property must be specified for the plugin ")
);
}

public void testReadFromPropertiesWithDependenciesAndOpenSearchVersion() throws Exception {
Path pluginDir = createTempDir().resolve("fake-plugin");
PluginTestUtil.writePluginProperties(
Expand All @@ -202,6 +264,62 @@ public void testReadFromPropertiesWithDependenciesAndOpenSearchVersion() throws
);
}

public void testReadFromPropertiesDependenciesEmptyOpenSearchVersionNotEmpty() throws Exception {
Path pluginDir = createTempDir().resolve("fake-plugin");
PluginTestUtil.writePluginProperties(
pluginDir,
"description",
"fake desc",
"name",
"my_plugin",
"version",
"1.0",
"opensearch.version",
Version.CURRENT.toString(),
"java.version",
System.getProperty("java.specification.version"),
"classname",
"FakePlugin",
"dependencies",
""
);
PluginInfo info = PluginInfo.readFromProperties(pluginDir);
assertEquals("my_plugin", info.getName());
assertEquals("fake desc", info.getDescription());
assertEquals("1.0", info.getVersion());
assertEquals("FakePlugin", info.getClassname());
assertEquals(Version.CURRENT.toString(), info.getOpenSearchVersionRanges().get(0).toString());
assertThat(info.getExtendedPlugins(), empty());
}

public void testReadFromPropertiesOpenSearchVersionEmptyDependenciesNotEmpty() throws Exception {
Path pluginDir = createTempDir().resolve("fake-plugin");
PluginTestUtil.writePluginProperties(
pluginDir,
"description",
"fake desc",
"name",
"my_plugin",
"version",
"1.0",
"dependencies",
"{opensearch:\"~" + Version.CURRENT.toString() + "\"}",
"java.version",
System.getProperty("java.specification.version"),
"classname",
"FakePlugin",
"opensearch.version",
""
);
PluginInfo info = PluginInfo.readFromProperties(pluginDir);
assertEquals("my_plugin", info.getName());
assertEquals("fake desc", info.getDescription());
assertEquals("1.0", info.getVersion());
assertEquals("FakePlugin", info.getClassname());
assertEquals("~" + Version.CURRENT.toString(), info.getOpenSearchVersionRanges().get(0).toString());
assertThat(info.getExtendedPlugins(), empty());
}

public void testReadFromPropertiesJavaVersionMissing() throws Exception {
Path pluginDir = createTempDir().resolve("fake-plugin");
PluginTestUtil.writePluginProperties(
Expand Down Expand Up @@ -609,7 +727,7 @@ public void testInvalidRangeInDependencies() throws Exception {
expectThrows(NumberFormatException.class, () -> PluginInfo.readFromProperties(pluginDir));
}

public void testhMultipleOpenSearchRangesInDependencies() throws Exception {
public void testMultipleOpenSearchRangesInDependencies() throws Exception {
Path pluginDir = createTempDir().resolve("fake-plugin");
PluginTestUtil.writePluginProperties(
pluginDir,
Expand Down
Loading