-
Notifications
You must be signed in to change notification settings - Fork 298
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 support for -Dbuild.snapshot=true/false and default to building snapshot. #1409
Conversation
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.
Building SNAPSHOT must be a separate PR from upgrade to OpenSearch 1.1, especially that there is already open PR #1335.
Codecov Report
@@ Coverage Diff @@
## main #1409 +/- ##
============================================
+ Coverage 64.79% 64.81% +0.02%
Complexity 3214 3214
============================================
Files 247 247
Lines 17264 17264
Branches 3053 3053
============================================
+ Hits 11186 11190 +4
+ Misses 4529 4525 -4
Partials 1549 1549
Continue to review full report at Codecov.
|
I'll be happy to rebase this on top of #1335 when it's merged. |
c7d81c5
to
5b2ad0a
Compare
There's probably some more that can be done here to make |
5b2ad0a
to
9bf4c0f
Compare
06a0fb0
to
25383ef
Compare
@@ -3,7 +3,7 @@ | |||
description=Provide access control related features for OpenSearch 1.0.0 | |||
# | |||
# 'version': plugin's version | |||
version=1.0.1.0 | |||
version=1.1.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.
This is security plugin version, not the OpenSearch version. If the goal of this PR is to add -SNAPSHOT
suffix in the OpenSearch version, this line doesn't need to be changed.
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 plugin version needs to be incremented because we're building OpenSearch 1.1. Every other plugin says it's version 1.1.0.0-SNAPSHOT.
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 expect 1.1.0.0-SNAPSHOT
, not 1.1.0.0
. If it is not snapshot, version should not be changed.
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 properties file by default has a number without snapshot. Then, with a profile, we change it to be -SNAPSHOT
unless -Dbuild.snapshot=false
. You can't have the logical condition the other way around if you want the default to be -Dbuild.snapshot=false
.
@@ -68,7 +68,8 @@ | |||
<maven.compiler.target>1.8</maven.compiler.target> | |||
<maven.compiler.release>8</maven.compiler.release> | |||
|
|||
<opensearch.version>1.1.0</opensearch.version> | |||
<opensearch.version>1.1.0-SNAPSHOT</opensearch.version> | |||
<version>1.1.0.0</version> |
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 referenced on the line 37 that is security plugin version. Same as this comment, doesn't seem to need a change at here. If yes, this is the example PR to bump plugin version.
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.
It had to move to properties or you cannot set it via a profile.
pom.xml
Outdated
@@ -103,7 +104,6 @@ | |||
<url>https://github.com/opensearch-project/security</url> | |||
<connection>scm:git:[email protected]:opensearch-project/security.git</connection> | |||
<developerConnection>scm:git:[email protected]:opensearch-project/security.git</developerConnection> | |||
<tag>1.0.1.0</tag> |
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 are we removing the tag 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.
What is the purpose of it here? I believe it's unused.
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.
<url>
, <connection>
and <developerConnection>
are not used either. It is there mostly for historical reasons. I don't see a good reason to remove <tag>
only.
</property> | ||
</activation> | ||
<properties> | ||
<version>1.1.0.0-SNAPSHOT</version> |
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.
Looks like this PR is bumping security version from 1.0.1.0
to 1.1.0.0
. If that's the case, is that the case?
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.
Yes, this is the case, I updated the description of the PR.
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.
Must be consistent with plugin-descriptor.properties
, I don't see how this can be done using profiles.
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.
Update: by default the version comes from plugin.properties and is set to 1.1.0.0. This sets the version to 1.1.0.0-SNAPSHOT unless you specify -Dbuild.snapshot=false
. Meaning it sets it by default. This effectively enables building both, snapshot or not snapshot, with snapshot by default.
If you put 1.1.0.0-SNAPSHOT in the properties file, then this profile condition would have to be inverted. But then the default can't be snapshot=true.
Tested the following cases:
|
Ready to re-review. |
@@ -45,7 +45,7 @@ jobs: | |||
|
|||
- name: Build OpenSearch | |||
working-directory: ./OpenSearch | |||
run: ./gradlew publishToMavenLocal -Dbuild.snapshot=false | |||
run: ./gradlew publishToMavenLocal |
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.
It does both at build time.
.github/workflows/cd.yml
Outdated
@@ -35,13 +35,13 @@ jobs: | |||
|
|||
- name: Build OpenSearch | |||
working-directory: ./OpenSearch | |||
run: ./gradlew publishToMavenLocal -Dbuild.snapshot=false | |||
run: ./gradlew publishToMavenLocal |
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.
CD runs for release and should not use SNAPSHOT
version of OpenSearch.
.github/workflows/cd.yml
Outdated
|
||
- name: Build | ||
run: | | ||
mvn -B clean package -Padvanced -DskipTests | ||
artifact_zip=`ls $(pwd)/target/releases/opensearch-security-*.zip | grep -v admin-standalone` | ||
./gradlew build buildDeb buildRpm --no-daemon -ParchivePath=$artifact_zip -Dbuild.snapshot=false | ||
./gradlew build buildDeb buildRpm --no-daemon -ParchivePath=$artifact_zip |
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.
CD runs for release and should not produce SNAPSHOT
versions of .rpm
and .deb
packages.
@@ -3,7 +3,7 @@ | |||
description=Provide access control related features for OpenSearch 1.0.0 | |||
# | |||
# 'version': plugin's version | |||
version=1.0.1.0 | |||
version=1.1.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.
I would expect 1.1.0.0-SNAPSHOT
, not 1.1.0.0
. If it is not snapshot, version should not be changed.
pom.xml
Outdated
@@ -68,7 +68,8 @@ | |||
<maven.compiler.target>1.8</maven.compiler.target> | |||
<maven.compiler.release>8</maven.compiler.release> | |||
|
|||
<opensearch.version>1.1.0</opensearch.version> | |||
<opensearch.version>1.1.0-SNAPSHOT</opensearch.version> |
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.
OpenSearch version change should be part of another PR.
pom.xml
Outdated
@@ -103,7 +104,6 @@ | |||
<url>https://github.com/opensearch-project/security</url> | |||
<connection>scm:git:[email protected]:opensearch-project/security.git</connection> | |||
<developerConnection>scm:git:[email protected]:opensearch-project/security.git</developerConnection> | |||
<tag>1.0.1.0</tag> |
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.
<url>
, <connection>
and <developerConnection>
are not used either. It is there mostly for historical reasons. I don't see a good reason to remove <tag>
only.
</property> | ||
</activation> | ||
<properties> | ||
<version>1.1.0.0-SNAPSHOT</version> |
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.
Must be consistent with plugin-descriptor.properties
, I don't see how this can be done using profiles.
We failed at having a scalable mechanism to distribute maven credentials across 15 plugins, so whereas OpenSearch bundle build 1.0 picked up output from security plugin CD, it will not for 1.1. Bundle will build from the top, including opensearch-min and security plugin, from source. Given that, this change ensures that security CD doesn't build an artifact that can be accidentally promoted to a release, i.e. always depend on a snapshot version of OpenSearch and produce a snapshot version of security plugin. The change in profiles does that, defaults to snapshot build unless you explicitly specify The plugin built in bundle is version 1.1, not 1.0.1. Unlike in ODFE or 1.0, plugins are now aligning with OpenSearch: when development starts on version X in a branch 'X', the version in that branch is incremented to X. Given that, what do you need me to change in this PR to get this in? |
|
I was trying to explain how we ended up here. The CD pipeline output is not used by anyone other than dev at this point, and thus should be a snapshot build that depends on OpenSearch snapshot.
Yes.
I am not trying to change dependency from OpenSearch 1.1.0 to 1.1.0-SNAPSHOT. I am trying to make security plugin build against 1.1.0-SNAPSHOT by default, and 1.1.0 if The pre-requisite of building OpenSearch locally will go away with opensearch-project/opensearch-build#20.
A released version is one that is tested as part of the bundle, signed and uploaded to opensearch.org. Every daily build can be a release candidate and is "declared" release by signing and publishing. Are you proposing to manually change We have two pipelines:
Plugins have had I will deal with |
Other plugins had this option as they utilize OpenSearch gradle extension. Security plugin is not built with gradle, it is built with maven and never supported |
While CD pipeline is no longer used to publish to maven and to S3 to produce distribution bundle, it is used to create release draft on github. There is no usage of CD pipeline outside of release build. For PR and builds on |
Extracted version increment into #1429. |
efad3d8
to
371c35a
Compare
7d3518f
to
4fa717e
Compare
A hacky-er alternative, opensearch-project/opensearch-build#356. |
snapshot. Signed-off-by: dblock <[email protected]>
4fa717e
to
a247984
Compare
Maven does not support using the same @dblock I would vote for the other solution that you proposed opensearch-project/opensearch-build#356 |
I agree, closing in favor of opensearch-project/opensearch-build#356. |
Signed-off-by: dblock [email protected]
opensearch-security pull request intake form
Please provide as much details as possible to get feedback/acceptance on your PR quickly
Maintenance
#1403
-Dbuild.snapshot=false
to not build a snapshot.The downside is that the pom.xml is incorrect for -SNAPSHOT build (contains non-snapshot version because pom.xml doesn't know anything about profiles).
Building the end-to-end bundle needs the artifacts to be able to build snapshot vs. not snapshot.
The build could only produce a snapshot version vs. non-snapshot.
Snapshot (default)
Not Snapshot
In a previous iteration of this PR, installed
opensearch-security
plugin on OpenSearch 1.1.0, and started the cluster.Use the profile to depend on OpenSearch 1.1.0-SNAPSHOT as well.
No
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
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.