Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url #2086

Rishikesh1159 · 2022-02-10T22:37:13Z

Signed-off-by: Rishikesh1159 [email protected]

Description

This PR overrides default distribution url with custom distribution url passes by user. Previous discussions regarding this PR can be found in PR #1737

Issues Resolved

#1240

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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.

…rl When User Passes a Url Signed-off-by: Rishikesh1159 <[email protected]>

opensearch-ci-bot · 2022-02-10T22:38:03Z

Can one of the admins verify this patch?

Rishikesh1159 · 2022-02-10T22:48:30Z

@dblock Sorry for Inconvenience, I had to close my previous PR because of issue with my repo (previous PR #1737 ). I have addressed the changes you suggested in this PR. Please let me know your opinion. Also had a chat with @saratvemulapalli , he suggested on top of these changes we can also check by passing a url from a opensearch internal plugin and see if it overrides default url. We want to make it like an integration test for this change. What is your opinion on this ?

opensearch-ci-bot · 2022-02-10T23:10:04Z

❌ Gradle Check failure 1bd18b0
Log 2329

Reports 2329

Rishikesh1159 · 2022-02-10T23:28:25Z

start gradle check

opensearch-ci-bot · 2022-02-11T00:05:34Z

✅ Gradle Check success 1bd18b0
Log 2330

Reports 2330

dblock

This is unnecessarily hacky. You can examine the repositories added without any such complication. See Rishikesh1159@6257d8e

Signed-off-by: Rishikesh1159 <[email protected]>

Rishikesh1159 · 2022-02-11T17:27:41Z

Thank you @dblock . I realize that I was unnecessarily complicating things. I have made changes you suggested in new PR.

Rishikesh1159 · 2022-02-11T17:30:10Z

Also @dblock what is your opinion on adding integration test for this change ? Something like passing custom url from opensearch internal plugin to test this change.

opensearch-ci-bot · 2022-02-11T18:07:11Z

✅ Gradle Check success ae4806d
Log 2339

Reports 2339

dblock · 2022-02-11T23:00:32Z

Also @dblock what is your opinion on adding integration test for this change ? Something like passing custom url from opensearch internal plugin to test this change.

I like more tests but won’t hold anything for an IT.

dblock · 2022-02-11T23:01:45Z

buildSrc/src/test/java/org/opensearch/gradle/DistributionDownloadPluginTests.java

+        Project project = createProject(null, false);
+        DistributionDownloadPlugin plugin = project.getPlugins().getPlugin(DistributionDownloadPlugin.class);
+        plugin.setupDistributions(project);
+        assertEquals(5, project.getRepositories().size());


Why do we still get 5 here? (And check for 4 below)

In case of when we not pass customDistribution URL to setupDownloadService() it goes to else block and calls addIvyRepo() with releases and release-candidates as last two parameters for PATTERN LAYOUT. In case when we pass customDistributionUrl PATTERN LAYOUT will be empty.

code: in setupDownloadService() else block

addIvyRepo(
project,
DOWNLOAD_REPO_NAME,
"https://artifacts.opensearch.org",
FAKE_IVY_GROUP,
"/releases" + RELEASE_PATTERN_LAYOUT,
"/release-candidates" + RELEASE_PATTERN_LAYOUT
);

In addIvyRepo() method we have Arrays.stream() which iterates through PATTERN LAYOUT parameter for two times and create two separate repos one for releases and one for release-candidates but both having same url. So, this is why we have 5 repos added instead of 4 for case of not passing custom Distribution url.

code: in addIvyRepo()

private static void addIvyRepo(Project project, String name, String url, String group, String... patternLayout) {
final List repos = Arrays.stream(patternLayout).map(pattern -> project.getRepositories().ivy(repo -> {

My question is more what do we want to do about this? If the URL is the going to be the same all the time, the second one should be removed, shouldn't it? If not, adjust the test for now to check all values.

I hit approve, but consider fixing this before merging.

Yes URL will be same both times. But I think either if we want to remove the second repo from repositories set or add a test check(opensearch-downloads2) again for same URL, we need to perform extra operation. Please correct me if I am wrong, adjusting the test to check all values would be like an extra safe step and better than removing repo. So, I will add test (checking opensearch-downloads2) to check all values as you mentioned in your previous suggestion.

works for me.

dblock

Actually one more thing. You need to document how to use this, probably in DEVELOPER_GUIDE.

Rishikesh1159 · 2022-02-15T07:42:06Z

Actually one more thing. You need to document how to use this, probably in DEVELOPER_GUIDE.

Sure. I was thinking to document this usage on all plugins repo DEVELOPER_GUIDE as we will be passing custom url from plugins side. May be we have to document this on both opensearch repo and plugins as-well. I will also get @saratvemulapalli opinion on where would be best place to document usage of this change.

dblock · 2022-02-16T16:31:34Z

Actually one more thing. You need to document how to use this, probably in DEVELOPER_GUIDE.

Sure. I was thinking to document this usage on all plugins repo DEVELOPER_GUIDE as we will be passing custom url from plugins side. May be we have to document this on both opensearch repo and plugins as-well. I will also get @saratvemulapalli opinion on where would be best place to document usage of this change.

For this PR DEVELOPER_GUIDE is sufficient.

…g usage of this in Developer Guide Signed-off-by: Rishikesh1159 <[email protected]>

dblock

Also, update the TOC in developer guide.

Let's tidy up the text.

dblock · 2022-02-16T17:33:25Z

DEVELOPER_GUIDE.md

+We can override the default distribution download url which always downloads latest version of OpenSearch, with custom distribution download url. This will help to pull artifacts from any location.
+This custom distribution download url can be passed from external plugins as an argument.
+
+Sample command: ./gradlew integTest -DcustomDistributionUrl= "custom distribution download url"


Nitpicking a bit. This is a doc, so instead of "we can override ..." write "To override ..." and instead of "This will help" with "This enables to ..." or "This allows to pull artifacts from a custom location."

url -> URL

Remove "Sample command: ", quote the example on its own line, and replace "custom distribution download url" with an actual URL that one might want to use.

Sure, I will make these changes.

Rishikesh1159 · 2022-02-16T17:35:13Z

Actually one more thing. You need to document how to use this, probably in DEVELOPER_GUIDE.

Sure. I was thinking to document this usage on all plugins repo DEVELOPER_GUIDE as we will be passing custom url from plugins side. May be we have to document this on both opensearch repo and plugins as-well. I will also get @saratvemulapalli opinion on where would be best place to document usage of this change.

For this PR DEVELOPER_GUIDE is sufficient.

Sure @dblock . Also had a chat with @saratvemulapalli, decided to document this change both in opensearch repo DEVELOPER_GUIDE and plugins repo DEVELOPER_GUIDE.

opensearch-ci-bot · 2022-02-16T19:04:08Z

❌ Gradle Check failure 5944093
Log 2460

Reports 2460

Signed-off-by: Rishikesh1159 <[email protected]>

dblock

the extra space after = makes it not copy-pastable, that needs to be fixed.

dblock · 2022-02-16T19:42:06Z

DEVELOPER_GUIDE.md

+To override the default Distribution Download URL which always downloads latest version of OpenSearch, with custom Distribution Download URL. This allows to pull artifacts from a custom location.
+This custom Distribution Download URL can be passed from external plugins as an argument.
+
+`./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"`


Extra space after =.

dblock · 2022-02-16T19:44:38Z

DEVELOPER_GUIDE.md

+#### Distribution Download Plugin
+
+To override the default Distribution Download URL which always downloads latest version of OpenSearch, with custom Distribution Download URL. This allows to pull artifacts from a custom location.
+This custom Distribution Download URL can be passed from external plugins as an argument.


I think this is a bit clearer:

The Distribution Download plugin downloads the latest version of OpenSearch by default, and supports overriding this behavior by setting customDistributionUrl.

./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"

Got it. Thanks @dblock for your suggestion. I think it looks cleaner this way.

opensearch-ci-bot · 2022-02-16T19:59:49Z

❌ Gradle Check failure 00d8023
Log 2462

Reports 2462

Signed-off-by: Rishikesh1159 <[email protected]>

opensearch-ci-bot · 2022-02-16T20:56:35Z

❌ Gradle Check failure 17bcb6a
Log 2464

Reports 2464

Rishikesh1159 · 2022-02-18T17:44:10Z

start gradle check

opensearch-ci-bot · 2022-02-18T18:22:18Z

❌ Gradle Check failure 17bcb6a
Log 2576

Reports 2576

Rishikesh1159 · 2022-02-18T18:29:08Z

start gradle check

…rl When User Passes a Url (#2086) * Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url Signed-off-by: Rishikesh1159 <[email protected]> * Adding test to check if correct IVY repos were added Signed-off-by: Rishikesh1159 <[email protected]> * Adding another test case when custom url is not passed and documenting usage of this in Developer Guide Signed-off-by: Rishikesh1159 <[email protected]> * Adding TOC and making changes in DEVELOPER_GUIDE Signed-off-by: Rishikesh1159 <[email protected]> * Making changes in DEVELOPER_GUIDE about Distribution Download plugin Signed-off-by: Rishikesh1159 <[email protected]> (cherry picked from commit 4a6f54b)

opensearch-ci-bot · 2022-02-18T19:18:36Z

✅ Gradle Check success 17bcb6a
Log 2586

Reports 2586

…bution Url When User Passes a Url (#2086)" This reverts commit 4a6f54b.

Override Default Distribution Download Url with Custom Distribution U…

1bd18b0

…rl When User Passes a Url Signed-off-by: Rishikesh1159 <[email protected]>

Rishikesh1159 requested a review from a team as a code owner February 10, 2022 22:37

Rishikesh1159 mentioned this pull request Feb 10, 2022

Override default Distribution Download URL with customTestDistributionUrl for OpenSearch, which is passed as a parameter from a plugin. #1737

Closed

5 tasks

dblock requested changes Feb 11, 2022

View reviewed changes

Adding test to check if correct IVY repos were added

ae4806d

Signed-off-by: Rishikesh1159 <[email protected]>

dblock reviewed Feb 11, 2022

View reviewed changes

dblock approved these changes Feb 14, 2022

View reviewed changes

dblock requested changes Feb 14, 2022

View reviewed changes

Adding another test case when custom url is not passed and documentin…

5944093

…g usage of this in Developer Guide Signed-off-by: Rishikesh1159 <[email protected]>

dblock requested changes Feb 16, 2022

View reviewed changes

Adding TOC and making changes in DEVELOPER_GUIDE

00d8023

Signed-off-by: Rishikesh1159 <[email protected]>

dblock requested changes Feb 16, 2022

View reviewed changes

Making changes in DEVELOPER_GUIDE about Distribution Download plugin

17bcb6a

Signed-off-by: Rishikesh1159 <[email protected]>

saratvemulapalli approved these changes Feb 18, 2022

View reviewed changes

dblock approved these changes Feb 18, 2022

View reviewed changes

dblock merged commit 4a6f54b into opensearch-project:main Feb 18, 2022

dblock added the backport 1.x label Feb 18, 2022

opensearch-trigger-bot bot mentioned this pull request Feb 18, 2022

[Backport 1.x] Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url #2175

Closed

Rishikesh1159 mentioned this pull request Feb 21, 2022

[Backport 1.x] Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url #2191

Merged

5 tasks

Rishikesh1159 added a commit that referenced this pull request Feb 25, 2022

Revert "Override Default Distribution Download Url with Custom Distri…

63853f5

…bution Url When User Passes a Url (#2086)" This reverts commit 4a6f54b.

This was referenced Feb 25, 2022

Revert "Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url" #2254

Closed

Revert "Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url" #2256

Merged

Rishikesh1159 mentioned this pull request Mar 8, 2022

Override Distribution Downloader to download opensearch-min artifact from a custom url #1240

Closed

Rishikesh1159 deleted the override_ddpl_url branch March 8, 2022 19:17

Rishikesh1159 mentioned this pull request Mar 9, 2022

Override Default Distribution Download Url with Custom Distribution Url when it is passed from Plugin #2420

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url #2086

Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url #2086

Rishikesh1159 commented Feb 10, 2022

opensearch-ci-bot commented Feb 10, 2022

Rishikesh1159 commented Feb 10, 2022 •

edited

Loading

opensearch-ci-bot commented Feb 10, 2022

Rishikesh1159 commented Feb 10, 2022

opensearch-ci-bot commented Feb 11, 2022

dblock left a comment

Rishikesh1159 commented Feb 11, 2022

Rishikesh1159 commented Feb 11, 2022

opensearch-ci-bot commented Feb 11, 2022

dblock commented Feb 11, 2022

dblock Feb 11, 2022

Rishikesh1159 Feb 11, 2022 •

edited

Loading

dblock Feb 14, 2022 •

edited

Loading

Rishikesh1159 Feb 15, 2022 •

edited

Loading

dblock Feb 16, 2022

dblock left a comment

Rishikesh1159 commented Feb 15, 2022

dblock commented Feb 16, 2022

dblock left a comment

dblock Feb 16, 2022

Rishikesh1159 Feb 16, 2022

Rishikesh1159 commented Feb 16, 2022

opensearch-ci-bot commented Feb 16, 2022

dblock left a comment

dblock Feb 16, 2022

dblock Feb 16, 2022

Rishikesh1159 Feb 16, 2022

opensearch-ci-bot commented Feb 16, 2022

opensearch-ci-bot commented Feb 16, 2022

Rishikesh1159 commented Feb 18, 2022

opensearch-ci-bot commented Feb 18, 2022

Rishikesh1159 commented Feb 18, 2022

opensearch-ci-bot commented Feb 18, 2022

Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url #2086

Override Default Distribution Download Url with Custom Distribution Url When User Passes a Url #2086

Conversation

Rishikesh1159 commented Feb 10, 2022

Description

Issues Resolved

Check List

opensearch-ci-bot commented Feb 10, 2022

Rishikesh1159 commented Feb 10, 2022 • edited Loading

opensearch-ci-bot commented Feb 10, 2022

Rishikesh1159 commented Feb 10, 2022

opensearch-ci-bot commented Feb 11, 2022

dblock left a comment

Choose a reason for hiding this comment

Rishikesh1159 commented Feb 11, 2022

Rishikesh1159 commented Feb 11, 2022

opensearch-ci-bot commented Feb 11, 2022

dblock commented Feb 11, 2022

dblock Feb 11, 2022

Choose a reason for hiding this comment

Rishikesh1159 Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

dblock Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

Rishikesh1159 Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

dblock Feb 16, 2022

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

Rishikesh1159 commented Feb 15, 2022

dblock commented Feb 16, 2022

dblock left a comment

Choose a reason for hiding this comment

dblock Feb 16, 2022

Choose a reason for hiding this comment

Rishikesh1159 Feb 16, 2022

Choose a reason for hiding this comment

Rishikesh1159 commented Feb 16, 2022

opensearch-ci-bot commented Feb 16, 2022

dblock left a comment

Choose a reason for hiding this comment

dblock Feb 16, 2022

Choose a reason for hiding this comment

dblock Feb 16, 2022

Choose a reason for hiding this comment

Rishikesh1159 Feb 16, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Feb 16, 2022

opensearch-ci-bot commented Feb 16, 2022

Rishikesh1159 commented Feb 18, 2022

opensearch-ci-bot commented Feb 18, 2022

Rishikesh1159 commented Feb 18, 2022

opensearch-ci-bot commented Feb 18, 2022

Rishikesh1159 commented Feb 10, 2022 •

edited

Loading

Rishikesh1159 Feb 11, 2022 •

edited

Loading

dblock Feb 14, 2022 •

edited

Loading

Rishikesh1159 Feb 15, 2022 •

edited

Loading