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

Add pluginZip publication for plugins in the plugins folder #16219

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Oct 7, 2024

Description

This PR adds a new publication for plugins in the plugins/ folder to publish zip files of the artifacts.

This enables tasks like the following:

➜  OpenSearch git:(publish-plugin-zip) ./gradlew :plugins:mapper-size:publishPluginZipToMavenLocal

➜  OpenSearch git:(publish-plugin-zip) ✗ ls -latr ~/.m2/repository/org/opensearch/plugin/mapper-size/3.0.0-SNAPSHOT
total 328
drwxr-xr-x@ 12 cwperx  staff     384 May  6 16:32 ..
-rw-r--r--@  1 cwperx  staff    9981 Oct  1 16:51 mapper-size-3.0.0-SNAPSHOT.jar
-rw-r--r--@  1 cwperx  staff    8359 Oct  1 16:51 mapper-size-3.0.0-SNAPSHOT-sources.jar
-rw-r--r--@  1 cwperx  staff    1950 Oct  1 16:51 mapper-size-3.0.0-SNAPSHOT.module
-rw-r--r--@  1 cwperx  staff  113170 Oct  1 16:51 mapper-size-3.0.0-SNAPSHOT-javadoc.jar
drwxr-xr-x@  9 cwperx  staff     288 Oct  7 11:43 .
-rw-r--r--@  1 cwperx  staff   14238 Oct  7 11:43 mapper-size-3.0.0-SNAPSHOT.zip
-rw-r--r--@  1 cwperx  staff    1086 Oct  7 11:43 mapper-size-3.0.0-SNAPSHOT.pom
-rw-r--r--@  1 cwperx  staff     718 Oct  7 11:43 maven-metadata-local.xml

Related Issues

Related to #16195

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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: Craig Perkins <[email protected]>
@reta
Copy link
Collaborator

reta commented Oct 7, 2024

Sorry @cwperks , missed the original issue , I think this is clearly a miss now that opensearch-plugin does find the plugins for non-released versions. However, publishing it explicitly would deviate from the experience that we offer for buoltin plugins (plus, we do not publish ZIPs for any builtin plugins or modules now).

I think for SNAPSHOTs, this command should work (as it does for releases):

./opensearch-plugin install  mapper-size

That would probably require CLI tooling to be aware of snapshot repository where the plugins are published (much like the SNAPSHOT distribution).

Copy link
Contributor

github-actions bot commented Oct 7, 2024

❌ Gradle check result for 8a11171: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@cwperks
Copy link
Member Author

cwperks commented Oct 7, 2024

I think for SNAPSHOTs, this command should work (as it does for releases):

How would this command work if the zipfiles are not published to the SNAPSHOTS repo? Zipfiles are published for other plugins in the default distributions, but are not published for the built-in plugins. Do you think it makes sense to publish zips for the built-in plugins? If not, I will close this PR.


This PR would publish the zipfiles, but additional opensearch-plugin CLI modification would be needed to ensure that a command like ./opensearch-plugin install mapper-size would work for SNAPSHOTS to get the zips from the snapshot maven repo.

@reta
Copy link
Collaborator

reta commented Oct 7, 2024

Do you think it makes sense to publish zips for the built-in plugins? If not, I will close this PR.

It depends what is the goal. I think we should be focusing getting this one to work:

opensearch-plugin install  mapper-size

Would that require publishing zips for the built-in plugins? It seems like not since we haven't been doing it for any of the releases (so far)

@reta
Copy link
Collaborator

reta commented Oct 7, 2024

To share a more context why I think so, it comes strait from the typical usages, all over the place. For example, we publish Docker images for releases and SNAPSHOTs. If someone wants to customize Docker images with more plugins installed, the simplest way to do it is:

FROM opensearch-project/opensearch:<version>
RUN ./bin/opensearch-plugin install -b mapper-size

That (ideally) should work for any image that we (OpenSearch project) publish, release or SNAPSHOT, not the case now.

@cwperks cwperks marked this pull request as draft October 7, 2024 18:42
@cwperks
Copy link
Member Author

cwperks commented Oct 7, 2024

I think the snapshot zips would need to be published in order for this to work. I was able to rework some old code within InstallPluginCommand and have it look for snapshots by default if it detects that its within a snapshot build of opensearch. I'm not able to test it though since the snapshots do not exist:

➜  opensearch-3.0.0-SNAPSHOT git:(publish-plugin-zip) ✗ ./bin/opensearch-plugin install mapper-size
-> Installing mapper-size
-> Downloading mapper-size from opensearch
-> Failed installing mapper-size
-> Rolling back mapper-size
-> Rolled back mapper-size
Exception in thread "main" java.io.IOException: Server returned HTTP response code: 403 for URL: https://artifacts.opensearch.org/snapshots/plugins/mapper-size/3.0.0/mapper-size-3.0.0-SNAPSHOT.zip
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1998)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1599)
	at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:223)
	at org.opensearch.plugins.InstallPluginCommand.downloadZip(InstallPluginCommand.java:455)
	at org.opensearch.plugins.InstallPluginCommand.downloadAndValidate(InstallPluginCommand.java:532)
	at org.opensearch.plugins.InstallPluginCommand.download(InstallPluginCommand.java:319)
	at org.opensearch.plugins.InstallPluginCommand.execute(InstallPluginCommand.java:273)
	at org.opensearch.plugins.InstallPluginCommand.execute(InstallPluginCommand.java:250)
	at org.opensearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:104)
	at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
	at org.opensearch.cli.MultiCommand.execute(MultiCommand.java:104)
	at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
	at org.opensearch.cli.Command.main(Command.java:101)
	at org.opensearch.plugins.PluginCli.main(PluginCli.java:66)

Let me see if I can test it using maven local.

Converting this to Draft in the meantime

Copy link
Contributor

github-actions bot commented Oct 7, 2024

❌ Gradle check result for 2fa838d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Craig Perkins <[email protected]>
@reta
Copy link
Collaborator

reta commented Oct 7, 2024

I think the snapshot zips would need to be published in order for this to work.

We may need some help from build team on that, we do publish min SHAPSHOT distributions fe, @peterzhuamazon could you help us here to find the right way / place to have snapshot based distributions published? Thank you.

Copy link
Contributor

github-actions bot commented Oct 7, 2024

✅ Gradle check result for b3c57db: SUCCESS

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.80%. Comparing base (a81b868) to head (b3c57db).
Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #16219      +/-   ##
============================================
- Coverage     71.94%   71.80%   -0.15%     
+ Complexity    64612    64552      -60     
============================================
  Files          5298     5299       +1     
  Lines        301952   301959       +7     
  Branches      43627    43629       +2     
============================================
- Hits         217247   216823     -424     
- Misses        66884    67322     +438     
+ Partials      17821    17814       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@prudhvigodithi
Copy link
Contributor

prudhvigodithi commented Oct 9, 2024

The default behaviour for opensearch-plugin install is to look for https://artifacts.opensearch.org, today we publish core snapshots example https://artifacts.opensearch.org/snapshots/core/opensearch/3.0.0-SNAPSHOT/opensearch-min-3.0.0-SNAPSHOT-darwin-x64-latest.tar.gz, similar what we can do it

or another option is

  • Update the opensearch-plugin install command to default to the Maven repository. For snapshots, it should check https://aws.oss.sonatype.org/content/repositories/snapshots to download the zip files. Moving forward, we should publish the snapshot native plugin zips to Maven (currently, we only publish jars, whereas other non-native plugins publish both jars and zips, so we should align by doing the same for native plugins).

Some useful discussion from past:
opensearch-project/opensearch-build#4291 (comment)

Adding @getsaurabh02 @dblock

@reta
Copy link
Collaborator

reta commented Oct 11, 2024

@cwperks do you want a hand on this with infra part? thanks!

@dblock
Copy link
Member

dblock commented Oct 11, 2024

Coming from opensearch-project/opensearch-api-specification#609 (comment) I'd like these to be published!

@cwperks
Copy link
Member Author

cwperks commented Oct 11, 2024

@cwperks do you want a hand on this with infra part? thanks!

Yes, that would be great. The first commit of this PR was intended to publish the snapshot zips. I added another commit later to modify the InstallPluginCommand, but I am not able to properly test it without having published any snapshot zips. wdyt about starting with publishing snapshots of the native plugin zipfiles?

@prudhvigodithi
Copy link
Contributor

If we go with maven we should consider this point, both jars and zips should not be published under the same groupID, example org.opensearch.plugin has the jars published for native plugins, so we should use another groupID for zips.

For regular plugins this org.opensearch.plugin is used for publishing the plugin zips and their jars are published to org.opensearch.<plugin_name> (example https://aws.oss.sonatype.org/content/repositories/releases/org/opensearch/opensearch-geo/1.2.1/).

So should we do the same for native plugin? Publish the jars to org.opensearch.<plugin_name> (example org.opensearch.repository-s3) and zips to org.opensearch.plugin ?

@reta
Copy link
Collaborator

reta commented Oct 11, 2024

If we go with maven we should consider this point, both jars and zips should not be published under the same groupID, example org.opensearch.plugin has the jars published for native plugins, so we should use another groupID for zips.

I think publication wise, we should be installing ZIPs under: https://artifacts.opensearch.org/snapshots/plugins/
The problem though, we are publishing releases now (3.0.0, 2.18.0) and not snapshots, that makes things very confusing. Why is that?

@prudhvigodithi
Copy link
Contributor

yes @reta just pushing the native plugin snapshot zips to s3 https://artifacts.opensearch.org/snapshots/plugins/ and updating the InstallPluginCommand logic accordingly (to pull from snapshot path) should solve the problem here.

@reta
Copy link
Collaborator

reta commented Oct 11, 2024

InstallPluginCommand logic accordingly (to pull from snapshot path) should solve the problem here.

It actually supports that already :) But there is no publications to fetch

@cwperks
Copy link
Member Author

cwperks commented Oct 11, 2024

Is the stagingHash actually necessary? It looked like a legacy from before the fork and safe to remove, but let me know if its actually needed.

@reta
Copy link
Collaborator

reta commented Oct 11, 2024

Is the stagingHash actually necessary? It looked like a legacy from before the fork and safe to remove, but let me know if its actually needed.

May need to be sorted out, right

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.

4 participants