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

Added opensearch.pluginzip plugin #31

Merged
merged 8 commits into from
May 10, 2022
Merged

Conversation

prudhvigodithi
Copy link
Member

Signed-off-by: pgodithi [email protected]

Description

Add opensearch.pluginzip custom gradle plugin to the template.

Issues Resolved

opensearch-project/opensearch-build#1916
opensearch-project/opensearch-plugins#144

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.

@prudhvigodithi prudhvigodithi changed the title Addec opensearch.pluginzip plugin Added opensearch.pluginzip plugin May 3, 2022
@prudhvigodithi
Copy link
Member Author

Hey @reta and @dblock, I have included the custom plugin opensearch.pluginzip to the template, please check.

@@ -30,3 +30,4 @@ jobs:
- name: Build and Run Tests
run: |
./gradlew build
./gradlew help --task publishPluginZipPublicationToZipStagingRepository
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we just need ./gradlew publishPluginZipPublicationToZipStagingRepository?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reta, I wanted to check opinions here, yes even this ./gradlew publishPluginZipPublicationToZipStagingRepository can be added, but do we need to, I see there are multiple custom plugins already added to the template, but we dont run a plugin specific task, just ./gradlew build exists, do we even need to add publishPluginZipPublicationToZipStagingRepository to test the CI?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, I think for the sake of checking that publishing works, we could publish to local repo: publishPluginZipPublicationToZipLocalRepository

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can add ./gradlew publishPluginZipPublicationToZipStagingRepository, also moving forward its better we add some validation by running the custom plugin tasks, so that way we can some evidence that the custom plugins task actually work, I will add ./gradlew publishPluginZipPublicationToZipStagingRepository to the CI.yaml.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

settings.gradle Outdated
@@ -8,3 +8,4 @@
*/

rootProject.name = 'plugin-template'
startParameter.excludedTaskNames=["validatePluginZipPom"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, where this setting comes from?

Copy link
Member Author

@prudhvigodithi prudhvigodithi May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reta This is part of the plugin setup, usually all plugins managed build scripts or default build scripts run assemble task and publish tasks (specific to repo), example job-scheduler, but not direct build task, if called we need to exclude this validatePluginZipPom as general build task will include this, if this task called it will fail with POM validation with the error

> Task :validatePluginZipPom FAILED
Execution optimizations have been disabled for task ':validatePluginZipPom' to ensure correctness due to the following reasons:
  - Gradle detected a problem with the following location: '/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom'. Reason: Task ':validatePluginZipPom' uses this output of task ':generatePomFileForNebulaPublication' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.4.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.
name is missing in [/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom]
description is missing in [/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom]
licenses is empty in [/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom]
developers is empty in [/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom]

Ideally we dont need to run this task as we define the POM values during actual publish.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to add I have also added this point to the plugin usage document (PR under review)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@dblock dblock May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still looks wrong to me, shouldn't we be fixing the POM by adding these fields? Open a new issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was fixed after adding

allprojects {
    project.ext.licenseName = 'The Apache Software License, Version 2.0'
    project.ext.licenseUrl = 'http://www.apache.org/licenses/LICENSE-2.0.txt'
    publishing {
        publications {
            all {
                pom.withXml { XmlProvider xml ->
                    Node node = xml.asNode()
                    node.appendNode('inceptionYear', '2022')
                    node.appendNode('name', 'rename')
                    node.appendNode('description', 'Custom plugin')
                    Node license = node.appendNode('licenses').appendNode('license')
                    license.appendNode('name', project.licenseName)
                    license.appendNode('url', project.licenseUrl)
                    Node developer = node.appendNode('developers').appendNode('developer')
                    developer.appendNode('name', 'OpenSearch')
                    developer.appendNode('url', 'https://github.com/opensearch-project/opensearch-plugin-template-java')
                }
            }
        }
    }
}

Then we dont need to exclude, I would say we can add the above template to the build.gradle and it's up-to the plugin team, if they want to exclude the the task validatePluginZipPom or add the POM fields in right way, I can update the info in the document as well, thoughts? @dblock

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excluding is better, rather than forcing user to add these POM fields as then it allow user to run ./gradlew build, without any restrictions, @dblock @reta @bbarani @peterzhuamazon

Copy link
Member

@dblock dblock May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand. Plugins should all look the same and have all the right/same info in the POM. Why wouldn't this be in all plugins?

Copy link
Member Author

@prudhvigodithi prudhvigodithi May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some plugins do not have all of these settings @dblock, but in our case to publish zips we dont need this task validatePluginZipPom.
This is coming from PomValidationTask (org.opensearch.gradle.precommit.PomValidationTask)

.github/workflows/CI.yml Outdated Show resolved Hide resolved
Signed-off-by: pgodithi <[email protected]>
Copy link
Collaborator

@reta reta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prudhvigodithi so I pulled the pull request locally and it seems like more work is needed:

> Task :publishPluginZipPublicationToZipStagingRepository
Execution optimizations have been disabled for task ':publishPluginZipPublicationToZipStagingRepository' to ensure correctness due to the following reasons:
  - Gradle detected a problem with the following location: 'opensearch-plugin-template-java/build/distributions/rename-unspecified.pom'. Reason: Task ':publishPluginZipPublicationToZipStagingRepository' uses this output of task ':generatePomFileForNebulaPublication' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.4.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

Those warnings should not be there, we need to find the way to properly address POM file exclusion or/and generation

@prudhvigodithi
Copy link
Member Author

Those warnings should not be there, we need to find the way to properly address POM file exclusion or/and generation

Hey @reta, thanks for testing,
In this case I can see several warning with ./gradlew build that also includes validatePluginZipPom.

 ./gradlew build
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 7.4.2
  OS Info               : Linux 5.4.186-113.361.amzn2int.x86_64 (amd64)
  JDK Version           : 17 (Eclipse Temurin JDK)
  JAVA_HOME             : /opt/java/openjdk-17
  Random Testing Seed   : 6130E2759B36F54C
  In FIPS 140 mode      : false
=======================================

> Task :validatePluginZipPom
Execution optimizations have been disabled for task ':validatePluginZipPom' to ensure correctness due to the following reasons:
  - Gradle detected a problem with the following location: '/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom'. Reason: Task ':validatePluginZipPom' uses this output of task ':generatePomFileForNebulaPublication' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.4.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.

> Task :integTest
OpenJDK 64-Bit Server VM warning: Ignoring option --illegal-access=warn; support was removed in 17.0
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by org.opensearch.bootstrap.BootstrapForTesting (file:/usr/share/opensearch/.gradle/caches/modules-2/files-2.1/org.opensearch.test/framework/2.0.0-rc1-SNAPSHOT/7bca874662f27c649cbba03d409417f3b7c22c4b/framework-2.0.0-rc1-SNAPSHOT.jar)
WARNING: Please consider reporting this to the maintainers of org.opensearch.bootstrap.BootstrapForTesting
WARNING: System::setSecurityManager will be removed in a future release
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by org.gradle.api.internal.tasks.testing.worker.TestWorker (file:/usr/share/opensearch/.gradle/wrapper/dists/gradle-7.4.2-bin/48ivgl02cpt2ed3fh9dbalvx8/gradle-7.4.2/lib/plugins/gradle-testing-base-7.4.2.jar)
WARNING: Please consider reporting this to the maintainers of org.gradle.api.internal.tasks.testing.worker.TestWorker
WARNING: System::setSecurityManager will be removed in a future release

> Task :test
OpenJDK 64-Bit Server VM warning: Ignoring option --illegal-access=warn; support was removed in 17.0
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by org.opensearch.bootstrap.BootstrapForTesting (file:/usr/share/opensearch/.gradle/caches/modules-2/files-2.1/org.opensearch.test/framework/2.0.0-rc1-SNAPSHOT/7bca874662f27c649cbba03d409417f3b7c22c4b/framework-2.0.0-rc1-SNAPSHOT.jar)
WARNING: Please consider reporting this to the maintainers of org.opensearch.bootstrap.BootstrapForTesting
WARNING: System::setSecurityManager will be removed in a future release
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by org.gradle.api.internal.tasks.testing.worker.TestWorker (file:/usr/share/opensearch/.gradle/wrapper/dists/gradle-7.4.2-bin/48ivgl02cpt2ed3fh9dbalvx8/gradle-7.4.2/lib/plugins/gradle-testing-base-7.4.2.jar)
WARNING: Please consider reporting this to the maintainers of org.gradle.api.internal.tasks.testing.worker.TestWorker
WARNING: System::setSecurityManager will be removed in a future release

> Task :yamlRestTest
OpenJDK 64-Bit Server VM warning: Ignoring option --illegal-access=warn; support was removed in 17.0
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by org.opensearch.bootstrap.BootstrapForTesting (file:/usr/share/opensearch/.gradle/caches/modules-2/files-2.1/org.opensearch.test/framework/2.0.0-rc1-SNAPSHOT/7bca874662f27c649cbba03d409417f3b7c22c4b/framework-2.0.0-rc1-SNAPSHOT.jar)
WARNING: Please consider reporting this to the maintainers of org.opensearch.bootstrap.BootstrapForTesting
WARNING: System::setSecurityManager will be removed in a future release
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by org.gradle.api.internal.tasks.testing.worker.TestWorker (file:/usr/share/opensearch/.gradle/wrapper/dists/gradle-7.4.2-bin/48ivgl02cpt2ed3fh9dbalvx8/gradle-7.4.2/lib/plugins/gradle-testing-base-7.4.2.jar)
WARNING: Please consider reporting this to the maintainers of org.gradle.api.internal.tasks.testing.worker.TestWorker
WARNING: System::setSecurityManager will be removed in a future release

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.4.2/userguide/command_line_interface.html#sec:command_line_warnings

Execution optimizations have been disabled for 1 invalid unit(s) of work during this build to ensure correctness.
Please consult deprecation warnings for more details.

however as we dont need this task validatePluginZipPom, once I disable it I dont see any warnings

./gradlew publishPluginZipPublicationToZipStagingRepository
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 7.4.2
  OS Info               : Linux 5.4.186-113.361.amzn2int.x86_64 (amd64)
  JDK Version           : 17 (Eclipse Temurin JDK)
  JAVA_HOME             : /opt/java/openjdk-17
  Random Testing Seed   : 3D31CDE948265F62
  In FIPS 140 mode      : false
=======================================

BUILD SUCCESSFUL in 583ms
8 actionable tasks: 2 executed, 6 up-to-date

Also I can see ls -ltr build/local-staging-repo/org/opensearch/plugin/plugin-template/, the zips being published to local maven repo

I have also tested this with job-scheduler and AD plugin, does not throw any errors, thoughts @reta ?
Should we consider still fixing this task validatePluginZipPom?

@prudhvigodithi
Copy link
Member Author

prudhvigodithi commented May 4, 2022

Dont want to go with hacks, but looks like validatePluginZipPom is coming from

./gradlew help --task  validatePluginZipPom
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 7.4.2
  OS Info               : Linux 5.4.186-113.361.amzn2int.x86_64 (amd64)
  JDK Version           : 17 (Eclipse Temurin JDK)
  JAVA_HOME             : /opt/java/openjdk-17
  Random Testing Seed   : 6AD3909B8FF392E7
  In FIPS 140 mode      : false
=======================================

> Task :help
Detailed task information for validatePluginZipPom

Path
     :validatePluginZipPom

Type
     PomValidationTask (org.opensearch.gradle.precommit.PomValidationTask)

Description
     -

Group
     -

We might to add this way https://github.com/opensearch-project/OpenSearch/blob/896b97e54df4e192a15994a1076a9d1d04312f74/modules/lang-painless/build.gradle#L80-L83
so that that implicit dependency warning will go away
@dblock @reta

@prudhvigodithi prudhvigodithi requested a review from reta May 4, 2022 16:42
@reta
Copy link
Collaborator

reta commented May 4, 2022

We might to add this way https://github.com/opensearch-project/OpenSearch/blob/896b97e54df4e192a15994a1076a9d1d04312f74/modules/lang-painless/build.gradle#L80-L83
so that that implicit dependency warning will go away
@dblock @reta

@prudhvigodithi looking, once sec

@prudhvigodithi
Copy link
Member Author

We might to add this way https://github.com/opensearch-project/OpenSearch/blob/896b97e54df4e192a15994a1076a9d1d04312f74/modules/lang-painless/build.gradle#L80-L83
so that that implicit dependency warning will go away
@dblock @reta

@prudhvigodithi looking, once sec

I have tried this way @reta, still get that warning :)
I'm also looking as I can see existing plugin setting is expecting this, not from this new change.

Signed-off-by: pgodithi <[email protected]>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

@dblock
Copy link
Member

dblock commented May 6, 2022

@reta re-review?

@prudhvigodithi
Copy link
Member Author

Hey @dblock, since the fix was pushed yesterday, the plugin exists but I dont think the fixed code will get into 2.0.0-rc1-SNAPSHOT as this is already shipped, this wont break but we should still expect the warning message on console, in the next OpenSearch version release these warnings should be fixed as well.

@reta
Copy link
Collaborator

reta commented May 7, 2022

@reta re-review?

@dblock LGTM for now, but I will try to revisit the POM publication part [1], it feels like better option exists

[1] https://github.com/opensearch-project/opensearch-plugin-template-java/pull/31/files#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R31

@reta
Copy link
Collaborator

reta commented May 9, 2022

@dblock @prudhvigodithi opened this one opensearch-project/OpenSearch#3252 so we could just do:

allprojects {
    project.ext.licenseName = 'The Apache Software License, Version 2.0'
    project.ext.licenseUrl = 'http://www.apache.org/licenses/LICENSE-2.0.txt'
    project.ext.developerName = 'OpenSearch'
    project.ext.developerUrl = 'https://github.com/opensearch-project/OpenSearch'
}

@prudhvigodithi
Copy link
Member Author

Hey @reta, thanks for the PR, but I see some existing plugins already use the syntax as of which I have created the PR example for job-scheduler, this is used for the jar's to get those POM fields, now with PR looks like the plugins team should add allProjects settings with two types one with general way (like how I have added in PR) for the jar files and the way how you just added for zip's. Does this not add some some duplicate code blocks? if so we might need to add the fix for existing Publish plugin as well.
@dblock @bbarani

@reta
Copy link
Collaborator

reta commented May 10, 2022

Hey @reta, thanks for the PR, but I see some existing plugins already use the syntax as of which I have created the PR example for job-scheduler, this is used for the jar's to get those POM fields, now with PR looks like the plugins team should add allProjects settings with two types one with general way (like how I have added in PR) for the jar files and the way how you just added for zip's. Does this not add some some duplicate code blocks? if so we might need to add the fix for existing Publish plugin as well. @dblock @bbarani

I think we are dealing with different publications here: for JAR and for ZIP, which essentially generate different POM files (at least, it should), but thanks for the hit, I will make sure we could use this model as well.

@prudhvigodithi
Copy link
Member Author

I think we are dealing with different publications here: for JAR and for ZIP, which essentially generate different POM files (at least, it should), but thanks for the hit, I will make sure we could use this model as well.

I think @reta we can go with this PR for now and add this fix to both JAR and ZIP's as next new enhancement, thoughts ? @reta
Right now anyhow plugin teams are passing how I have added to this PR, that should add for both JAR and ZIP's.

@reta
Copy link
Collaborator

reta commented May 10, 2022

I think we are dealing with different publications here: for JAR and for ZIP, which essentially generate different POM files (at least, it should), but thanks for the hit, I will make sure we could use this model as well.

I think @reta we can go with this PR for now and add this fix to both JAR and ZIP's as next new enhancement, thoughts ? @reta Right now anyhow plugin teams are passing how I have added to this PR, that should add for both JAR and ZIP's.

I am fine with merging it as-is, thanks @prudhvigodithi

Signed-off-by: pgodithi <[email protected]>
Signed-off-by: pgodithi <[email protected]>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to merge on green against any version of OpenSearch.

build.gradle Outdated
@@ -37,7 +60,7 @@ validateNebulaPom.enabled = false

buildscript {
ext {
opensearch_version = "2.0.0-SNAPSHOT"
opensearch_version = "3.0.0-SNAPSHOT"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prudhvigodithi this is right change in general, but for plugins we should target 2.0.0 at most - we expect most of the development happening here.

Signed-off-by: pgodithi <[email protected]>
@prudhvigodithi
Copy link
Member Author

Hey @dblock and @reta we have a --component build now available PR opensearch-project/opensearch-build#2100, the OpenSearch was build with 2.0.0 manifest and I can see the latetst timestamp in nexus https://aws.oss.sonatype.org/content/repositories/snapshots/org/opensearch/opensearch/2.0.0-SNAPSHOT/
All checks pass :)

@prudhvigodithi
Copy link
Member Author

Hey @reta, @dblock, @bbarani if you have merge access can you please merge this PR

@reta reta merged commit 12d756e into opensearch-project:main May 10, 2022
@prudhvigodithi
Copy link
Member Author

Thanks @reta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants