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

Adding external property customDistributionUrl to let developer override default distribution Download url #380

Conversation

Rishikesh1159
Copy link
Member

@Rishikesh1159 Rishikesh1159 commented Feb 24, 2022

Signed-off-by: Rishikesh1159 rishireddy1159@gmail.com

Description

This PR adds external property "customDistributionUrl" which can be used to override default distribution download url

Issues Resolved

#381

Check List

  • 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.

…loper override default distribution Download url

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
@Rishikesh1159 Rishikesh1159 requested a review from a team February 24, 2022 18:04
ylwu-amzn
ylwu-amzn previously approved these changes Mar 11, 2022
@ylwu-amzn
Copy link
Collaborator

BTW, will this customDistributionUrl work on 1.3.0? I tried ./gradlew integTest -DcustomDistributionUrl="https://ci.opensearch.org/ci/dbc/bundle-build/1.2.0/1127/linux/x64/dist/opensearch-1.2.0-linux-x64.tar.gz" , seems the integ cluster is still 1.3.0

@Rishikesh1159
Copy link
Member Author

@ylwu-amzn sorry the I had to revert my PR in opensearch repo which actually makes his change possible. So, iI could not get this change done in 1.3 version.

@Rishikesh1159
Copy link
Member Author

Few days back I released a new PR in opensearch repo which will make this change work. But I will have to wait until 1.3 release to backport my PR to 1.x branch in opensearch repo. I will update this PR once I backport my changes to 1.x in opensarch repo and then we can use different versions by passing a custom URL from plugins.

@amitgalitz
Copy link
Member

Hi @Rishikesh1159 could you provide some update on this PR?

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
@Rishikesh1159
Copy link
Member Author

Sorry @amitgalitz @ylwu-amzn for the delay, I forgot about this PR. My change got merged in main and it works with all plugins. We just need to document that this feature is available and how to use it to users, which I did in latest commit. Other than that no changes are necessary in this repo for this feature to work.

@codecov-commenter
Copy link

Codecov Report

Merging #380 (d3f412c) into main (f630c8f) will decrease coverage by 0.03%.
The diff coverage is n/a.

❗ Current head d3f412c differs from pull request most recent head 3685923. Consider uploading reports for the commit 3685923 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #380      +/-   ##
============================================
- Coverage     79.07%   79.04%   -0.04%     
+ Complexity     4213     4210       -3     
============================================
  Files           296      296              
  Lines         17686    17686              
  Branches       1880     1880              
============================================
- Hits          13986    13980       -6     
- Misses         2805     2808       +3     
- Partials        895      898       +3     
Flag Coverage Δ
plugin 79.04% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rch/ad/transport/AnomalyResultTransportAction.java 80.13% <0.00%> (-0.69%) ⬇️
.../main/java/org/opensearch/ad/ml/CheckpointDao.java 69.55% <0.00%> (-0.65%) ⬇️
...opensearch/ad/indices/AnomalyDetectionIndices.java 72.12% <0.00%> (-0.19%) ⬇️

Copy link
Member

@amitgalitz amitgalitz left a comment

Choose a reason for hiding this comment

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

Thanks for adding this

@saratvemulapalli
Copy link
Member

@Rishikesh1159 @amitgalitz whats needed here? Another +1 ?

@amitgalitz
Copy link
Member

On my end, yes, this is just added documentation for something that was added and I verified in the past it works. If nothing changed on @Rishikesh1159 side then just needs another approval.

@saratvemulapalli
Copy link
Member

@amitgalitz I've approved it. Could you merge them if you are good?

@saratvemulapalli saratvemulapalli merged commit bd063bd into opensearch-project:main Sep 2, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 7, 2022
…ide default distribution Download url (#380)

* Adding uasage and external property customDistributionUrl to let developer override default distribution Download url

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>

* Adding doc and removing system property from build.gradle

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
(cherry picked from commit bd063bd)
ohltyler pushed a commit that referenced this pull request Sep 7, 2022
…ide default distribution Download url (#380)

* Adding uasage and external property customDistributionUrl to let developer override default distribution Download url

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>

* Adding doc and removing system property from build.gradle

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
(cherry picked from commit bd063bd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants