-
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
Native S2I Builder image #1358
Native S2I Builder image #1358
Conversation
How cool is this? (for #304) |
Looks great! |
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. Added some minor suggestions, feel free to approve or reject them 😄
Co-Authored-By: vorburger <[email protected]>
Co-Authored-By: vorburger <[email protected]>
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
Very good stuff! |
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.
That's pretty cool, I've made a few comments.
LABEL io.openshift.s2i.scripts-url="image:///usr/local/s2i" \ | ||
io.openshift.s2i.destination="/tmp" \ | ||
|
||
io.k8s.description="Platform for building and running Quarkus.io Native Java applications" \ |
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.
The wording is not totally in line with the website - check https://github.com/quarkusio/quarkus/blob/master/docs/src/main/asciidoc/0-glossary.adoc.
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've attempted to adhere to that in an upcoming update (and raised #1377), hopefully better now.
oc new-app quarkus-native-s2i~https://github.com/quarkusio/quarkus-quickstarts --context-dir=getting-started-native --name=getting-started-native | ||
|
||
Note that GraalVM-based native build are more memory & CPU intensive than regular pure Java builds, | ||
and you therefore may need to increase the quota for OpenShift's S2I builder containers. |
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.
Can you give some hint about this?
I guess you can edit the build object.
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 was hoping someone else would help with how exactly to do that... 😃
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.
There now is at least some doc. Could always be further improved, but god enough for a 0.1?
BTW, how will we version the S2I, during the release process? If so how does that work? |
Not 100% sure I know what you mean, but if you are asking how we will release a container image from this Dockerfile to e.g. Quay.io (and/or Docker Hub), then that's trivial to set up (IFF you keep a single |
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.
A few comments
|
||
ENV PATH=$PATH:"/usr/local/s2i" | ||
|
||
RUN mkdir /quarkus.io/ |
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.
Really minor, but I try to about .
in my directory name. What about just "quarkus"
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.
it's now /home/quarkus
OK for you?
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.
Yes
This should fix this S2I build error seen on OpenShift: build error: image "..." must specify a user that is numeric and within the range of allowed users
* master: (167 commits) Allow for usage of @ConfigProperty on fields without the need for @Inject. Fixes quarkusio#1315. Expose all flavors (core, axle, rx) of Vert.x EventBus RestClient - passing incoming JAX-RS headers, fixes quarkusio#1017 JAX-RS body writers for Vert.x JsonObject and JsonArray Increase CI memory PathTestHelper should accept default Gradle build paths now + make sure we run the test in gradle-it Change to use ./mvnw Update opentracing guide to describe using application.properties as alternative to env vars fix npe in dev mode when source is missing in recompilation diagnostic Add the quarkus-elytron-security component in the BOM Moves the logic that handles the registration of schema class for serialization in smallrye-openapi-extension Add Maven Wrapper to generated projects [security extension] - AuthConfig only configured if IdentityManager is produced quarkusio#1522 Warning to keep H2 version as is Minor corrections to mx and git repo clone instructions. programmatic client generation for MP Rest Client, fixes quarkusio#1317 Hibernate ORM - Fix config property name in error message Camel - Use a constant for Camel root package directory Micro version updates JAX-RS Generate reflection declaration from MP OpenAPI schema definition ...
@cescoffier I've further tested this, and (from my side) this is good for merge now. |
Waiting for CI and will merge. |
# This part is heavily inspired by ../centos-graal/Dockerfile | ||
FROM centos:latest | ||
|
||
ARG GRAAL_VERSION=1.0.0-rc13 |
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.
In the future we would need to have a way to easily remember to update this every time we are working on updating to the latest GraalVM version
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.
Yup, just like in a few other places where it appears.
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.
👍
# a TODO future PRODUCTION version of this would probably want tomake it possible to | ||
# e.g. pass additional parameters to the executable, as does fabric8io-images/s2i's run script. | ||
|
||
${QUARKUS_HOME}/*-runner -Dquarkus.http.host=0.0.0.0 |
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.
-Dquarkus.http.host=0.0.0.0
shouldn't be needed anymore since it's now the default
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.
But is that already released, right now? I would prefer to keep this and then see it removed a while after a new release, together with more generally cleaning this up in many places where it currently still appears.
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.
The fix is in master and will be present in the 0.12.0
release that I think @cescoffier will be doing tomorrow.
I'll leave it up to you to best decide how to handle this :)
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 guess we should keep it based on this #1312 (comment)
@cescoffier merge? |
Thanks! |
No description provided.