jboss-container-images / openjdk

Source To Image (S2I) image for Red Hat OpenShift providing OpenJDK
Apache License 2.0
54 stars 58 forks source link

[OPENJDK-2911] Merge all 3-templates into one #477

Closed jhuttana closed 5 months ago

jhuttana commented 5 months ago

JIRA : https://issues.redhat.com/browse/OPENJDK-2911 Could you please review the changes? My local crc is again giving some expected problems like space issue. I will fix that. Meanwhile thought to raise a PR.

Also for time being I have commented triggers section.

jmtd commented 5 months ago

Syntactically valid, loads into OpenShift OK; processing OK, creating the objects OK. Manually triggering the first buildConfig...

jmtd commented 5 months ago

First build fine; manually starting second build, completed fine; starting third build, fail: Error "Error resolving ImageStreamTag intermediate:latest in namespace jlink2: unable to find latest tagged image" for field "from".

jmtd commented 5 months ago

The stage 2 buildConfig output: key is indented wrong, it should belong under spec: but is at the same indent level. The same problem exists in jlink/jlink-builder/jlink-builder-template.yaml, I thought we'd fixed it but that must have been missed. Please fix it in the merged version. Then, please delete jlink/jlink-builder/jlink-builder-template.yaml and templates/multistage-dockerfile-buildconfig.yaml.

jhuttana commented 5 months ago

The stage 2 buildConfig output: key is indented wrong, it should belong under spec: but is at the same indent level. The same problem exists in jlink/jlink-builder/jlink-builder-template.yaml, I thought we'd fixed it but that must have been missed. Please fix it in the merged version. Then, please delete jlink/jlink-builder/jlink-builder-template.yaml and templates/multistage-dockerfile-buildconfig.yaml.

Sure Jon I will do that.

jmtd commented 5 months ago

Once I'd addressed the indenting, and I side-tracked to commit a suitable test application somewhere (https://github.com/jboss-container-images/openjdk-test-applications/pull/5), it works!

jhuttana commented 5 months ago

Once I'd addressed the indenting, and I side-tracked to commit a suitable test application somewhere (jboss-container-images/openjdk-test-applications#5), it works!

Actually I just wanted to add QUARKUS_PACKAGE_TYPE=uber-jar to the parameter list. Its good that you have added that to .s2i/environment in the application itself. Thank you!

jhuttana commented 5 months ago

@jmtd To move templates/jlink/jlink-builder/README.md to templates/jlink/ there is one conflict, because already we have a README.md there which was created for multistage Docker build. If we want to retain that then we will have to name templates/jlink/jlink-builder/README.md to `templates/jlink/README_3PHASE.md is that fine?

jhuttana commented 5 months ago

The template in this PR worked fine for all the three stages and created the expected outputs.

$ oc get builds
NAME                       TYPE     FROM          STATUS     STARTED          DURATION
jlink-builder-jdk-17-1     Docker   Dockerfile    Complete   19 minutes ago   1m9s
jlink-s2i-jdk-17-1         Source   Git@0635f38   Complete   16 minutes ago   6m23s
multistage-buildconfig-2   Docker   Dockerfile    Complete   5 minutes ago    46s

$ oc get is
NAME                    IMAGE REPOSITORY                                                                        TAGS                                                      UPDATED
intermediate            default-route-openshift-image-registry.apps-crc.testing/phase-1/intermediate            latest                                                    10 minutes ago
java                    default-route-openshift-image-registry.apps-crc.testing/phase-1/java                    11,8,latest,openjdk-11-ubi8,openjdk-17-ubi8 + 1 more...   4 hours ago
lightweight-image       default-route-openshift-image-registry.apps-crc.testing/phase-1/lightweight-image       latest                                                    5 minutes ago
ubi9-openjdk-11         default-route-openshift-image-registry.apps-crc.testing/phase-1/ubi9-openjdk-11         1.18,1.13,1.14,1.15,1.16,1.17                             4 hours ago
ubi9-openjdk-17         default-route-openshift-image-registry.apps-crc.testing/phase-1/ubi9-openjdk-17         1.18,1.13,1.14,1.15,1.16,1.17                             21 minutes ago
ubi9-openjdk-17-jlink   default-route-openshift-image-registry.apps-crc.testing/phase-1/ubi9-openjdk-17-jlink   latest                                                    18 minutes ago
ubi9-openjdk-21         default-route-openshift-image-registry.apps-crc.testing/phase-1/ubi9-openjdk-21         1.17,1.18                                                 4 hours ago
ubimicro                default-route-openshift-image-registry.apps-crc.testing/phase-1/ubimicro                latest                                                    20 minutes ago

$ oc get bc
NAME                     TYPE     FROM                  LATEST
jlink-builder-jdk-17     Docker   Dockerfile            1
jlink-s2i-jdk-17         Source   Git@quarkus-uberjar   1
multistage-buildconfig   Docker   Dockerfile            2

$ oc get templates
NAME                 DESCRIPTION                                                                 PARAMETERS    OBJECTS
jlink-app-template   Template to produce imagestreams and buildconfigs for jlinked application   4 (3 blank)   7
jmtd commented 5 months ago

To move templates/jlink/jlink-builder/README.md to templates/jlink/ there is one conflict, because already we have a README.md there which was created for multistage Docker build.

The existing templates/jlink/README.md for the multistage docker build (as well as the multistage docker build sources) can be removed.

jmtd commented 5 months ago

The process worked for me! Please remove the old templates/jlink/README.md, move the other the other one into place and I can merge.

jhuttana commented 5 months ago

Do we need to update README.md to match new template jlinked-app.yaml ? Or is it good to do that in another PR?

jmtd commented 5 months ago

Do we need to update README.md to match new template jlinked-app.yaml ? Or is it good to do that in another PR?

Another PR