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

[Backport 2.x] ZIP publication groupId value is configurable (#4156) … #4733

Conversation

lukas-vlcek
Copy link
Contributor

@lukas-vlcek lukas-vlcek commented Oct 11, 2022

…and Further simplification of the ZIP publication implementation (#4360)

Description

There is one noticeable difference, the project.group property can be empty in which case it will fall back to default value: org.opensearch.plugin. See #4156 (comment) for more details.

Check List

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 lukas-vlcek requested review from a team and reta as code owners October 11, 2022 13:44
@lukas-vlcek
Copy link
Contributor Author

Hi @prudhvigodithi and @reta
I created a single pack-port PR for 2.x that combines two PRs: #4156 and #4360
I am not sure if there are any specific steps to follow when creating back-port PRs. I tried my best and followed a common sense.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2022

Codecov Report

Merging #4733 (bd53fca) into 2.x (a90fd73) will increase coverage by 0.00%.
The diff coverage is 65.74%.

@@             Coverage Diff             @@
##                2.x    #4733     +/-   ##
===========================================
  Coverage     70.62%   70.62%             
- Complexity    57126    57398    +272     
===========================================
  Files          4585     4615     +30     
  Lines        274526   275861   +1335     
  Branches      40233    40365    +132     
===========================================
+ Hits         193876   194836    +960     
- Misses        64403    64794    +391     
+ Partials      16247    16231     -16     
Impacted Files Coverage Δ
...ava/org/opensearch/client/RestHighLevelClient.java 41.75% <0.00%> (-2.27%) ⬇️
...er/src/main/java/org/opensearch/client/Client.java 40.00% <ø> (ø)
.../org/opensearch/client/support/AbstractClient.java 34.25% <0.00%> (-0.45%) ⬇️
...main/java/org/opensearch/common/lucene/Lucene.java 68.15% <ø> (+2.33%) ⬆️
...rg/opensearch/common/settings/ClusterSettings.java 91.89% <ø> (ø)
...pensearch/common/settings/IndexScopedSettings.java 100.00% <ø> (ø)
...index/codec/PerFieldMappingPostingFormatCodec.java 64.28% <ø> (ø)
...rg/opensearch/index/engine/TranslogLeafReader.java 40.90% <ø> (ø)
...java/org/opensearch/index/get/ShardGetService.java 60.89% <ø> (ø)
.../java/org/opensearch/gradle/pluginzip/Publish.java 10.63% <6.66%> (-47.37%) ⬇️
... and 572 more

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

@prudhvigodithi
Copy link
Contributor

Thanks for the PR @lukas-vlcek should we should even backport this #4696 ? @reta WDYT?

@prudhvigodithi prudhvigodithi added the v2.4.0 'Issues and PRs related to version v2.4.0' label Oct 11, 2022
…rch-project#4156) and Further simplification of the ZIP publication implementation (opensearch-project#4360)

* This is a backport of opensearch-project#4156 and opensearch-project#4360 to 2.x
* opensearch-project#4156 was cherry-picked from 7fe5830
  -  Resolving conflicts in CHANGELOG.md
* opensearch-project#4360 was cherry-picked from ab6849f
  -  Resolving conflicts in CHANGELOG.md

There is one noticeable difference, the `project.group` property can be empty in which case it will fall back to default value: `org.opensearch.plugin`. See <opensearch-project#4156 (comment)> for more details.

Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>
@lukas-vlcek lukas-vlcek force-pushed the backport/backport-4156-and-4360-to-2.x branch from d221634 to bd53fca Compare October 11, 2022 17:41
@reta
Copy link
Collaborator

reta commented Oct 11, 2022

Thanks for the PR @lukas-vlcek should we should even backport this #4696 ? @reta WDYT?

@prudhvigodithi absolutely, right after this one I will create a backport

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta reta merged commit f4a3369 into opensearch-project:2.x Oct 11, 2022
@lukas-vlcek lukas-vlcek deleted the backport/backport-4156-and-4360-to-2.x branch October 12, 2022 05:13
@prudhvigodithi
Copy link
Contributor

Hey just noticed the default groupID even though its added as org.opensearch.plugin, its not picking the default
Example opensearch-ml-plugin, the default was set to org.opensearch @lukas-vlcek @reta

@reta
Copy link
Collaborator

reta commented Oct 12, 2022

Hey just noticed the default groupID even though its added as org.opensearch.plugin, its not picking the default Example opensearch-ml-plugin, the default was set to org.opensearch @lukas-vlcek @reta

@prudhvigodithi I see only 3.0.0-SNAPSHOT there, the default is only for 2.x

@prudhvigodithi
Copy link
Contributor

Ya If you see https://aws.oss.sonatype.org/content/repositories/snapshots/org/opensearch/plugin/opensearch-ml-plugin/2.4.0.0-SNAPSHOT/, I dont see any new zips after this PR is merged, when I test on my local /build.sh manifests/2.4.0/opensearch-2.4.0.yml --component job-scheduler, I see two problems:

  1. The default group ID is not getting applied as org.opensearch.plugin.
  2. If we add specifically the group = org.opensearch.plugin not only the zips but the jars are also being pushed to org.opensearch.plugin
[opensearch@ee8276647b38 2.4.0.0]$ pwd
/usr/share/opensearch/git/opensearch-build/tar/builds/opensearch/maven/org/opensearch/plugin/opensearch-job-scheduler/2.4.0.0
[opensearch@ee8276647b38 2.4.0.0]$ ls -ltr
total 528
-rw-rw-r-- 1 opensearch opensearch 218959 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.zip
-rw-rw-r-- 1 opensearch opensearch    128 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.pom.sha512
-rw-rw-r-- 1 opensearch opensearch     32 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.pom.md5
-rw-rw-r-- 1 opensearch opensearch     64 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.module.sha256
-rw-rw-r-- 1 opensearch opensearch     32 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.module.md5
-rw-rw-r-- 1 opensearch opensearch     40 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.jar.sha1
-rw-rw-r-- 1 opensearch opensearch    128 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.zip.sha512
-rw-rw-r-- 1 opensearch opensearch     40 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.zip.sha1
-rw-rw-r-- 1 opensearch opensearch     32 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.zip.md5
-rw-rw-r-- 1 opensearch opensearch    128 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-sources.jar.sha512
-rw-rw-r-- 1 opensearch opensearch     32 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-sources.jar.md5
-rw-rw-r-- 1 opensearch opensearch     64 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.pom.sha256
-rw-rw-r-- 1 opensearch opensearch     40 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.pom.sha1
-rw-rw-r-- 1 opensearch opensearch     40 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.module.sha1
-rw-rw-r-- 1 opensearch opensearch     32 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-javadoc.jar.md5
-rw-rw-r-- 1 opensearch opensearch  32247 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.jar
-rw-rw-r-- 1 opensearch opensearch  18311 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-sources.jar
-rw-rw-r-- 1 opensearch opensearch   1077 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.pom
-rw-rw-r-- 1 opensearch opensearch    128 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.module.sha512
-rw-rw-r-- 1 opensearch opensearch 156193 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-javadoc.jar
-rw-rw-r-- 1 opensearch opensearch     64 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.jar.sha256
-rw-rw-r-- 1 opensearch opensearch     64 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.zip.sha256
-rw-rw-r-- 1 opensearch opensearch     64 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-sources.jar.sha256
-rw-rw-r-- 1 opensearch opensearch     40 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-sources.jar.sha1
-rw-rw-r-- 1 opensearch opensearch   2188 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.module
-rw-rw-r-- 1 opensearch opensearch    128 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-javadoc.jar.sha512
-rw-rw-r-- 1 opensearch opensearch     64 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-javadoc.jar.sha256
-rw-rw-r-- 1 opensearch opensearch     40 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0-javadoc.jar.sha1
-rw-rw-r-- 1 opensearch opensearch    128 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.jar.sha512
-rw-rw-r-- 1 opensearch opensearch     32 Oct 12 18:46 opensearch-job-scheduler-2.4.0.0.jar.md5

@reta @bbarani @lukas-vlcek @dblock @peterzhuamazon

@reta
Copy link
Collaborator

reta commented Oct 12, 2022

Ya If you see https://aws.oss.sonatype.org/content/repositories/snapshots/org/opensearch/plugin/opensearch-ml-plugin/2.4.0.0-SNAPSHOT/, I dont see any new zips after this PR is merged, when I test on my local /build.sh manifests/2.4.0/opensearch-2.4.0.yml --component job-scheduler, I see two problems:

I could confirm that I see the same, the issue seem to be introduced by 7fe5830, before this commit.

ll build/local-staging-repo/org/opensearch/                                                             
drwxrwxr-x 3 andriy.redko andriy.redko 4.0K Oct 12 15:19 opensearch-job-scheduler              
drwxrwxr-x 3 andriy.redko andriy.redko 4.0K Oct 12 15:19 opensearch-job-scheduler-spi          
drwxrwxr-x 3 andriy.redko andriy.redko 4.0K Oct 12 15:19 plugin   

But after this commit (the opensearch-job-scheduler is gone):

ll build/local-staging-repo/org/opensearch/                                                             
drwxrwxr-x 3 andriy.redko andriy.redko 4.0K Oct 12 15:19 opensearch-job-scheduler-spi          
drwxrwxr-x 3 andriy.redko andriy.redko 4.0K Oct 12 15:19 plugin   

The pluginZip and group are configured like that:

allprojects {
    group = 'org.opensearch'
}

publishing {
    publications {
        pluginZip(MavenPublication) { publication ->
            pom {
                name = "opensearch-job-scheduler"
                description = "OpenSearch Job Scheduler plugin"
                group = 'org.opensearch.plugin'
            }
        }
    }
}

@prudhvigodithi
Copy link
Contributor

I have raised a bug #4756

@prudhvigodithi
Copy link
Contributor

prudhvigodithi commented Oct 12, 2022

I see the property groupId would switch the publication group, that being said, following setting should be working, but still the default groupID is not getting added as org.opensearch.plugin.

publishing {
    publications {
        pluginZip(MavenPublication) { publication ->
            pom {
                name = "opensearch-job-scheduler"
                description = "OpenSearch Job Scheduler plugin"
                groupId = 'org.opensearch.plugin'
            }
        }
    }
}

@reta please confirm.

@reta
Copy link
Collaborator

reta commented Oct 12, 2022

@reta please confirm.

Correct, groupId = 'org.opensearch.plugin' should be used, not group

@lukas-vlcek
Copy link
Contributor Author

Yes, the groupId should be used in the Publication context (see https://docs.gradle.org/current/userguide/publishing_maven.html#sec:identity_values_in_the_generated_pom).

I tried to run ad-hoc test with the following config:

plugins {
  id 'java-gradle-plugin'
  id 'opensearch.pluginzip'
}

version='2.0.0.0'

// A bundlePlugin task mockup
tasks.register('bundlePlugin', Zip.class) {
  archiveFileName = "sample-plugin-${version}.zip"
  destinationDirectory = layout.buildDirectory.dir('distributions')
  from layout.projectDirectory.file('sample-plugin-source.txt')
}

allprojects {
  group = 'org.opensearch'
}

publishing {
  publications {
    pluginZip(MavenPublication) { publication ->
      // Notice the missing groupId here...
      pom {
        name = "sample-plugin"
        description = "pluginDescription"
      }
    }
  }
}

And the ZIP artifact was deployed into build/local-staging-repo/org/opensearch/sample-plugin/2.0.0.0 folder.

% ls -l
total 80
-rw-r--r--  1 lukas.vlcek  staff  508 Oct 12 22:47 sample-plugin-2.0.0.0.pom
-rw-r--r--  1 lukas.vlcek  staff   32 Oct 12 22:47 sample-plugin-2.0.0.0.pom.md5
-rw-r--r--  1 lukas.vlcek  staff   40 Oct 12 22:47 sample-plugin-2.0.0.0.pom.sha1
-rw-r--r--  1 lukas.vlcek  staff   64 Oct 12 22:47 sample-plugin-2.0.0.0.pom.sha256
-rw-r--r--  1 lukas.vlcek  staff  128 Oct 12 22:47 sample-plugin-2.0.0.0.pom.sha512
-rw-r--r--  1 lukas.vlcek  staff  148 Oct 12 22:47 sample-plugin-2.0.0.0.zip
-rw-r--r--  1 lukas.vlcek  staff   32 Oct 12 22:47 sample-plugin-2.0.0.0.zip.md5
-rw-r--r--  1 lukas.vlcek  staff   40 Oct 12 22:47 sample-plugin-2.0.0.0.zip.sha1
-rw-r--r--  1 lukas.vlcek  staff   64 Oct 12 22:47 sample-plugin-2.0.0.0.zip.sha256
-rw-r--r--  1 lukas.vlcek  staff  128 Oct 12 22:47 sample-plugin-2.0.0.0.zip.sha512

% cat sample-plugin-2.0.0.0.pom
<?xml version="1.0" encoding="UTF-8"?>
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <modelVersion>4.0.0</modelVersion>
  <groupId>org.opensearch</groupId>
  <artifactId>sample-plugin</artifactId>
  <version>2.0.0.0</version>
  <packaging>zip</packaging>
  <name>sample-plugin</name>
  <description>pluginDescription</description>
</project>

Which I believe is what we need, correct?

@lukas-vlcek
Copy link
Contributor Author

BTW, I will add this ^^ test into our test package, I think it is useful.

@prudhvigodithi
Copy link
Contributor

prudhvigodithi commented Oct 12, 2022

Thanks @lukas-vlcek, the only thing I see now is org.opensearch.plugin is not being added as default, but which is fine, I'm working on adding groupId = "org.opensearch.plugin" to all plugins.
opensearch-project/opensearch-build#2521

@lukas-vlcek
Copy link
Contributor Author

lukas-vlcek commented Oct 12, 2022

@prudhvigodithi That is odd, because we have tests for this (in 2.x). See the PublishTests.missingGroupValue which test that if missingGroupValue.gradle script is used then the artifact is deployed into org/opensearch/plugin (following the default groupId value).
What is the specific configuration of that plugin?

@prudhvigodithi
Copy link
Contributor

@lukas-vlcek with job-scheduler when I commented the line groupId = "org.opensearch.plugin" and ran ./build.sh manifests/2.4.0/opensearch-2.4.0.yml --component job-scheduler
I dont see org/opensearch/plugin folder and all the zips and jars are now under /org/opensearch/opensearch-job-scheduler/2.4.0.0

[opensearch@ee8276647b38 opensearch]$ cd maven/org/opensearch/
[opensearch@ee8276647b38 opensearch]$ ls -la
total 16
drwxrwxr-x 4 opensearch opensearch 4096 Oct 12 21:18 .
drwxrwxr-x 3 opensearch opensearch 4096 Oct 12 21:18 ..
drwxrwxr-x 3 opensearch opensearch 4096 Oct 12 21:18 opensearch-job-scheduler
drwxrwxr-x 3 opensearch opensearch 4096 Oct 12 21:18 opensearch-job-scheduler-spi

I suspect this is because of this line

allprojects {
    group = 'org.opensearch' 
}

@lukas-vlcek
Copy link
Contributor Author

lukas-vlcek commented Oct 13, 2022

@prudhvigodithi @reta @peterzhuamazon
I created a new PR with additional tests to demonstrate how the group or groupId propagates. This should finally make it clear about what impact we can expect on individual plugins.

See #4772
(If that PR gets merged then we shall consider back-porting it to 2.x as well)

lukas-vlcek added a commit to lukas-vlcek/opensearch-plugins that referenced this pull request Oct 21, 2022
Reflect the change in [PR#4733](opensearch-project/OpenSearch#4733) in the documentation.

Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>
saratvemulapalli pushed a commit to opensearch-project/opensearch-plugins that referenced this pull request Oct 24, 2022
…175)

Reflect the change in [PR#4733](opensearch-project/OpenSearch#4733) in the documentation.

Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>

Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants