add support for adoptopenjdk binaries #67
Conversation
|
The option name “distro” makes much more sense here than “vendor” IMO (see examples at #13 (comment)). I’d go with “adopt” for the AdoptOpenJDK distro (likely to last longer given recent gripes around the use of the “OpenJDK” trademark in names as opposed to the fair use rules laid out on the OpenJDK site). I think we should structure to accommodate several distros, and pre-populate some of the additional likely distro names: zulu This way folks can later just tweak the (initially empty?) implementation for each distro to enable them. I’m pretty sure at least a couple of them will fill those in pretty quickly... Should we break out the distro-specific items to a separate .ts file? |
@giltene updated to have separate files.
Agreed, updated
I'd prefer to stick with AdoptOpenJDK for now. In the event of a rebrand, it will be very easy to resolve this with an alias |
gdams
force-pushed
the
adopt
branch
14 times, most recently
from
8d8cb42 to
5382c49
Compare
|
For the readme, we should probably add a section ( at the bottom?) describing the possible distro options and their limitations (if any). I anticipate that we as we move past having only one distro we will quickly get to 4-7, and with each distro potentially having different limitations (only some version syntax supported, only some package types, platforms, or architectures supported, possibly only some versions or ranges of versions supported), we can state the limitations of each in a concise way under their distro description in the readme. The version specification description at the top should remain generic (no “Zulu only” or adopt specific notes), and we can add a small subnote saying “see distro-specific limitations below”. A key purpose purpose of the limitation notes (which each distro can list separately if they want) is to intercept a user being surprised that a a specification that is covered in the general way for specifying a version and package doesn’t work in a specific distro for some reason (e.g. using semver range syntax for adopt in current implementation, or “Oracle OpenJDK does not have jdk+fx or jre packages available”). We don’t have to list all limitations, or even any (you can leave them blank and have people just figure out why something fails), but they would be helpful for distros to include. As limitations are removed (full semver syntax supported, additional package types or platforms added), distributions can update the notes, but we probably don’t want to be churning the readme often to e.g. describe the full set of versions supported by each distro, as that would have us editing the readme many times each quarter, and likely having it be stale and incorrect most of the time. |
|
This should be good to land, is somebody happy to merge? |
Please address the previous notes on the readme changes you propose... (e.g. remove the "XYZ only" additions to some of the version specification syntax, introduce a distros list to the readme, with notes that each distro can have (as needed) about any limitations or specific expectations.) |
gdams
force-pushed
the
adopt
branch
3 times, most recently
from
8154901 to
1223d9e
Compare
| @@ -19,7 +19,8 @@ steps: | |||
| - uses: actions/checkout@v2 | |||
| - uses: actions/setup-java@v1 | |||
| with: | |||
| java-version: '9.0.4' # The JDK version to make available on the path. | |||
| java-version: '11' # The JDK version to make available on the path. | |||
| vendor: adoptopenjdk # (adoptopenjdk or zulu) - defaults to adoptopenjdk | |||
There was a problem hiding this comment.
Looks like it should be distro instead of vendor?
| @@ -6,8 +6,8 @@ | |||
| "main": "dist/index.js", | |||
| "scripts": { | |||
| "build": "ncc build -o dist/setup src/setup-java.ts && ncc build -o dist/cleanup src/cleanup-java.ts", | |||
| "format": "prettier --write **/*.ts", | |||
| "format-check": "prettier --check **/*.ts", | |||
| "format": "prettier --write **/*.ts src/distro/*.ts", | |||
There was a problem hiding this comment.
nitpick: I think src/distro/*.ts should be covered by **/*.ts?
There was a problem hiding this comment.
that was what I thought but for some reason prettier wasn't picking it up
| async function setupVendor(distro: string) { | ||
| switch (distro) { | ||
| case 'adoptopenjdk': | ||
| [jdkFile, version] = await getJavaAdoptOpenJDK( | ||
| version, | ||
| javaPackage, | ||
| arch | ||
| ); | ||
| break; | ||
| case 'zulu': | ||
| [jdkFile, version] = await getJavaZulu(version, javaPackage); | ||
| break; |
There was a problem hiding this comment.
I suggest to move this function outside of getJava function to improve readability
There was a problem hiding this comment.
I'd also echo the "change vendor to distro" and ask to use that everywhere for consistency, including in the code. So this should be setupDistro()...
| const http = new httpm.HttpClient('setup-java', undefined, { | ||
| allowRetries: true, | ||
| maxRetries: 3 | ||
| }); |
There was a problem hiding this comment.
May be we could use some inheritance here.
We can create base class JavaInstaller and two child classes: AzulInstaller, AdoptInstaller. It would allow us to move some common logic to base class and avoid code duplication.
For example, http initialization could be put to constructor and re-used.
|
We've decided to move forward with a A lot of the hard work has already been done in this PR so we can use this once the ADR is accepted. A bit of restructuring will be needed but we'll get this across the finish line 😃 If anyone would like to offer some feedback or suggestions in the ADR, I would greatly appreciate it! |
|
Closing since V2 was released recently. |
partially resolves #13