-
Notifications
You must be signed in to change notification settings - Fork 277
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
Added version qualifier support for OpenSearch. #1731
Conversation
Signed-off-by: dblock <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1731 +/- ##
============================================
+ Coverage 94.78% 94.79% +0.01%
Complexity 17 17
============================================
Files 169 169
Lines 3525 3536 +11
Branches 26 26
============================================
+ Hits 3341 3352 +11
Misses 181 181
Partials 3 3
Continue to review full report at Codecov.
|
@@ -7,6 +7,7 @@ ci: | |||
build: | |||
name: OpenSearch | |||
version: 2.0.0 | |||
qualifier: alpha1 |
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 will be the qualifier for GA or prod artifact? Will this field be empty?
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, we'll just remove this.
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 that case this gradle command having a null or empty value for qualifier will not be an issue?
https://github.com/opensearch-project/opensearch-build/pull/1731/files#diff-004d8135f0e1267ff669721dc64b44f84729dc6fe871c6c2ef02eb9d1e04959dR75
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 doesn't seem like it would be a problem, similarly we don't always pass a value for -Dbuild.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.
Okay. Also do we need to have a check to allow only certain values as a qualifier?
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.
^Maybe not. This qualifier check logic is already handled downstream in the component build, so adding it here seems redundant and creates two sources of truth. So good to skip imo.
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 it only supports (alpha|beta|rc)
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'm not sure if plugins build script inherits the qualifier check logic as well (?) If not, it might make sense to add 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.
Build will fail downstream, but probably makes sense to have a check here to fail on PRs to the manifest (fail earlier). Will add in a future PR.
Signed-off-by: dblock [email protected]
Description
Adds an optional build version qualifier to the input manifest that is passed onto OpenSearch, which already supports qualified version numbers (currently only
alpha
,beta
andrc
).Issues Resolved
Part of #1632.
Check List
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.