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

Updates for build qualifiers #1742

Closed

Conversation

peternied
Copy link
Member

Description

Making sure to fully support build qualifiers as used in OpenSearch 2.0.0 builds

Issues Resolved

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

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.

@peternied peternied requested a review from a team April 5, 2022 15:19
@peternied
Copy link
Member Author

Working on a test failure in org.opensearch.security.dlic.dlsfls.DlsTest.testDlsWithMinDocCountZeroAggregations,

@cliu123
Copy link
Member

cliu123 commented Apr 5, 2022

It'll be good to avoid hardcode versions. Please see the changes in this branch(WIP). I'm testing against OpenSearch 2.0.0 alpha1 on the branch. Once everything is ready, I can create a PR from the branch.

@peternied
Copy link
Member Author

It'll be good to avoid hardcode versions.

How are these properties hardcoded? They can be altered by passing in command line parameters the test matrix is from the OpenSearch 2.0.0 build logs, e.g. for K-NN the command is executed as ./gradlew assemble --no-daemon --refresh-dependencies -DskipTests=true -Dopensearch.version=2.0.0-alpha1-SNAPSHOT -Dbuild.snapshot=true -Dbuild.version_qualifier=alpha1

List<String> descriptorProperties = [
"description=Provide access control related features for OpenSearch",
"version=${version}",
"version=${version.tokenize('-')[0]}",
Copy link
Member

Choose a reason for hiding this comment

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

what would this change achieve?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously our version string would only be the numeric version 2.0.0 now it can include 2.0.0-alpha and this is not accepted in the descriptor file. String.tokenize(delim)[0] is a quick way in groovy to substring before the given token.

@codecov-commenter
Copy link

Codecov Report

Merging #1742 (fe20622) into main (3f7fffb) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #1742      +/-   ##
============================================
- Coverage     60.39%   60.32%   -0.08%     
+ Complexity     3197     3194       -3     
============================================
  Files           253      253              
  Lines         18095    18093       -2     
  Branches       3245     3245              
============================================
- Hits          10929    10915      -14     
- Misses         5585     5596      +11     
- Partials       1581     1582       +1     
Impacted Files Coverage Δ
...iance/ComplianceIndexingOperationListenerImpl.java 60.86% <0.00%> (ø)
...nsearch/security/compliance/FieldReadCallback.java 55.05% <0.00%> (ø)
...security/configuration/DlsFlsFilterLeafReader.java 56.02% <ø> (-1.86%) ⬇️
...security/auditlog/sink/InternalOpenSearchSink.java 69.23% <0.00%> (-11.54%) ⬇️
...a/org/opensearch/security/tools/SecurityAdmin.java 37.50% <0.00%> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f7fffb...fe20622. Read the comment docs.

@cliu123
Copy link
Member

cliu123 commented Apr 5, 2022

It'll be good to avoid hardcode versions.

How are these properties hardcoded? They can be altered by passing in command line parameters the test matrix is from the OpenSearch 2.0.0 build logs, e.g. for K-NN the command is executed as ./gradlew assemble --no-daemon --refresh-dependencies -DskipTests=true -Dopensearch.version=2.0.0-alpha1-SNAPSHOT -Dbuild.snapshot=true -Dbuild.version_qualifier=alpha1

I'm referring to versions being hardcoded in gradle.properties file.

@peternied
Copy link
Member Author

I'm referring to versions being hardcoded in gradle.properties file.

@cliu123 These can be substituted with command line overrides, and there needs to be defaults when the build occurs without any parameters. If this value is stored in a gradle.build file or gradle.properties or some other convention - this sounds like a preference, how can we move forward with this PR?

@cliu123
Copy link
Member

cliu123 commented Apr 5, 2022

@peternied Can we avoid mixing up changes? You don't have to merge the changes in the PR#1741 into this PR when PR#1741 is in WIP.

@peternied
Copy link
Member Author

@cliu123 I needed to pull in the test fixes otherwise the build with the qualifier wouldn't function. The integration tests of the distribution and the security plugin are blocked on these changes, so optimizing for speed is important.

Copy link
Member

@cliu123 cliu123 left a comment

Choose a reason for hiding this comment

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

Speed is not the first priority. This PR is for version qualifier. There's no need to include other changes, especially the changes from another open PR. BTW, there's opensearch-project/OpenSearch#2769 on OpenSearch 2.0.0-alpha1. Please hold off moving forward to alpha1 or alpha2 for now.

@peternied peternied closed this Apr 7, 2022
@peternied peternied deleted the build-qualifier-rebase branch April 7, 2022 01:37
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.

Add support for build version qualifiers.
5 participants