Automatically determine appropriate URL based off platform in VersionDownloadSource - #96
Conversation
bodowd
changed the title
9b4feba to
8f2b025
Compare
| @Override | ||
| public URL downloadUrl() throws MalformedURLException { | ||
| return new URL(String.format(Locale.ENGLISH, VERSION_DOWNLOAD_URL, version)); | ||
| return buildDownloadUrl(this.version, platform( |
There was a problem hiding this comment.
This bit of indirection made it easier to write unit tests on buildDownloadUrl and platform
| System.getProperty("os.arch"))); | ||
| } | ||
|
|
||
| static URL buildDownloadUrl(String version, String platform) throws MalformedURLException { |
There was a problem hiding this comment.
This and platform could potentially be in Utils and re-used for other tests like ShutdownTest. What do you think?
There was a problem hiding this comment.
Could be done later when/if we adjust ShutdownTest to be able to run locally from aarch64_mac
…DownloadSource Currently, downloading the cratedb releases assumes the following URL "https://cdn.crate.io/downloads/releases/crate-%s.tar.gz" However, this will not work if the user is running on aarch64_mac, and potentially other platforms. We now check the architecture of the system running the code and then determine the appropriate URL for downloading the desired cratedb release.
8f2b025 to
467b8de
Compare
| case X64_WINDOWS: | ||
| return new URL(String.format(Locale.ENGLISH, RELEASE_PLATFORM_URL, platform, version)); | ||
| case X64_LINUX: | ||
| return new URL(String.format(Locale.ENGLISH, RELEASE_URL, version)); |
There was a problem hiding this comment.
Just fyi: x64_linux is also available under the platform specific url: https://cdn.crate.io/downloads/releases/cratedb/x64_linux/
That they're also under https://cdn.crate.io/downloads/releases/ is historic before we had builds for any other platforms, but there's no need to use this location - would be possible to unify the logic further.
3 participants
Summary of the changes / Why this is an improvement
Related to this issue: crate/jmx_exporter#102
Related to PR: crate/jmx_exporter#104 so that jmx_exporter can simply pass a desired CrateDB version for download to crate-java-testing. This PR makes it easier to test different releases on jmx_exporter from different systems, not only Linux.
Currently,
CrateTestCluster.fromVersionwill download fromhttps://cdn.crate.io/downloads/releases/crate-%s.tar.gzHowever, if the client is running on aarch64_mac, for example, the releases at this URL are not suitable for the client's system. There are releases for specific architectures that should be used instead. For example: https://cdn2.crate.io/downloads/releases/nightly/aarch64_mac/
Currently, the client must work around this by browsing https://cdn2.crate.io/downloads/releases/ to find the url to the release they want and then call
CrateTestCluster.fromURL.This PR allows the client to simply pass the desired version they want to download and it abstracts away figuring out the appropriate CrateDB release to download.
Note
This PR does not yet address making tests run locally from aarch64_mac. Further changes would be necessary for that, for example here:
crate-java-testing/src/test/java/io/crate/testing/ShutdownTest.java
Lines 9 to 20 in 67b63a4
This still has a hardcoded URL. The workaround in
DEVELOP.rstis still necessary if developers want to run local tests on macosChecklist