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

Download of JDK from the Elastic catalog instead of Adoptium #15514

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

andsel
Copy link
Contributor

@andsel andsel commented Oct 30, 2023

Release notes

[rn:skip]

What does this PR do?

Adapted the JDK's download URL creation to intereact with Elastic catalog to get metadata, and return the catalog download link instead of directly pointing to Adoptium API

Why is it important/What is the impact to the user?

Let the CI work smoothly, using the unified source of JDKs.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • [ ] I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

Use Gradle task:

./gradlew copyJdk -Pjdk_bundle_os=windows -Pjdk_arch=x86_64

using differn OSes (windows, linux, darwin) and different CPU architectures (x86_64,arm64).
arm64 is valid only for MacOS (Darwin).

Related issues

…alog to get metadata, and return the catalog download link instead of directly pointing to Adoptium API
@andsel andsel added the build label Oct 30, 2023
@andsel andsel self-assigned this Oct 30, 2023
@andsel andsel changed the title Switch download of JDK from Elastic catalog Switch download of JDK from the Elastic catalog Oct 30, 2023
@andsel andsel changed the title Switch download of JDK from the Elastic catalog Download of JDK from the Elastic catalog Oct 30, 2023
@andsel andsel changed the title Download of JDK from the Elastic catalog Download of JDK from the Elastic catalog instead of Adoptium Oct 30, 2023
build.gradle Show resolved Hide resolved
// arch x86_64 never used, only aarch64 if macos
def url = "https://jvm-catalog.elastic.co/jdk/latest_adoptiumjdk_${major}_${osName}"

// Append the cpu's arch only if Mac on aarch64, all the other OSes doesn't have CPU extension
Copy link
Contributor

@dliappis dliappis Oct 30, 2023

Choose a reason for hiding this comment

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

Hm, I don't think that's correct, for example for Linux aarch64 artifacts we have https://jvm-catalog.elastic.co/jdk/latest_adoptiumjdk_17_linux_aarch64, for Windows e.g. /jdk/latest_zulu_19_windows_aarch64 or even 32bit variants like /jdk/latest_adoptopenjdk_16_windows_x86_32.

Or maybe I musunderstood the comment as in we don't need it, but for DRA my understanding is that we need all combinations e.g. in https://buildkite.com/elastic/logstash-dra-snapshot-pipeline/builds/98#018b6ef8-3807-4234-8cd3-10b2e9c0fb8e/70-298 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't use a generalized URL template like https://jvm-catalog.elastic.co/jdk/latest_adoptiumjdk_${major}_${osName}_${arch} because the url https://jvm-catalog.elastic.co/jdk/latest_adoptiumjdk_17_linux_x86_64 return a

Could not find download for latest_adoptiumjdk_17_linux_x86_64

But the https://jvm-catalog.elastic.co/jdk/latest_adoptiumjdk_17_linux return the x86_64 artifact.

So we could use the arch format iff it's an ARM (not only for MacOS but also for Linux).

WDYT?

Copy link
Contributor

@dliappis dliappis Oct 30, 2023

Choose a reason for hiding this comment

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

I agree. We can use the default URL for x86_64 and use a suffix (aarch64 in our case) for the specific aarch64 artifacts for macOS, Linux or Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by b968888

@andsel andsel requested review from jsvd and dliappis October 30, 2023 12:46
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM

As discussed elsewhere, I triggered an aarch64 build which run fine with this PR: https://buildkite.com/elastic/logstash-pull-request-pipeline/builds/151 (compared to e.g. https://buildkite.com/elastic/logstash-aarch64-pipeline/builds/24 which failed on Friday).

Would be nice to check if windows building works fine as well e.g. by triggered the main snapshot DRA job using pull/15514/merge, but from my PoV we are good here.

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

LGTM

@elastic-sonarqube
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

LGTM^2

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @andsel

@andsel andsel merged commit 73daec0 into elastic:main Oct 30, 2023
2 checks passed
@andsel
Copy link
Contributor Author

andsel commented Oct 30, 2023

@logstashmachine backport 8.11

github-actions bot pushed a commit that referenced this pull request Oct 30, 2023
* Adapted the JDK's download URL creation to intereact with Elastic catalog to get metadata, and return the catalog download link instead of directly pointing to Adoptium API

* Silenced the Download task of JDK to print the full url

(cherry picked from commit 73daec0)
andsel added a commit that referenced this pull request Oct 30, 2023
…#15515)

* Adapted the JDK's download URL creation to intereact with Elastic catalog to get metadata, and return the catalog download link instead of directly pointing to Adoptium API

* Silenced the Download task of JDK to print the full url

(cherry picked from commit 73daec0)

Co-authored-by: Andrea Selva <[email protected]>
@andsel
Copy link
Contributor Author

andsel commented Aug 21, 2024

@logstashmachine backport 7.17

github-actions bot pushed a commit that referenced this pull request Aug 21, 2024
* Adapted the JDK's download URL creation to intereact with Elastic catalog to get metadata, and return the catalog download link instead of directly pointing to Adoptium API

* Silenced the Download task of JDK to print the full url

(cherry picked from commit 73daec0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Gradle script to download JDK images using the Elastic JVM catalog
4 participants