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

Conversation

Projects
None yet
5 participants
@rhamedy
Copy link
Contributor

commented Mar 2, 2019

Contributes to #34459

Converted

  • PluginPropertiesExtension.groovy -> java
  • Deleted the groovy files
@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Tried to convert the BuildPluginBuild.groovy to java and include it in this PR however, I hit a few roadblocks relating to

  • Replacing project.pluginProperties.extension.name equivalentwith project.getTasks().getByName("pluginProperties").getName()?
  • How to access module in project.integTestCluster.module(project)?
  • No publishing property on project for project.publishing.publications.nebula(MavenPublication).artifactId
  • A few others like this

I have paused working on BuildPluginBuild.groovy because of one too many question marks. Not sure if this PR can go through independent of BuildPluginBuild refactor?

@elasticmachine

This comment has been minimized.

Copy link

commented Mar 4, 2019

@atorok
Copy link
Contributor

left a comment

Thank you for your contribution @rhamedy !
We prefer small PRs so this is even better than having the plugin converted too.
I left a few comments. Also would be kind enough and add tests for these classes ?
We want all the java code to have tests.

@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

@atorok thank you for the review. I have addressed them in my recent commit as well as replied to some of your comments that need a reply.

I would definitely write tests! Is there a test class that I could use as a guide? I need a little sense of direction on how write tests 🙂

@atorok

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Thanks @rhamedy for the quick update. For tests I think you should look at GradleUnitTestCase and other classes that implement it.

@elasticmachine test this please

@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@atorok I have addressed the code review comments as well as added some tests. I could not find a class that I could use as baseline to write tests probably because the tests are different in other classes and this is plugin.

The test ensures tha the plugin-descriptor.properties file is picked up from classpath and it's substituted with props from extension and it's saved in the project build directory under templates.

Please let me know if any changes to be made 👍

@atorok

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@elasticmachine test this please

@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@rjernst and @atorok thank you for the review. Replied to the comments. Do you have any feedback on unit tests? It would be nice to tackle the review changes along with tests all at once.

@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@rjernst @atorok I pushed up my new changes taking into account the code review comments. I have removed the unecessary test/resources/plugin-descriptor.properties file and after throwing away the doLast everything works magically and according to my tests the templates and generated-resources directories have correct content 👍


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

This comment has been minimized.

Copy link
@atorok

atorok Mar 19, 2019

Contributor

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.

This comment has been minimized.

Copy link
@rhamedy

rhamedy Mar 19, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 20, 2019

Member

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.

This comment has been minimized.

Copy link
@atorok

atorok Mar 20, 2019

Contributor

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

@atorok

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

@elasticmachine test this please

@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@atorok @rjernst I have factored the changes requested and pushed the changes up. The tests that I have added are working 👍 however when running the command ./gradlw check I get the following error

> Task :modules:aggs-matrix-stats:pluginProperties FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':modules:aggs-matrix-stats:pluginProperties'.
> java.nio.file.FileAlreadyExistsException: /Users/raf/workspace/opensource-contributions/elastic/elasticsearch/modules/aggs-matrix-stats/build/templates/plugin-descriptor.properties

I double checked the modules:aggs-matrix-stats and unlike the exception the file modules/aggs-matrix-stats/build/templates/plugin-descriptor.properties is `present 🤔

@atorok

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

@elasticmachine test this please

@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

@atorok @rjernst please let me know if any further changes need to be made. I will clean up (squash) the commits after the PR is in mergeable state 🙂

@atorok

atorok approved these changes Mar 25, 2019

Copy link
Contributor

left a comment

LGTM no need to squash we do that on merge anyhow.

@rjernst

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

@elasticmachine test this please

@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

@atorok Done! Curious what's blocking merge of changes in this PR? 🤔

@rhamedy rhamedy force-pushed the rhamedy:plugin-properties-task-tojava branch from 758a406 to 03fa92f Apr 17, 2019

@atorok

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@elasticmachine test this please

@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@atorok pushed my changes! 👍

@atorok

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@elasticmachine test this please

@atorok

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@rhamedy looks like there are some failing tests

@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

@atorok the recent changes fix the test failures 👍

@rhamedy rhamedy changed the title Convert PluginPropertiesExtension & PluginPropertiesTask Groovy to Java Convert PluginPropertiesExtension Groovy to Java Apr 20, 2019

@atorok

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@elasticmachine test this please

@atorok

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Sorry for the delay @rhamedy I'll see to merge this as soon as the PR checks pass

Merge master and make version lazy
Merge remote-tracking branch 'origin/master' into plugin-properties-task-tojava
When the version is not set, use the project version bu do so lazily so
we don't snapshot it at construction time
@rhamedy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@atorok that would be awesome! 🙂

@atorok atorok merged commit 04df52a into elastic:master Jun 13, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.