-
Notifications
You must be signed in to change notification settings - Fork 18
Onboard jaz to Microsoft's OpenJDK container images #132
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
Conversation
| # Redeclare ARG to make it available in this build stage | ||
| ARG INSTALLER_TAG | ||
| ARG JDK_VERSION | ||
| ARG JDK_URL="https://aka.ms/download-jdk/microsoft-jdk-${JDK_VERSION}-linux-ARCH.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a variable or something? (Perhaps a nicety of dockerfiles?)
| ARG JDK_URL="https://aka.ms/download-jdk/microsoft-jdk-${JDK_VERSION}-linux-ARCH.tar.gz" | |
| ARG JDK_URL="https://aka.ms/download-jdk/microsoft-jdk-${JDK_VERSION}-linux-$ARCH.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noup, it is a word that gets replaced with the proper architecture. It is good as it is.
scripts/validate-image.sh
Outdated
| if [[ "$jaz_version_string" =~ "jaz version:" ]]; then | ||
| echo "::notice title=JAZ present ($jdkversion-$distro)::Image '${image}' has JAZ installed." | ||
| else | ||
| echo "::warning title=JAZ missing ($jdkversion-$distro)::Image '${image}' does not have JAZ installed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to just have a warning or should it fail? We will add jaz to all of our images correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that is what we want (to fail).
|
You have some warnings and issues for JDK25 |
Jaz team pushed a new version, and this is now resolved. |
jmjaffe37
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I am ready to approve when all remaining comments are addressed :)
jmjaffe37
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This change will add the
jazpackage to Microsoft's OpenJDK container images. In addition to adding the new package, there are additional smoke tests which were added to verify thejazinstallation.validate-image.sh, we now check to determine that a version ofjazis installed. If we are able to validate specific versions, then we can introduce that logic into this change. Though for now, ideally, we want to ensure thatjazis included with the images.2. Enables execution thetest-image.shscript in CI. This script will create containers which will run simple java programs. One of the containers will run invokingjazand ensures that any argumentsjazgenerates as defaults are accepted byjava.Note: Some files were using CRLF line endings those were changed to use LF endings.