Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Liberica JDK #225

Merged
merged 7 commits into from Nov 29, 2021
Merged

Add support for Liberica JDK #225

merged 7 commits into from Nov 29, 2021

Conversation

RazorNd
Copy link
Contributor

@RazorNd RazorNd commented Sep 12, 2021

Description:
Add support for Liberica JDK distribution.

Related issue:
#70

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@dchuyko
Copy link

dchuyko commented Nov 2, 2021

Hi. Daniil, a nice PR for Actions. Can we assist somehow? Workflows are awaiting approval for some time, probably should ping maintainers/owners. @chrispat @giltene any advice?

@maxim-lobanov
Copy link
Contributor

Hey,
We will take a look at this PR.
cc: @dmitry-shibanov @MaksimZhukov

@maxim-lobanov
Copy link
Contributor

@RazorNd, I have left a couple of nitpicks. If you can fix them and pull latest main / rebuild code, it would be awesome.
Also, I think @dmitry-shibanov @MaksimZhukov can help with e2e testing of these changes since their team owns this action for now.

Co-authored-by: Maxim Lobanov <maxim-lobanov@github.com>
@RazorNd
Copy link
Contributor Author

RazorNd commented Nov 2, 2021

@RazorNd, I have left a couple of nitpicks. If you can fix them and pull latest main / rebuild code, it would be awesome. Also, I think @dmitry-shibanov @MaksimZhukov can help with e2e testing of these changes since their team owns this action for now.

Thanks for your corrections. I fixed them. I pulled main 6 days ago. This PR is up to date.

@dchuyko
Copy link

dchuyko commented Nov 9, 2021

@dmitry-shibanov Are there any issues? Anything that blocks approving the workflows?

@dmitry-shibanov
Copy link
Contributor

Hello @RazorNd. Thank you for your pull request. Could you please add e2e tests. I've tested your changes and got error during java installation. I've used such workflow as:

name: Test java
on: [push]
jobs:
  run:
    name: Test java ${{ matrix.version }} ${{ matrix.java-package }} ${{ matrix.os }}
    runs-on: ${{ matrix.os }}
    strategy:
      fail-fast: false
      matrix:
        os: [ubuntu-18.04, macos-latest, windows-latest]
        version: [ '8', '11', '13', '15', '16', '13-ea', '15-ea', '16-ea' ]
        java-package: ['jdk', 'jre']
    steps:
    - name: Checkout
      uses: actions/checkout@v2

    - uses: RazorNd/setup-java@liberica
      with:
        java-version: ${{ matrix.version }}
        distribution: liberica
        java-package: ${{ matrix.java-package }}
    - name: Java version
      shell: pwsh
      run: java -version

@RazorNd
Copy link
Contributor Author

RazorNd commented Nov 10, 2021

@dmitry-shibanov thanks for the hint. I fixed the errors that occurred during e2e testing. build example Also I tried adding e2e test in current workflow. Update for worflow. Add these changes to the current PR?

@dchuyko
Copy link

dchuyko commented Nov 11, 2021

@RazorNd As far as I can see other recent additions (openj9) included e2e-versions.yml changes.

@dmitry-shibanov
Copy link
Contributor

Hello @RazorNd. Thank you for updating branch with e2e tests. I've tested your changes from my side. In general it works as expected, but I have one minor question:

  • Does Liberica jdk provide ea versions ? I've tried to install couple of early access versions, but array of possible versions was empty.

@RazorNd
Copy link
Contributor Author

RazorNd commented Nov 11, 2021

@dchuyko I was not sure if changing the pipeline for a non-owner is a good idea. But as a last resort I'll revert my changes.

@RazorNd
Copy link
Contributor Author

RazorNd commented Nov 11, 2021

Hello @RazorNd. Thank you for updating branch with e2e tests. I've tested your changes from my side. In general it works as expected, but I have one minor question:

  • Does Liberica jdk provide ea versions ? I've tried to install couple of early access versions, but array of possible versions was empty.

@dmitry-shibanov I also noticed this yesterday. But if you look in API, Liberica has only 2 early access builds for Java 8. @dchuyko you don't release anymore early access builds? Or am I doing something wrong? API Request Example

@dchuyko
Copy link

dchuyko commented Nov 11, 2021

  • Does Liberica jdk provide ea versions ? I've tried to install couple of early access versions, but array of possible versions was empty.

Currently EA is not among public artifacts (new versions). But it may appear again.

Copy link

@Hampedude Hampedude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah very good n smart

@dmitry-shibanov
Copy link
Contributor

Hello @giltene. If you do not mind, could you please take a look on this pull request ?

Copy link
Contributor

@MaksimZhukov MaksimZhukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @RazorNd! Thank you for the contribution!
Overall looks good to me! I've left a few minor comments

README.md Show resolved Hide resolved
.github/workflows/e2e-versions.yml Outdated Show resolved Hide resolved
@RazorNd
Copy link
Contributor Author

RazorNd commented Nov 25, 2021

@MaksimZhukov thanks for you comments. I have resolved all the problems you are reporting.

@dmitry-shibanov
Copy link
Contributor

Hello @RazorNd. We want to describe our next steps. We will merge your pull request and release a new version next week. We will notify you when we release a new version and update the tag v2.
Thanks for your pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants