Override Default Distribution Download Url with Custom Distribution Url when it is passed from Plugin #2420
Conversation
Signed-off-by: Rishikesh1159 <[email protected]>
…lt Distribution Download URL with custom Distribution URL Signed-off-by: Rishikesh1159 <[email protected]>
…passed from Plugins Signed-off-by: Rishikesh1159 <[email protected]>
|
Can one of the admins verify this patch? |
|
I was able to root cause failure in Alerting plugin. In order to fix this issue #1240, in my previous PR I moved call to setupDownloadServiceRepo() in DistributionDownloadPlugin.java from apply() method to setupDistributions() method, which is called in afterEvaluate(). Here is previous PR: #2086 which is reverted. -> This change made to setup repo at the end of configuration phase [One of gradle life cycle process] after entire root and sub projects are configured. -> Internally in subprojects of Alerting plugin in configuration phase call to the repo is being made before it was actually setup. But the setup will happen after configuration phase ends. So, this was the reason Alerting plugin was failing. -> Temporarily we moved back call to setupDownloadServiceRepo() in DistributionDownloadPlugin.java to apply() method as it was before. Here the repo is setup before evaluation happens. But, it does not fix this issue. Fix: -> The fix I found was to use Project properties to set the custom url from plugin instead of system properties. Before, I was trying to use system properties to set custom url from plugin. -> These system properties are not available for us to use them in configuration phase, we had to wait until configuration phase ends and afterEvaluate() happens to use these system properties. ->But Project properties are different and are available in configuration phase. -> So, the fix would be using Project properties to set custom url from plugin side. -> So no moving of seupDownloadServiceRepo() call out of apply() in DistributionDownloadPlugin.java is necessary and just adding conditions and logic of cutom Url is enough to fix this issue. Test case for handling Alerting plugin failure: -> I haven't thought of a way writing test case for this failure. As, it is happening because of moving method call in gradle life cycle phase, I find it a bit complex to write a test case for this scenario. opensearch-project/alerting#312 |
|
Here is the exact line where Alerting plugin fail with my previous reverted PR #2086. There are other plugins using similar piece of code but for those this piece of code is wrapped inside an after evaluate block, which is making it not fail. So, test case mentioned in my above comment will be related to this. This was the error on Alerting plugin side: opensearch-project/alerting#312 |
|
This PR fixes the issue, but I am planning to keep it open until we write a test case for above mentioned failure. If someone in future tries to do same mistake as I did then a test case should catch that. So, I am planning on keeping this open until I do that. @dblock please let me know your opinion on this, should we keep this PR open until we write a test case or should that be a new issue? |
Because this is an issue in the tooling and not the engine, I would timebox writing the test for this, and move on if it takes too long. |
There was a problem hiding this comment.
Thanks for this @Rishikesh1159 !
I think we do lack the infrastructure to test these kinds of cases.
Lets start with opening an issue and as we discussed see how other opensource gradle plugins are being tested. We should bring in a similar framework to test these cases.
Sure. Thank you! |
Sure I will look for other opensource gradle plugins, and see if their framework fits for our use case. Thank you! |
|
Can we merge this? |
Description
This PR overrides default distribution url with custom distribution url passes by user from plugin.
Issues Resolved
#1240
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.