Use the project.group value for Zip POM groupId value

[lukas-vlcek]

Description

When publishing Zip POM the groupId value was hard-coded to org.opensearch.plugin value which works fine for existing core plugins but is not convenient for other plugins (such as community plugins maintained in independent repositories).

This PR introduces a main change in where the value for the groupId is taken from. Instead of using a hard-coded value it is taken from the project.group value.

This PR also brings a major rework of tests in PublishTests class. Individual testing scenarios are driven by "real" gradle building scripts (utilizing java-gradle-plugin gradle plugin).

Issues Resolved

Closes #3692

Check List

• [x] New functionality includes testing.
  • [x] All tests pass
• [ ] New functionality has been documented.
  • [x] New functionality has javadoc added
• [x] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Lukáš Vlček [email protected]

[lukas-vlcek]

Before merging the opensearch.plugin part of the documentation will need some update as well.

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/1549/
• CommitID: e02a7ffefe6b94f30617fb381af027cbfeb4853f

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/1550/
• CommitID: 3e1e547853360184093858a305b58815d8e64c91

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS :white_check_mark:
• URL: https://build.ci.opensearch.org/job/gradle-check/1551/
• CommitID: 6beec00b94190c547f3eac4056c24b845f5cbe1b

[codecov-commenter]

Codecov Report

Merging #4156 (2234a97) into main (1bfabed) will increase coverage by 0.10%. The diff coverage is 0.00%.

:exclamation: Current head 2234a97 differs from pull request most recent head 48c132f. Consider uploading reports for the commit 48c132f to get more accurate results

@@             Coverage Diff              @@
##               main    #4156      +/-   ##
============================================
+ Coverage     70.65%   70.76%   +0.10%     
+ Complexity    57230    57227       -3     
============================================
  Files          4610     4610              
  Lines        275105   275099       -6     
  Branches      40283    40284       +1     
============================================
+ Hits         194376   194663     +287     
+ Misses        64450    64092     -358     
- Partials      16279    16344      +65     

Impacted Files	Coverage Δ
.../java/org/opensearch/gradle/pluginzip/Publish.java	0.00% <0.00%> (-58.00%)	:arrow_down:
...luster/routing/allocation/RoutingExplanations.java	41.37% <0.00%> (-58.63%)	:arrow_down:
...a/org/opensearch/tasks/TaskCancelledException.java	50.00% <0.00%> (-50.00%)	:arrow_down:
...cluster/coordination/PendingClusterStateStats.java	20.00% <0.00%> (-48.00%)	:arrow_down:
.../admin/cluster/reroute/ClusterRerouteResponse.java	55.00% <0.00%> (-45.00%)	:arrow_down:
...adcast/BroadcastShardOperationFailedException.java	55.55% <0.00%> (-44.45%)	:arrow_down:
...h/action/ingest/SimulateDocumentVerboseResult.java	60.71% <0.00%> (-39.29%)	:arrow_down:
...cluster/routing/allocation/RerouteExplanation.java	65.00% <0.00%> (-35.00%)	:arrow_down:
...min/cluster/snapshots/get/GetSnapshotsRequest.java	52.63% <0.00%> (-31.58%)	:arrow_down:
... and 478 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[lukas-vlcek]

BTW: Looking at https://github.com/opensearch-project/project-website/pull/930 it seems an article about zip POMs is in preparation 👍

What I see is that all the core plugins are expected to have org.opensearch.plugin groupId value. I think this will hold true as long as all the individual plugin project.group values result into this value as well. And if I read it correctly it should be the case, see: https://github.com/opensearch-project/OpenSearch/blob/9168f1fb43015cf7f33eab30f1300ec85f4d4305/plugins/build.gradle#L37 Though we need to double-check and test it.

[lukas-vlcek]

I also tested this on the analysis-icu plugin:

I added the following line:

apply plugin: 'opensearch.pluginzip'

below this section in its build.gradle file https://github.com/opensearch-project/OpenSearch/blob/006c832c5fe8f509aa6285de90f2c7583b3dff35/plugins/analysis-icu/build.gradle#L31-L32

and run ../../gradlew --info publishPluginZipPublicationToZipStagingRepository while being in the root folder of the ICU plugin.

The resulting POM file and other mvn files can be found in the root of the OpenSearch like this:

% ls -la ../../build/local-staging-repo/org/opensearch/plugin/analysis-icu 

drwxr-xr-x   8 lukas.vlcek  staff  256 Aug  9 11:40 ./
drwxr-xr-x   3 lukas.vlcek  staff   96 Aug  9 11:40 ../
drwxr-xr-x  17 lukas.vlcek  staff  544 Aug  9 11:40 3.0.0-SNAPSHOT/
-rw-r--r--   1 lukas.vlcek  staff  329 Aug  9 11:40 maven-metadata.xml
-rw-r--r--   1 lukas.vlcek  staff   32 Aug  9 11:40 maven-metadata.xml.md5
-rw-r--r--   1 lukas.vlcek  staff   40 Aug  9 11:40 maven-metadata.xml.sha1
-rw-r--r--   1 lukas.vlcek  staff   64 Aug  9 11:40 maven-metadata.xml.sha256
-rw-r--r--   1 lukas.vlcek  staff  128 Aug  9 11:40 maven-metadata.xml.sha512

% ls -la ../../build/local-staging-repo/org/opensearch/plugin/analysis-icu/3.0.0-SNAPSHOT 

drwxr-xr-x  17 lukas.vlcek  staff       544 Aug  9 11:40 ./
drwxr-xr-x   8 lukas.vlcek  staff       256 Aug  9 11:40 ../
-rw-r--r--   1 lukas.vlcek  staff       920 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.pom
-rw-r--r--   1 lukas.vlcek  staff        32 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.pom.md5
-rw-r--r--   1 lukas.vlcek  staff        40 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.pom.sha1
-rw-r--r--   1 lukas.vlcek  staff        64 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.pom.sha256
-rw-r--r--   1 lukas.vlcek  staff       128 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.pom.sha512
-rw-r--r--   1 lukas.vlcek  staff  12935875 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.zip
-rw-r--r--   1 lukas.vlcek  staff        32 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.zip.md5
-rw-r--r--   1 lukas.vlcek  staff        40 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.zip.sha1
-rw-r--r--   1 lukas.vlcek  staff        64 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.zip.sha256
-rw-r--r--   1 lukas.vlcek  staff       128 Aug  9 11:40 analysis-icu-3.0.0-20220809.094043-1.zip.sha512
-rw-r--r--   1 lukas.vlcek  staff       783 Aug  9 11:40 maven-metadata.xml
-rw-r--r--   1 lukas.vlcek  staff        32 Aug  9 11:40 maven-metadata.xml.md5
-rw-r--r--   1 lukas.vlcek  staff        40 Aug  9 11:40 maven-metadata.xml.sha1
-rw-r--r--   1 lukas.vlcek  staff        64 Aug  9 11:40 maven-metadata.xml.sha256
-rw-r--r--   1 lukas.vlcek  staff       128 Aug  9 11:40 maven-metadata.xml.sha512

The POM itself looks like this:

% cat ../../build/local-staging-repo/org/opensearch/plugin/analysis-icu/3.0.0-SNAPSHOT/analysis-icu-3.0.0-20220809.094043-1.pom

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <modelVersion>4.0.0</modelVersion>
  <groupId>org.opensearch.plugin</groupId>
  <artifactId>analysis-icu</artifactId>
  <version>3.0.0-SNAPSHOT</version>
  <packaging>zip</packaging>
  <inceptionYear>2021</inceptionYear>
  <licenses>
    <license>
      <name>The Apache Software License, Version 2.0</name>
      <url>http://www.apache.org/licenses/LICENSE-2.0.txt</url>
      <distribution>repo</distribution>
    </license>
  </licenses>
  <developers>
    <developer>
      <name>OpenSearch</name>
      <url>https://github.com/opensearch-project/OpenSearch</url>
    </developer>
  </developers>
  <url>unknown</url>
  <scm>
    <url>unknown</url>
  </scm>
</project>

BTW, the opensearch.pluginzip plugin is not currently used in any of OpenSearch plugins that are found in the source code. Do you think it would make sense to add an example use of it in some plugin under plugins/example folder?

[prudhvigodithi]

Hey @lukas-vlcek thanks for PR, quick question here, if the plugins not part of OpenSearch/plugins folder and does not have a group specified as group = 'org.opensearch.plugin', what would be the default groupID? From your PR i see as String groupId = getProperty("group", project);, I dont see any default set, so the all listed plugins, that are not part of OpenSearch/plugins folder should they explicitly mention the group to be org.opensearch.plugin? @dblock @reta @bbarani Thank you

[lukas-vlcek]

Hi @prudhvigodithi

If the project.group value is not found then the build will fail (see relevant test that I created for this case https://github.com/opensearch-project/OpenSearch/pull/4156/files#diff-ecd94c54475d67ab08078a5cdb6e381c01094427bad63b70c4e67c7978b3d895R58-R70) in which case I think the failure is a product of the maven-publish plugin (or some of its dependencies).

So there is nothing like default or fall-back value for missing group. I think the value should be always specified (IMO it is perfectly fine if the value is defined in parent project).

Do we have any examples of plugins that will be using opensearch.pluginzip and do not have project.group defined?

[prudhvigodithi]

Do we have any examples of plugins that will be using opensearch.pluginzip and do not have project.group defined?

@lukas-vlcek mostly all of them that have used opensearch.pluginzip so far use default settings org.opensearch.plugin, example job-scheduler

./gradlew properties 
group: org.opensearch

asynchronous-search

./gradlew properties
group: org.opensearch
opensearch_group: org.opensearch

[lukas-vlcek]

@prudhvigodithi

I need to look at it but my initial (naive) question would be why there needs to be a code that actually live under different group than it wants to manifest itself. It almost sounds like we are asking for troubles but again, I am not familiar with the details about why this is done like that.

However, with recent release 2.2.0 I noticed this issue: https://github.com/opensearch-project/notifications/issues/501#issue-1330364398 and it makes me ask the question if this is related?

[dblock]

This LGTM.

@lukas-vlcek do you want to make a PR updating https://github.com/opensearch-project/opensearch-plugins/blob/main/BUILDING.md#opensearchpluginzip?

@prudhvigodithi Any reason not to merge this as is?

I think we can go and add project.group in the plugins where needed to make it explicit if that's the right solution. There should be no difference between plugins that ship with the default distribution vs. other plugins.

[prudhvigodithi]

Two things, one is for all existing plugins that has publication pluginZip should have a new line groupID Example as

publishing {
    publications {
        pluginZip(MavenPublication) { publication ->
            groupId = 'org.opensearch.plugin'
            pom {
              name = pluginName
              description = pluginDescription
              licenses {
                license {
                  name = "The Apache License, Version 2.0"
                  url = "http://www.apache.org/licenses/LICENSE-2.0.txt"
                }
              }
              developers {
                developer {
                  name = "OpenSearch"
                  url = "https://github.com/opensearch-project/opensearch-plugin-template-java"
                }
              }
            }
        }
    }
}

Second is there will be 2 groupID always as minimum per project (which is fine), but when not mentioned groupId explicitly for pluginZip MavenPublication , it will default to org.opensearch, then the idea of plugins (that ship with the default distribution) being in central place would no longer be true, each could have its own groupID within the maven and it would be hard to locate. So my thought is why not it be default as org.opensearch.plugin, unless it overrides with a specific groupId only within the pluginZip MavenPublication. @dblock @lukas-vlcek @bbarani

[prudhvigodithi]

Hey, Just to add, here is the issue created from community feedback https://github.com/opensearch-project/documentation-website/issues/838, the ask is for a reference of Maven Coordinates for the plugins, it would be good if the default is org.opensearch.plugin and the artifcatID being the plugin name. To allow this pluginZip MavenPublication to other plugins, configuring a groupID should override org.opensearch.plugin. So long story short lets go with default (when no groupID specified for pluginZip(MavenPublication)) as org.opensearch.plugin, but still an option to override with groupID. Note: This groupID override should only consider if it's part of the specific publication pluginZip(MavenPublication), else it might override the entire project groupID (should test this though). Thoughts? @dblock @lukas-vlcek

[dblock]

@lukas-vlcek ? ^

[lukas-vlcek]

@dblock @prudhvigodithi Thanks for the review. I was out last week. I am getting back to this now.

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/2068/
• CommitID: 5764c3b70d2eb16e2eb762c0c5b8916ffaa47afd

[lukas-vlcek]

Oops, some precommit check are failing, I forgot to run the formatting check before the commit ... my bad. Fixing it now.

[dblock]

@prudhvigodithi Are you good with this?

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/2069/
• CommitID: 01329b0dfb7e40eb9f5a739df1783df3a99fad25

[lukas-vlcek]

Hi @prudhvigodithi

I updated the PR. It is now possible to override (customize) the groupId value directly in the Maven publication object. This will override whatever the actual value of Gradle project.group is.

So this means that for any existing OpenSearch plugin that does not have the project.group value equal to org.opensearch.plugin we can add a single line into the publication task to override it. Do you think that would be acceptable?

I added a test to illustrate this case as well. Please see the customizedGroupValue.gradle (line 20). Notice that this also means that the maven publication task will reflect the new groupId value when uploading the artifact into maven repo. See PublishTests.java, the customizedGroupValue test (around the line 200). When the groupId is changed to I.am.customized then the file is uploaded into ../I/am/customized/.. path as well. Which I think is correct from the maven POW.

So in essence there are two ways how to configure what the groupId value will be:

1. The value is taken from Gradle project.group
2. The value can be overridden in ZIP Maven publication task (as explained and demonstrated by new test)

I do not think there can be anything like a default value. You either need to set the Gradle project.group correctly or you have to override it in the publication object.

Notice: if we are ok with this change then before it is merged I suggest that we also add the groupId override for all the plugins that will need that (for instance the job-scheduler plugin). I think it would be good to have it as part of this PR.

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/2072/
• CommitID: 266be9c5ba98c67be01b14c49ca048583edd4624

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/2074/
• CommitID: 5f35f1dd3422bade670a21ce312cd0eeff683889

[lukas-vlcek]

These check failures seem to be unrelated.

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: null :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/2112/
• CommitID: 5f35f1dd3422bade670a21ce312cd0eeff683889

[prudhvigodithi]

I updated the PR. It is now possible to override (customize) the groupId value directly in the Maven publication object. This will override whatever the actual value of Gradle project.group is.

Thanks for the update @lukas-vlcek, nice, this is good change, ideally for each publication the groupID configured could be different, not necessarily be the project groupID.

Also @lukas-vlcek lets go with the default as org.opensearch.plugin this line https://github.com/opensearch-project/OpenSearch/blob/5f35f1dd3422bade670a21ce312cd0eeff683889/buildSrc/src/main/java/org/opensearch/gradle/pluginzip/Publish.java#L54 would publish all the publication outputs to the same project groupID, which can be tar, zip and jar extensions under same path, why not this to be org.opensearch.plugin for pluginZip. This value is already configured and published for all OpenSearch plugins. Once You have this change I will do a quick test with your fork and we are good to go, rest all code and tests i'm fine with them. Thank you

[github-actions[bot]]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE :x:
• URL: https://build.ci.opensearch.org/job/gradle-check/2127/
• CommitID: 5f35f1dd3422bade670a21ce312cd0eeff683889

[rursprung]

This value is already configured and published for all OpenSearch plugins.

but that's only true for plugins from the OpenSearch project itself. 3rd party plugins shouldn't be published under that path. but if you set it as the default they'll end up there (because who bothers reading the docs or changing default values? 😉) which would be wrong. i vote for no default / taking it from the build, as currently done by @lukas-vlcek

[prudhvigodithi]

I partially agree ;) @rursprung There needs to be some default value set, every publication need not directly use the default project groupID, if thats the case and imagine they have the same artifcatID, ideally which will have, because the project name would be same, then under same folder you end up having multiple format of files if the project has several publications of different formats. Again I'm open both thoughts, both has equal pros :) @dblock @bbarani thoughts? Thank you

[lukas-vlcek]

Hi @prudhvigodithi

If I understand correctly what you mean then I would have the same objection as @rursprung.

The whole point of this PR is to enable external plugins to benefit from the ZIP publication mechanism. But with the hard-coded default value it would work differently for core plugins and external plugins, the authors of external plugins will always have to specify the groupId in the ZIP publication config (unless they want to publish their artifact under org.opensearch.plugin namespace) on the other hand the core plugins will always publish under org.opensearch.plugin namespace regardless of which namespace they actually live in. This can definitely create some confusion. I am not sure if it is neccessary.

If we, however, make the rules the same for all the plugin authors (regardless whether they implement a core plugin or external one) then we will make things more predictable and the documentation will not need complicated exceptions.

How about this: If we take the changes in this PR then how many of existing core plugins will have to add groupId = "org.opensearch.plugin" to its ZIP publication config? Does it apply to "Bundled plugins" list? Or perhaps all the plugins listed in opensearch-build/issues/1916 under the "Campaign Issues"?

If that is the case then I was checking some of those plugins and I am ok with adding the one extra line into their publication config. Such as here or here.

Then the only challenge would be correct synfronization of mergin all the changes. I think we could safely merge changes in all affected plugins first (nothing should break at this point) and then we could merge this PR (with no notion of the default value, basically as it is now). And all should be fine IMO...

WDYT?

Lukáš