Adding gradle wrapper through distribution is now possible

[Jenson3210]

• Fixes Update gradle wrapper adds wrong wrapper when customDistributionUrl is requested. #4490

By allowing only distributionUri OR version/distribution, we also no longer use gradle services to determine the version which could not be available on the distributionUrl

Follow up from

• Custom distributionUrl in gradle properties should be upgraded in place #4445

[Jenson3210]

Strange... this failing test succeeds locally 🤷‍♂️
Would you have any idea @timtebeek ?

[timtebeek]

Linking the failure here for easy access:

UpdateGradleWrapperTest > servicesGradleOrgUnavailable() FAILED
    java.lang.AssertionError: 
    Expecting actual not to be null
        at org.openrewrite.gradle.UpdateGradleWrapperTest.lambda$new$0(UpdateGradleWrapperTest.java:58)
        at org.openrewrite.gradle.UpdateGradleWrapperTest$$Lambda$1061/0x00007fd74485ef88.apply(Unknown Source)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:561)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:130)
        at org.openrewrite.gradle.UpdateGradleWrapperTest.servicesGradleOrgUnavailable(UpdateGradleWrapperTest.java:712)

I'd have to dive in, but I'm about to head out. Will try to circle back!

[shanman190]

So the intent of the parameters ${version} and ${distribution} for the wrapper URI, was to make reasoning about the recipe a little easier when being used within an enterprise environment that does choose to install distributions locally. These parameters have always been meant to be previously known, exact versions and not allowing dynamic resolution as is the case with the default path against services.gradle.org. It's pretty equal though in just specifying the full wrapperUri as well.

[Jenson3210]

I could always alter the validation to allow for exact versions and distributions + do the replacements in distributionurl if that sounds better for you?

But at this moment it's using services.gradle for the resolution if mentioned.

I also believe the distributionurl is more important in deciding the impact than the fact if services.gradle is available or not. At our site we have access to services.gradle, but have custom wrappers in local repo.
It is impossible to get there with current implementation as we receive a version from gradle services repo. If you mention a url, you already know you want specific behaviour.

[shanman190]

I don't think resolving the dynamic versions should be in scope right now. The reasons that it's been avoided so far as that each repository hosting tool has a different schema for versions, so it would be difficult to make a fully workable solution that covers all cases including within an environment that completely disallows access to services.gradle.org.

Personally, I don't see much of a difference between the wrapperUri being fully defined versus the exact version parameterized replacements because they both are right there in the same spot and must be configured by the user at the time of defining the recipe. Maybe @timtebeek or @sambsnyd have some more opinions, but I've been more inclined to just use the wrapperUri as is with no replacements like you've done here in this PR.

[Laurens-W]

The only slight concern I see is that with this change we completely disregard the checksum when using the wrapperUri. Then again I agree with @shanman190 that we shouldn't be looking at implementing full support for every hosting tool here.

I think we adopt the same mindset as during my work on #4445, if the user specifies a wrapperUri we assume they know it's valid.

[shanman190]

I think the checksum part is a non-issue because if the URL is anything except a services.gradle.org, it's most likely that the wrapper is being updated per some organization policy. At that point, the checksum would have been invalid anyway.

As @Jenson3210 was pointing out on the linked issue, in their case services.gradle.org is available, but they were also setting an internal wrapperUri. As a result, the custom wrapperUri wasn't being used.

I could see a case being made to expose the checksum as a potential parameter though, so that a user could still provide it for a custom wrapperUri.

[Jenson3210]

@shanman190 , @Laurens-W I added the checksum property like you mentioned above.

[timtebeek]

Strange... this failing test succeeds locally 🤷‍♂️ Would you have any idea @timtebeek ?

Interesting; I saw the following locally:

1. Run all tests the first time: UpdateGradleWrapperTest#servicesGradleOrgUnavailable fails
2. Run servicesGradleOrgUnavailable in isolation: test passes
3. Run all tests a second time: all tests pass
4. rm -rf ~/.rewrite/remote (via DEFAULT_ARTIFACT_CACHE)
5. goto 1.

I suspect it's to do with tests using overlapping remote files, and one filling/polluting the cache for another.
Hope that's enough for you to have another go at fixing the test failure definitively.

[Jenson3210]

Adapted the test with the following reasoning:

The addition of a wrapper downloads the necessary scripts/jar from the downloadURI. If that service is unavailable, it makes sense that the result is empty.

As that test before was only bumping/updating wrapper version in url, I added the wrapper files and expect that they will not be touched if downloaduri is not available. With this before state, it is better representing the current situation if the bat/sh/jar files are present already.

[timtebeek]

Looks good to me! I especially like the validate() change to clear out conditionals in getGradleWrapper(). Thanks for picking this up!

Since @shanman190 had some feedback earlier I'll leave a decision to merge up to him.

[shanman190]

I don't think this section is necessary as the wrapperUri would already be what's used in the GradleWrapper object from the getGradleWrapper() invocation.

On the same version part of the equation do we want to be changing the distribution URL without a version change?

[Jenson3210]

That's not necessarily true i think.

Imagine calling the updatewrapper("8.2", null...) with a custom url already present in the properties file. Getwrapper would return the gradle services hosted wrapper (as no custom url is given) with sha and jarsha. We want to early on replace that with the custom distribution url wrapper without shas so that later replacement could be removed in the properties entry visitor (as that was where the patch of url happened before) + would add a sha in the properties visitor (previous bug I discovered during adding tests)

[shanman190]

Ok, I see. The previous case is we weren't able to resolve while this case is we were.

Any reason not to just merge these two of blocks together then? So that if the distribution host is null or doesn't match?

There's an additional edge case of going from internal to external hosting that could be another possibility.