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

• New functionality includes testing.
  • All tests pass
• New functionality has been documented. (Doc update: The groupId is inferred from gradle project properties. opensearch-plugins#175)
  • New functionality has javadoc added
• 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 lukas.vlcek@aiven.io

[lukas-vlcek]

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

[github-actions]

Gradle Check (Jenkins) Run Completed with:

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

[github-actions]

Gradle Check (Jenkins) Run Completed with:

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

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• 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%.

❗ 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%)	⬇️
...luster/routing/allocation/RoutingExplanations.java	41.37% <0.00%> (-58.63%)	⬇️
...a/org/opensearch/tasks/TaskCancelledException.java	50.00% <0.00%> (-50.00%)	⬇️
...cluster/coordination/PendingClusterStateStats.java	20.00% <0.00%> (-48.00%)	⬇️
.../admin/cluster/reroute/ClusterRerouteResponse.java	55.00% <0.00%> (-45.00%)	⬇️
...adcast/BroadcastShardOperationFailedException.java	55.55% <0.00%> (-44.45%)	⬇️
...h/action/ingest/SimulateDocumentVerboseResult.java	60.71% <0.00%> (-39.29%)	⬇️
...cluster/routing/allocation/RerouteExplanation.java	65.00% <0.00%> (-35.00%)	⬇️
...min/cluster/snapshots/get/GetSnapshotsRequest.java	52.63% <0.00%> (-31.58%)	⬇️
... 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 opensearch-project/project-website#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:

OpenSearch/plugins/build.gradle

Line 37 in 9168f1f

group = 'org.opensearch.plugin'

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

OpenSearch/plugins/analysis-icu/build.gradle

Lines 31 to 32 in 006c832

	apply plugin: 'opensearch.yaml-rest-test'
	apply plugin: 'opensearch.internal-cluster-test'

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:
opensearch-project/notifications#501 (comment)
and it makes me ask the question if this is related?

[dblock]

👏

[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 opensearch-project/documentation-website#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]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• 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?

[prudhvigodithi]

Hey @lukas-vlcek merged the PR, can you please raise a PR for 2.x. Next updates we can follow up on this META issue.
Thank you