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

Convert PluginPropertiesExtension Groovy to Java #39605

Merged
merged 14 commits into from
Jun 13, 2019
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ class PluginBuildPlugin extends BuildPlugin {
super.apply(project)

PluginPropertiesExtension extension = project.extensions.create(PLUGIN_EXTENSION_NAME, PluginPropertiesExtension, project)
project.pluginManager.apply('elasticsearch.esplugin')
rhamedy marked this conversation as resolved.
Show resolved Hide resolved
rhamedy marked this conversation as resolved.
Show resolved Hide resolved

configureDependencies(project)

// this afterEvaluate must happen before the afterEvaluate added by integTest creation,
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.gradle.plugin;

import org.gradle.api.Project;

import java.io.File;
import java.util.ArrayList;
import java.util.List;

/**
* A container for plugin properties that will be written to the plugin descriptor, for easy
* manipulation in the gradle DSL.
*/
public class PluginPropertiesExtension {
private String name;

private String version;

private String description;

private String classname;

/** Other plugins this plugin extends through SPI */
private List<String> extendedPlugins = new ArrayList<>();
rhamedy marked this conversation as resolved.
Show resolved Hide resolved

private boolean hasNativeController;

/** Indicates whether the plugin jar should be made available for the transport client. */
private boolean hasClientJar;

/** True if the plugin requires the elasticsearch keystore to exist, false otherwise. */
private boolean requiresKeystore;

/** A license file that should be included in the built plugin zip. */
private File licenseFile;

/**
* A notice file that should be included in the built plugin zip. This will be
* extended with notices from the {@code licenses/} directory.
*/
private File noticeFile;

private final Project project;

public PluginPropertiesExtension(Project project) {
this.name = project.getName();
this.version = project.getVersion().toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized this now, this is a problem present in the old code as well.
The way we assign this default makes it prone to ordering.
If the extension is created before project.version is assigned we won't pick it up.
We should have a null check in the getter instead and returnproject.version if version is null
This is fine also in a subsequent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can update the PluginPropertiesExtension.getVersion() to return project.getVersion().toString() if version is null 👍

Copy link
Member

Choose a reason for hiding this comment

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

It is not possible for version to be null. VersionProperties statically loads the version. BuildPlugin is initialized before any PluginPropertiesExtension, since it is applied by PluginBuildPlugin, which extends BuildPlugin. Adding null checks all over for version will only make the code more complicated to understand with no actual additional safety.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do this early on in the root level build.gradle

allprojects {
  group = 'org.elasticsearch'
  version = VersionProperties.elasticsearch
  description = "Elasticsearch subproject ${project.path}"
}

So indeed it's not possible for project.version to be null in our build,
but all bets are off for external plugins, there's no guarantee that it won't be null.
We might not want to be lenient here and throw an exception if the version is null, but I still think we should do it rather than leave it be an NPE.
Alternatively, we could hard code this to VersionProperties.elasticsearch as we only test building plugins where the version of build-tools matches that of elasticsearch.
External plugins can then have whatever project.version they like.
What do you think of this approach @rjernst

this.project = project;
}

public String getName() {
return name;
}

public void setName(String name) {
this.name = name;
}

public String getVersion() {
return version;
}

public void setVersion(String version) {
this.version = version;
}

public String getDescription() {
return description;
}

public void setDescription(String description) {
this.description = description;
}

public String getClassname() {
return classname;
}

public void setClassname(String classname) {
this.classname = classname;
}

public List<String> getExtendedPlugins() {
return this.extendedPlugins;
}

public boolean getHasNativeController() {
rhamedy marked this conversation as resolved.
Show resolved Hide resolved
return hasNativeController;
}

public void setHasNativeController(boolean hasNativeController) {
this.hasNativeController = hasNativeController;
}

public boolean isHasClientJar() {
return hasClientJar;
}

public void setHasClientJar(boolean hasClientJar) {
this.hasClientJar = hasClientJar;
}

public boolean isRequiresKeystore() {
return requiresKeystore;
}

public void setRequiresKeystore(boolean requiresKeystore) {
this.requiresKeystore = requiresKeystore;
}

public File getLicenseFile() {
return licenseFile;
}

public void setLicenseFile(File licenseFile) {
this.project.getExtensions().add("licenseFile", licenseFile);
this.licenseFile = licenseFile;
}

public File getNoticeFile() {
return noticeFile;
}

public void setNoticeFile(File noticeFile) {
this.project.getExtensions().add("noticeFile", noticeFile);
this.noticeFile = noticeFile;
}

public Project getProject() {
return project;
}

public void setExtendedPlugins(List<String> extendedPlugins) {
this.extendedPlugins = extendedPlugins;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.gradle.plugin;

import org.elasticsearch.gradle.test.GradleUnitTestCase;
import org.gradle.api.Project;
import org.gradle.api.plugins.JavaPlugin;
import org.gradle.testfixtures.ProjectBuilder;

public class PluginPropertiesExtensionTests extends GradleUnitTestCase {

public void testCreatingPluginPropertiesExtensionWithNameAndVersion() {
String projectName = "Test";
String projectVersion = "5.0";

PluginPropertiesExtension pluginPropertiesExtension =
new PluginPropertiesExtension(this.createProject(projectName, projectVersion));

assertEquals(projectName, pluginPropertiesExtension.getName());
assertEquals(projectVersion, pluginPropertiesExtension.getVersion());
}

public void testCreatingPluginPropertiesExtensionWithNameWithoutVersion() {
String projectName = "Test";

PluginPropertiesExtension pluginPropertiesExtension =
new PluginPropertiesExtension(this.createProject(projectName, null));

assertEquals(projectName, pluginPropertiesExtension.getName());
assertEquals("unspecified", pluginPropertiesExtension.getVersion());
}

private Project createProject(String projectName, String version) {
Project project = ProjectBuilder.builder().withName(projectName).build();
project.setVersion(version);

project.getPlugins().apply(JavaPlugin.class);

return project;
}
}