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.

Sign up for GitHub

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

Added opensearch.pluginzip plugin #31

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
046bc8c
Addec opensearch.pluginzip plugin
prudhvigodithi May 3, 2022
7e5f4ac
Added 2.0.0-rc1-SNAPSHOT
prudhvigodithi May 3, 2022
c0f6192
Added publishPluginZipPublicationToZipStagingRepository
prudhvigodithi May 3, 2022
ddeb3db
fixed NIT's
prudhvigodithi May 4, 2022
dcb899d
Add implicit dependency
prudhvigodithi May 4, 2022
4aa4a58
added POM
prudhvigodithi May 10, 2022
7674ea8
Added 3.0.0-SNAPSHOT
prudhvigodithi May 10, 2022
d8a7ee5
Updated OS version
prudhvigodithi May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/CI.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

👍

3 changes: 2 additions & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ apply plugin: 'java'
apply plugin: 'idea'
apply plugin: 'opensearch.opensearchplugin'
apply plugin: 'opensearch.yaml-rest-test'
apply plugin: 'opensearch.pluginzip'

def pluginName = 'rename'
def pluginDescription = 'Custom plugin'
Expand Down Expand Up @@ -37,7 +38,7 @@ validateNebulaPom.enabled = false

buildscript {
ext {
opensearch_version = "2.0.0-SNAPSHOT"
opensearch_version = "2.0.0-rc1-SNAPSHOT"
}

repositories {
Expand Down
1 change: 1 addition & 0 deletions settings.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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)