-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Use gradlew in native image docs #24969
Conversation
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.
Thanks for the PR, I left few comments.
build-parent/pom.xml
Outdated
@@ -39,7 +39,7 @@ | |||
<!-- These properties are needed in order for them to be resolvable by the documentation --> | |||
<!-- The Graal version we suggest using in documentation - as that's | |||
what we work with by self downloading it: --> | |||
<graal-sdk.version-for-documentation>22.0.0.2</graal-sdk.version-for-documentation> | |||
<graal-sdk.version-for-documentation>22.0</graal-sdk.version-for-documentation> |
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 that change necessary for documentation update?
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.
Reverted.
@@ -566,6 +566,8 @@ Sample Dockerfile for building with Gradle: | |||
---- | |||
## Stage 1 : build with maven builder image with native capabilities | |||
FROM quay.io/quarkus/ubi-quarkus-native-image:{graalvm-flavor} AS build | |||
USER root | |||
RUN microdnf install findutils |
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 find utils necessary for building the native artifact?
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.
gradlew ( https://github.com/gradle/gradle/blob/master/gradlew , line 235 ) uses xargs which are not in the image by default.
docs/pom.xml
Outdated
@@ -2786,7 +2786,7 @@ | |||
<quarkus-version>${project.version}</quarkus-version> | |||
<surefire-version>${version.surefire.plugin}</surefire-version> | |||
<graalvm-version>${graal-sdk.version-for-documentation}</graalvm-version> | |||
<graalvm-flavor>${graal-sdk.version-for-documentation}-java11</graalvm-flavor> | |||
<graalvm-flavor>${graal-sdk.version-for-documentation}-java17</graalvm-flavor> |
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.
I know if we want to set all documentation reference to java 17 as quarkus is still based on Java 11
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.
Reverted.
Just a note that we'll need to have the commits squashed once everything else is addressed |
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, @KlemenDanfoss could you squash your commits and name it with the intent of the modification ?
3f5dc9e
to
4785d6c
Compare
Done. |
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.
Thanks for those fix, the overall looks good to me. Could you just rephrase your commit message to something like "Use gradlew in native image docs", this will be used for the changelog.
5c87147
to
9486032
Compare
Updated version, updated sinstructions