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

Add build.version_qualifier and make security plugin compatible with OpenSearch 1.0.0-rc1 #1179

Merged
merged 4 commits into from
May 24, 2021
Merged

Add build.version_qualifier and make security plugin compatible with OpenSearch 1.0.0-rc1 #1179

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5041b33
Add build.version_qualifier and make security plugin compatible with …
cliu123 May 22, 2021
55fcef8
update the opensearch dependency from rc1 to snapshot
cliu123 May 24, 2021
8704c15
Change qualifier and dependencies back from snapshot to rc1
cliu123 May 24, 2021
b478f83
Revert the change in plugin version
cliu123 May 24, 2021
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ jobs:

- name: Build OpenSearch
working-directory: ./OpenSearch
run: ./gradlew publishToMavenLocal -Dbuild.snapshot=false
run: ./gradlew publishToMavenLocal -Dbuild.version_qualifier=rc1 -Dbuild.snapshot=false
Copy link
Contributor

Choose a reason for hiding this comment

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

What makes this rc1? The checkout references 1.x branch that is a moving target.

Copy link
Member Author

@cliu123 cliu123 May 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, 1.x is a moving target.
CI checkout 1.x branch of OpenSearch core, but the version of OpenSearch core is 1.0.0-rc1 for RC, so plugins CI also builds opensearch-1.0.0-rc1 and plugins are required to be built on it. This is an agreement between plugins and OpenSearch core, for example AD and Index Management that have already made the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cliu123 Please see my comment #1176 (comment) regarding difference between SNAPSHOT and other tags. Either CI should use https://github.com/opensearch-project/OpenSearch/releases/tag/1.0.0-rc1 or it should use SNAPSHOT. Mixing both is confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

As @dblock mentioned on #1176: "in order for the plugin to start the version of the engine and the plugin will have to match, and the next version is rc1."
I don't really see any potential risks to build OpenSearch with the qualifier rc1 that is consistent with the versions of both OpenSearch core and plugins. It would help to keep consistent across all plugins unless there are risks to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

rc1 must correspond to the actual rc1 candidate of OpenSearch, not just to a random/latest commit on the 1.x branch.

Copy link
Member

Choose a reason for hiding this comment

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

@vrozov You're right, what should really happen is that we take an XYZ build of OpenSearch, test it, and label it as rc1 after we're done. However today this is proving itself to be difficult because versions must match across OpenSearch and plugins for everything to start together, including the rc1 qualifier, if any. So it's a chicken and egg problem. I described what we should be doing in opensearch-project/OpenSearch#763, please add your comments.

For this PR I suggest letting it slide to make rc1 because I don't think we can fix 763 in time. But for rc2 or afterwards we should, I'll followup on getting that assigned.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dblock I don't see a chicken and egg problem here and there was no such problem with the ODFE releases: ES releases first, plugins use released ES artifacts at build, test and runtime. The same process should be followed with Opensearch: rc1 is tagged (best will be to publish artifacts) and plugins should use the tag. Once rc2 is available on Opensearch, plugins should update to that tag. Using latest commit on 1.x branch, but pretending that it is rc1 makes security plugin build unstable and more importantly testing of security plugin would be done against latest commit, not rc1, so it can't guarantee that plugin is compatible with the actual rc1 Opensearch.

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

Copy link
Member

Choose a reason for hiding this comment

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

@MMakaria: this is an ask to effectively change the release train sequence, I had brought it up before

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing that would make security plugin built on different version/commits of OpenSearch core from other plugins. Let's have a decision before changing this. Whatever the decision is going to be, all plugins will need to take the same action or to stick with building plugins on top of 1.x branch of OpenSearch core.


- name: Checkstyle
run: mvn -B checkstyle:checkstyle
Expand Down
4 changes: 2 additions & 2 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ plugins {
buildDir = 'gradle-build'

ext {
opensearchVersion = '1.0.0.0-beta1'
opensearchVersion = '1.0.0.0-rc1'
isSnapshot = "true" == System.getProperty("build.snapshot", "true")
}

Expand Down Expand Up @@ -67,7 +67,7 @@ ospackage {
user 'root'
permissionGroup 'root'

requires('opensearch-oss', "1.0.0-beta1", EQUAL)
requires('opensearch-oss', "1.0.0-rc1", EQUAL)
packager = 'Amazon'
vendor = 'Amazon'
os = 'LINUX'
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
<maven.compiler.target>1.8</maven.compiler.target>
<maven.compiler.release>8</maven.compiler.release>

<opensearch.version>1.0.0-beta1</opensearch.version>
<opensearch.version>1.0.0-rc1</opensearch.version>

<!-- deps -->
<netty-native.version>2.0.25.Final</netty-native.version>
Expand Down