Remove nebula publish plugin usage #69027
Conversation
breskeby
commented
Feb 16, 2021
breskeby
commented
- instead setup publication on our own.
- replaces nebula with a elastic publication
breskeby
added
:Delivery/Build
|
Pinging @elastic/es-delivery (Team:Delivery) |
- instead setup publication on our own. - replaces nebula with a elastic publication
breskeby
force-pushed
the
remove-nebula-publish-usage
branch
from
c4d311c to
74b4d74
Compare
| private static void configurePublishing(Project project, PluginPropertiesExtension extension) { | ||
| if (project.plugins.hasPlugin(MavenPublishPlugin)) { | ||
| project.publishing.publications.nebula(MavenPublication).artifactId(extension.name) | ||
| project.publishing.publications.elastic(MavenPublication).artifactId(extension.name) |
There was a problem hiding this comment.
Yay! Now we don't have to explain wtf "nebula" means.
server/build.gradle
Outdated
| @@ -13,7 +13,7 @@ apply plugin: 'elasticsearch.internal-cluster-test' | |||
|
|
|||
| publishing { | |||
| publications { | |||
| nebula { | |||
| elastic(MavenPublication) { | |||
There was a problem hiding this comment.
Why is it necessary to specify the type here? Isn't that only when creating a new container object? In this context we're configuring an existing publication, no?
There was a problem hiding this comment.
yes. that was an oversight of an earlier implementation where I created the publication in an afterEvaluate. But I moved it to be done immediately to avoid exactly this. yeah we should only configure existing publications here.
|
|
||
| @Override | ||
| public void apply(Project project) { | ||
| project.getPluginManager().apply(BasePlugin.class); | ||
| project.getPluginManager().apply("nebula.maven-nebula-publish"); | ||
| project.getPlugins().apply(MavenPublishPlugin.class); |
There was a problem hiding this comment.
Shoudl we use the PluginManager version consistently here?
| extraProperties.set(MAVEN_JAR, Boolean.FALSE); | ||
|
|
||
| project.getPlugins().withType(JavaPlugin.class).configureEach(plugin -> extraProperties.set(MAVEN_JAR, Boolean.TRUE)); | ||
| project.getPluginManager().withPlugin("com.github.johnrengelman.shadow", plugin -> extraProperties.set(MAVEN_JAR, Boolean.TRUE)); |
There was a problem hiding this comment.
Shouldn't we be mutating the MAVEN_SHADOW property here instead? And really, is tracking these things via properties necessary? Couldn't we simply do configureWithShadowPlugin here when the plugin is applied rather than setting a flag then reading it in afterEvalutate?
There was a problem hiding this comment.
indeed. The general problem we deal here with is applying an either or pattern. If just the java plugin is applied then do this. If shadow plugin is applied do that.
I looked a bit deeper into this and since how we handle the shadow plugin here we actually could simplify this without the need for the extra properties and the after evaluate hook. pushed a fix for this
There was a problem hiding this comment.
Doesn't the shadow plugin also apply the java plugin? If so can't we just order it such that it's safe to always do the "java" configuration and we just override those bits if necessary when the shadow plugin gets applied? I'm quite certain this was how it behaved before.
There was a problem hiding this comment.
No it doesn't apply it. It only reacts on the java plugin being applied. But we're good here now I think
Simplify publication configuration
| @@ -116,25 +101,24 @@ private static void addNameAndDescriptiontoPom(Project project, NamedDomainObjec | |||
| } | |||
|
|
|||
| private static void configureWithShadowPlugin(Project project, MavenPublication publication) { | |||
| project.getPluginManager().withPlugin("com.github.johnrengelman.shadow", plugin -> { | |||
| ShadowExtension shadow = project.getExtensions().getByType(ShadowExtension.class); | |||
| shadow.component(publication); | |||
There was a problem hiding this comment.
Why were we doing this? Is this no longer necessary?
There was a problem hiding this comment.
we configure shadow jars to have the default naming. with having tis here we would basically add a 2nd artifact which causes gradle to declare the packaging to be a pom as we have already declared the java component as artifact in the withJava block.
- instead setup publication on our own. - replaces nebula with a elastic publication
