Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Gradle custom java zippublish plugin #2988
Gradle custom java zippublish plugin #2988
Changes from 11 commits
d60eafe
4a61852
1e8c5f1
5e87cde
770c886
342bc64
a8647ec
0865be2
be37b91
8a7c18e
a43e651
a523ab1
e8a11b7
cf9e291
88d21d4
056d06e
edf9028
6baf646
f8a528c
ebd85a3
26d83ee
cf9ee09
1aeb73a
b98ab64
bdf32dd
321ec96
5e97819
9ccbf81
8b85320
4d83dd1
ac2f151
daa06c3
88d4562
dba1cac
36a98f9
5e3ca97
f1da84e
71b1337
15cd29f
122d48f
30ca4c6
c733586
89bf24a
416e74e
02a8234
ad1e589
2f9bca8
ed0d1b0
4445608
b48cf2e
710611c
b61104d
eb17f5a
bfb812d
740516e
3071a55
c2b5121
f44aa2a
303465f
8f4a8d3
7f87df9
9b1cb92
c866641
9f32f4d
cdac61d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hardcode
generatePomFileForMavenZipPublication
, get rid ofZipPublishUtil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need
LOCAL_STAGING_REPO_PATH
? I assume we should use appropriate Maven repository (local or not)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reta
LOCAL_STAGING_REPO_PATH = "/build/local-staging-repo";
, this is a file system path, that we consider as a local staging repo, for maven (in terms on maven terminology) its just a publication repo, so considering thislocal-staging-repo
which is added across build scripts I have addedLOCAL_STAGING_REPO_PATH
, which denotes that its a filesystem path for locally staging the artifacts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly what is unclear: there is a clear notion of Maven repository (for publishing), why we need staging one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So right now with the code flow and process, we dont directly publish to end maven repo, we create a local staging repo on filesystem, publish the artifacts to this local staging repo and using CI, having secure creds republish the artifacts from local maven repo to actual nexus maven repo.
Example: https://github.com/opensearch-project/job-scheduler/blob/main/scripts/build.sh#L71-L73
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks @prudhvigodithi , not much we can do about it ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is backwards. You have access to version and qualifier properties, so the last thing you should be doing is parsing the ZIP filename. Build the correct version from those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @dblock, the
$VERSION
currently already has the qualifier and snapshot info ex2.0.0-rc1-SNAPSHOT
, it will save some flags for running the task and would reduce the human errorsNow a user just need to pass
./gradlew publishMavenzipPublicationToZipstagingRepository -Dopensearch.version=$VERSION
, dont need to pass any additional flags.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have it backwards. When I do
./gradlew publishMavenzipPublicationToZipstagingRepository -Dopensearch.version=2.0.0
the output is-2.0.0.zip
, but when I do./gradlew publishMavenzipPublicationToZipstagingRepository -Dopensearch.version=2.0.0 -Dbuild.version_qualifier=rc1
the output is-2.0.0-rc1.zip
. All this version parsing you're trying to do is not necessary.I believe what you want here is
VersionProperties.getOpenSearch()
- see how it's used all over the place in .gradle scripts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version that is getting passed has the value as
VERSION=2.0.0-alpha1-SNAPSHOT
, not just2.0.0
, hence I went with that parsing logic as in$VERSION
itself I can get the qualifier and snapshot info.so now a user need not pass all other flags
version_qualifier
andbuild.snapshot
. I can try withVersionProperties.getOpenSearch()
, but again it this only gives just the version number, then we expect a user to add the other flags to execute the task.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the existing task
bundlePlugin
coming from PluginBuildPlugin, the output is an generated as a zip file, I have added the publish artifact to look for this output.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
zipArtifact
name will be the project root name andversion
is the gradle property of the project.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is the same, just
BUILD_DISTRIBUTIONS_LOCATION
changes.All these CAPITALIZED variables are weird. Only CONSTANTS should be capitalized and I suspect half of them aren't needed anyway. Cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have considered them as constants (using variable modifiers static and final ) :)
As these vars are constants, and have added final to them, these are referenced throughout the code once or multiple times, also these should not be changed moving forward with any logic, hence considering these I have CAPITALIZED them. Please correct me if my though process is wrong, tomorrow if we have to change some name setting, staging repo settings and distributions path, all we need is to just update the constants, no need to touch the actual methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this example
BUILD_DISTRIBUTIONS_LOCATION
is not a constant. You declared it in the class aspublic static String BUILD_DISTRIBUTIONS_LOCATION = "/build/distributions/";
and you are reassigning it here asBUILD_DISTRIBUTIONS_LOCATION = getProperty("zipFilePath");
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with
BUILD_DISTRIBUTIONS_LOCATION
, also when I read back again, better it would be aszipDistributionLocation
, fixed this in my latest commit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will conflict when multiple publication tasks are run. Care to explain what breaks if, for example,
sourceJarTask.setEnabled(false);
is not here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, anyhow I see we are not publishing the generated sourcesJar and javadocJar to actual nexus maven repo, this publish code will ignore these jars before uploading them to nexus, so ideally these just exists on the local staging repo (filesystem) and not in maven repo.
We can also proceed to remove the task suspension from plugin code which will cause no harm but along with zips, javadocJar and sourcesJar are also included to local staging repo, and off course we stop them from publishing to nexus with the existing publish code .
The other way is remove the task suspension and allow users to declare the suspension at their own will in their gradle project globally in settings .gradle as
startParameter.excludedTaskNames=["sourcesJar", "javadocJar"]
or from task using./gradlew publishMavenzipPublicationToZipstagingRepository -Dopensearch.version=$VERSION -Dbuild.snapshot=$SNAPSHOT -Dbuild.version_qualifier=$QUALIFIER -x sourcesJar -x javadocJar
So there are multiple options go with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prudhvigodithi
For plugins we do [1] (and for everything else as well), probably we only skip that for snapshots:
mapper-size-1.3.1-javadoc.jar 2022-03-30 20:38 414035
mapper-size-1.3.1-javadoc.jar.asc 2022-03-30 20:38 287
mapper-size-1.3.1-javadoc.jar.md5 2022-03-30 20:38 32
mapper-size-1.3.1-javadoc.jar.sha1 2022-03-30 20:38 40
mapper-size-1.3.1-javadoc.jar.sha256 2022-03-30 20:38 64
mapper-size-1.3.1-javadoc.jar.sha512 2022-03-30 20:38 128
mapper-size-1.3.1-sources.jar 2022-03-30 20:38 8245
mapper-size-1.3.1-sources.jar.asc 2022-03-30 20:38 287
mapper-size-1.3.1-sources.jar.md5 2022-03-30 20:38 32
mapper-size-1.3.1-sources.jar.sha1 2022-03-30 20:38 40
mapper-size-1.3.1-sources.jar.sha256 2022-03-30 20:38 64
mapper-size-1.3.1-sources.jar.sha512 2022-03-30 20:38 128
mapper-size-1.3.1.jar 2022-03-30 20:38 10416
mapper-size-1.3.1.jar.asc 2022-03-30 20:38 287
mapper-size-1.3.1.jar.md5 2022-03-30 20:38 32
mapper-size-1.3.1.jar.sha1 2022-03-30 20:38 40
mapper-size-1.3.1.jar.sha256 2022-03-30 20:38 64
mapper-size-1.3.1.jar.sha512 2022-03-30 20:38 128
We should not disable that.
[1] https://repo1.maven.org/maven2/org/opensearch/plugin/mapper-size/1.3.1/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks @reta, this info really helps, I can see them on releases repo as well example: https://aws.oss.sonatype.org/content/repositories/releases/org/opensearch/opensearch-job-scheduler/1.3.0.0/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So based on this @reta and @dblock we should be going back to my previous changes to actually modify the existing PublishPlugin to avoid publication artifact's of sourcesJar and javadocJar if the publication is for zip.
We should be removing the
setEnabled(false)
from the plugin code.To refer here are the changes we need to add to existing PublishPlugin, to make the sourcesJar and javadocJar are still enabled for other publications but not zip, changes are
https://github.com/prudhvigodithi/OpenSearch/blob/gradleplugin-2.3/buildSrc/src/main/java/org/opensearch/gradle/PublishPlugin.java#L124-L145
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the above change for PublishPlugin and tested job-scheduler, anomaly-detection and alerting, looks good just published the zip and not the sourcesJar and javadocJar along with zips, but they are added for jar maven publication, so no interruption for existing jar publications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this change here, am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would replace
zip
terminology withPluginDistribution
since this is reflects what exactly we intend to publish and where is it applicable, fePublishPluginDistributionExtension
, wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since i have organized as folder under
zippublish/
I guess it should be good, the entire effort of this plugin is to publish the zips, hence added with startZip
:)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the notion of zip is implementation detail, what we are developing is publication mechanism for plugin distribution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have add the java classes based on plugin name
zippublish
(with camel case), how about thisZipPublishPluginDistributionExtension
@reta ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am with @reta and would call this
PluginDistributionExtension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you call this with well known values, so just hard-code those values and remove this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Util's I have added in consideration for future as well, we might need to merge the existing publish task and this zippublish plugin to one common plugin, so then these handy Util's will come into picture. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public final static String PUBLICATION_NAME = "mavenzip";
.MAVEN_ZIP_PUBLISH_POM_TASK
is a constant that uses that,public final static String MAVEN_ZIP_PUBLISH_POM_TASK = "generatePomFileFor" + ZipPublishUtil.capitalize(PUBLICATION_NAME) + "Publication";
capitalize
, just doThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
off course I can hardcode them, but if we change the
PUBLICATION_NAME
, then its expected from user to also modifyMAVEN_ZIP_PUBLISH_POM_TASK
to fix the publication, hence I added the Util.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we ever want to change these constants? I think you're over-optimizing for a complicated future problem by adding a new method (and no tests for it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to write tests that ensure that this new plugin actually works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more assertions to make sure plugin wiring works properly.
https://github.com/opensearch-project/OpenSearch/pull/2988/files#diff-47de290a01c633870926f91e6e25ca99346b4a42f80aa9e60f54e0ed54a1d4f7