Override default Distribution Download URL with customTestDistributionUrl for OpenSearch, which is passed as a parameter from a plugin. by Rishikesh1159 · Pull Request #1737 · opensearch-project/OpenSearch

Rishikesh1159 · 2021-12-15T18:53:43Z

Override default Distribution Download URL with custom URL

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

Description

It Overrides Distribution Download URL with customTestDistributionUrl which is passed as a parameter from plugin. Along with changes made in this PR, plugins should make subsequent change to allow user to pass custom url from CLI.

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.

…nUrl for OpenSearch, which is passed as a parameter from a plugin. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>

opensearch-ci-bot · 2021-12-15T18:54:03Z

Can one of the admins verify this patch?

Rishikesh1159 · 2021-12-15T19:08:29Z

start gradle check

dblock

I expect the behavior of this feature to be "if I set the distribution URL, that's what's used and nothing else". So this is close! Let's document this as well as part of this PR.

dblock · 2021-12-15T19:17:30Z

buildSrc/src/main/java/org/opensearch/gradle/DistributionDownloadPlugin.java

    private NamedDomainObjectContainer<OpenSearchDistribution> distributionsContainer;
    private NamedDomainObjectContainer<DistributionResolution> distributionsResolutionStrategiesContainer;

+    private static String customUrl;


Name consistently, I would just do distributionUrl and setDistributionUrl since it's downloading ... a distribution.

Sure, I will rename them accordingly

dblock · 2021-12-15T19:17:57Z

buildSrc/src/main/java/org/opensearch/gradle/DistributionDownloadPlugin.java

+        if (customUrl != null && !customUrl.isEmpty()) {
+            addIvyRepo(project, DOWNLOAD_REPO_NAME, customUrl, FAKE_IVY_GROUP, "");
+            addIvyRepo(project, SNAPSHOT_REPO_NAME, customUrl, FAKE_SNAPSHOT_IVY_GROUP, "");
+            customUrl = null;


Why does this URL get reset?

When I was testing in local, if I don't reset the customUrl to null it is getting cached by gradle and when someother plugin/method which don't want to use customUrl calls this method setupDownloadServiceRepo(), the customurl value is still being set to old value and condition in if block will be true and executed. So, to avoid this I had to reset the customUrl value. I observed when a plugin individually is tested against Opensearch it executes this method setupDownloadServiceRepo() only once.

You have added a static property, so when you set it, it gets set globally, and forever. You need to figure out a way of how to set it only for the scope/instance that you are interested in instead.

Yes I thought about it, but there is certain way the parameter is being passed to DistributionDownloadPlugin.java which makes to use static variable.

--> First when plugin in ran against Opensearch it calls "TestClustersPlugin.java", here a call to DistributionDownoadPlugin.java is happening only once with project as parameter.

--> But Project values are still unset when a call to distributionDownloadPlugin.java is happening. So, I currently don't have custom URL passed by user from plugin. This file uses afterEvaluate() method which is listener interface and make some methods of file execute after project is evaluated.

--> Custom URL is passed to OpenSearchCluster.java using a setter method. This file is called by TestClustersPlugin.java after calling DistributionDownloadPlugin.java. So by the time I have custom URL from plugin, call to DistributionDownloadPlugin.java is already done.

--> If I try to create another instance of DistributionDownloadPlugin.java after getting custom URL from plugin with same project as parameter, it throws an error that "this project is already present".

--> So, the only option I had to send custom URL to DistributionDownloadPlugin.java was to use a static setter and call it by it's class name.

--> I have tried different ways to avoid setting a static variable, but they were failing. I will try asking other developers for suggestions to avoid it.

opensearch-ci-bot · 2021-12-15T19:38:28Z

✅ Gradle Check success 816a593
Log 1511

Reports 1511

opensearch-ci-bot · 2021-12-15T19:49:20Z

✅ Gradle Check success 816a593
Log 1512

Reports 1512

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

dblock · 2021-12-15T20:42:16Z

Also needs tests, https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/test/java/org/opensearch/gradle/DistributionDownloadPluginTests.java.

opensearch-ci-bot · 2021-12-15T21:17:57Z

✅ Gradle Check success 2f68702
Log 1514

Reports 1514

nknize

/cc @saratvemulapalli can you review? Make sure this is the approach that was discussed on the issue.

Let's also make sure this isn't opening something nefarious by exposing unfriendly URLs to the build process. Before approval be sure to have good documentation for this (e.g., javadocs) public methods that enable changing the download URL are sneaky so this isn't clear whether this is a benign change.

saratvemulapalli

Dropped a couple of comments.
+1 to unit tests, documentation.

saratvemulapalli · 2021-12-16T18:23:02Z

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java

+    // This method is called by plugins to override default Url
+    public static void setDistributionUrl(String distributionUrl) {
+        DistributionDownloadPlugin.setDistributionUrl(distributionUrl);
+    }
+


Instead of exposing a public static method, let the interface define this method TestClusterConfiguration, OpenSearchCluster.java and OpenSearchNode.java can override it.

And since its really a custom url, I would rename it to setCustomDistributionUrl

Instead of directly exposing OpenSearchCluster to DistributionDownloadPlugin, pass this information to OpenSearchNode.java which should set the right distribution. OpenSearchNode.java is designed to setup up the node for tests.

Suggested change

// This method is called by plugins to override default Url

public static void setDistributionUrl(String distributionUrl) {

DistributionDownloadPlugin.setDistributionUrl(distributionUrl);

}

// This method sets a custom distribution url overriding Ivy repository configured.

@Override

public void setCustomDistributionUrl(String customDistributionUrl) {

...

}

https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/TestClusterConfiguration.java#L53

saratvemulapalli · 2021-12-16T18:36:42Z

buildSrc/src/main/java/org/opensearch/gradle/DistributionDownloadPlugin.java

+    public static void setDistributionUrl(String distributionUrl) {
+        DistributionDownloadPlugin.distributionUrl = distributionUrl;
+    }
+
+    // pkg private for tests and set up Download Repository


Is there a way to avoid distributionUrl to be static?

opensearch-ci-bot · 2021-12-17T17:28:02Z

❌ Gradle Check failure e101370
Log 1572

Reports 1572

opensearch-ci-bot · 2021-12-17T18:19:45Z

❌ Gradle Check failure 5ca37b2
Log 1574

Reports 1574

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

opensearch-ci-bot · 2021-12-17T19:19:49Z

❌ Gradle Check failure 4acf50d
Log 1579

Reports 1579

Rishikesh1159 · 2021-12-17T19:21:08Z

start gradle check

opensearch-ci-bot · 2021-12-17T19:49:39Z

❌ Gradle Check failure 4acf50d
Log 1580

Reports 1580

Rishikesh1159 · 2021-12-17T21:29:02Z

start gradle check

opensearch-ci-bot · 2021-12-17T22:09:29Z

✅ Gradle Check success 4acf50d
Log 1582

Reports 1582

Rishikesh1159 · 2021-12-17T22:11:59Z

I figured out a new approach for solving this issue which is much cleaner and simple. Here we will directly set customDistributionUrl as an external property (in ext {}section) in build.gradle file of plugin (ex:anomaly detection plugin).

We can access this external property by method "project.findProperty(“customDistributionUrl”)." This is possible because we are moving setupDownloadServiceRepo() to setupDistributions() in DistributionDownloadPlugin.java. We are moving this into setupDistribution() method because this method is executed after project is evaluated and by then project is configured and we have all properties set.

I think this approach will solve all of requested changes for this PR. Please comment on this approach, if this is any better than previous one and any changes needed

saratvemulapalli

Very nice work @Rishikesh1159 !!
Love this new approach.

Overall it looks good to me, dropped a comment on tests.

saratvemulapalli · 2022-01-05T03:09:55Z

Requested reviews from @dblock and @nknize as they have requested changes, github doesnt allow changes to be merged.

saratvemulapalli · 2022-01-05T03:14:44Z

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

        assertEquals(distro.getType(), Type.ARCHIVE);
    }

+    public void testCustomDistributionUrl() {


Could you add another test which sets the customDistributionUrl and make sure DistributionDownloadPlugin downloads the artifact?

Sure @saratvemulapalli I will add another test for this scenario.

dblock · 2022-01-05T13:19:43Z

buildSrc/src/main/java/org/opensearch/gradle/DistributionDownloadPlugin.java

-        addIvyRepo(project, SNAPSHOT_REPO_NAME, "https://artifacts.opensearch.org", FAKE_SNAPSHOT_IVY_GROUP, SNAPSHOT_PATTERN_LAYOUT);
+        // checks if custom Distribution Url has been passed by user from plugins
+        if (project.findProperty("customDistributionUrl") != null) {
+            String customDistributionUrl = project.findProperty("customDistributionUrl").toString();


Nit: avoid doing 2 lookups.

... customDistributionUrl = project.findProperty("customDistributionUrl") if (customDistributionUrl != null) { ...

@dblock sure, I will make this change.

opensearch-ci-bot · 2022-01-07T17:05:37Z

❌ Gradle Check failure a1d4d87
Log 1803

Reports 1803

opensearch-ci-bot · 2022-01-10T17:25:29Z

✅ Gradle Check success 5fcb072
Log 1830

Reports 1830

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

opensearch-ci-bot · 2022-01-10T18:36:27Z

✅ Gradle Check success e5957f1
Log 1832

Reports 1832

dblock

Needs tests that show which ivy repos were added and which were not based on this input.

Call setupDownloadServiceRepo and test the results.

opensearch-ci-bot · 2022-01-26T16:54:17Z

✅ Gradle Check success a24c895
Log 2056

Reports 2056

Rishikesh1159 · 2022-01-31T17:07:18Z

Needs tests that show which ivy repos were added and which were not based on this input.

Call setupDownloadServiceRepo and test the results.

As both addIvyRepo() snd setupDownloadServiceRepo() are private methods, so I find only way to mock them is using PowerMocks. If we don't want use PowerMocks we need to change access modifiers of both these methods. @saratvemulapalli can you please help me how proceed further with this?

dblock · 2022-02-02T18:43:42Z

@Rishikesh1159 Any good reason not make these methods public?

Rishikesh1159 · 2022-02-10T22:40:16Z

Accidently deleted the fork repo in which I opened this PR. Closing this PR now. New PR is opened to solve this, PR #2086 . Changes suggested and new discussion can found in new PR.

Override default Distribution Download URL with customTestDistributio…

816a593

…nUrl for OpenSearch, which is passed as a parameter from a plugin. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>

Rishikesh1159 requested a review from a team as a code owner December 15, 2021 18:53

dblock suggested changes Dec 15, 2021

View reviewed changes

Renaming customTestDistributionUrl to distributionUrl.

2f68702

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

nknize suggested changes Dec 16, 2021

View reviewed changes

saratvemulapalli reviewed Dec 16, 2021

View reviewed changes

Merge branch 'opensearch-project:main' into main

e101370

Merge branch 'opensearch-project:main' into main

5ca37b2

Setting customDistributionUrl using project property method.

4acf50d

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

saratvemulapalli approved these changes Jan 5, 2022

View reviewed changes

saratvemulapalli requested review from dblock and nknize January 5, 2022 03:09

saratvemulapalli reviewed Jan 5, 2022

View reviewed changes

dblock reviewed Jan 5, 2022

View reviewed changes

Merge branch 'opensearch-project:main' into main

a1d4d87

Merge branch 'opensearch-project:main' into main

5fcb072

adding unit test case for distribution download plugin and other form…

e5957f1

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

saratvemulapalli approved these changes Jan 10, 2022

View reviewed changes

dblock suggested changes Jan 10, 2022

View reviewed changes

Merge branch 'opensearch-project:main' into main

a24c895

Rishikesh1159 mentioned this pull request Feb 10, 2022

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

Merged

5 tasks

Rishikesh1159 closed this Feb 10, 2022

Conversation

Rishikesh1159 commented Dec 15, 2021

Description

Issues Resolved

Check List

Uh oh!

opensearch-ci-bot commented Dec 15, 2021

Uh oh!

Rishikesh1159 commented Dec 15, 2021

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Dec 15, 2021

Uh oh!

opensearch-ci-bot commented Dec 15, 2021

Uh oh!

dblock commented Dec 15, 2021

Uh oh!

opensearch-ci-bot commented Dec 15, 2021

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

saratvemulapalli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Dec 17, 2021

Uh oh!

opensearch-ci-bot commented Dec 17, 2021

Uh oh!

opensearch-ci-bot commented Dec 17, 2021

Uh oh!

Rishikesh1159 commented Dec 17, 2021

Uh oh!

opensearch-ci-bot commented Dec 17, 2021

Uh oh!

Rishikesh1159 commented Dec 17, 2021

Uh oh!

opensearch-ci-bot commented Dec 17, 2021

Uh oh!

Rishikesh1159 commented Dec 17, 2021

Uh oh!

saratvemulapalli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saratvemulapalli commented Jan 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Jan 7, 2022

Uh oh!

opensearch-ci-bot commented Jan 10, 2022

Uh oh!

opensearch-ci-bot commented Jan 10, 2022

Uh oh!

saratvemulapalli left a comment •

edited

Loading

saratvemulapalli left a comment •

edited

Loading

dblock left a comment •

edited

Loading

dblock commented Feb 2, 2022 •

edited

Loading