Unset MAVEN_CONFIG env in Maven based images #160

svor · 2019-12-09T17:54:39Z

Signed-off-by: svor vsvydenk@redhat.com

What does this PR do?

This PR unsets MAVEN_CONFIG env to make it possible to run mvnw scripts.

What issues does this PR fix or reference?

eclipse-che/che#13926

Signed-off-by: svor <vsvydenk@redhat.com>

nickboldt · 2019-12-09T19:46:43Z

arbitrary-users-patch/build_images.sh

@@ -31,7 +33,11 @@ while read -r line; do
  base_image_name=$(echo "$line" | tr -s ' ' | cut -f 1 -d ' ')
  base_image=$(echo "$line" | tr -s ' ' | cut -f 2 -d ' ')
  echo "Building ${NAME_FORMAT}/${base_image_name}:${TAG} based on $base_image ..."
-  docker build -t "${NAME_FORMAT}/${base_image_name}:${TAG}" --no-cache --build-arg FROM_IMAGE="$base_image" "${SCRIPT_DIR}"/
+  script_dir=${SCRIPT_DIR}
+  if [[ ${base_image:0:5} = ${MAVEN_BASE} ]] ; then


This seems like a too-restrictive check. What if the base image name is just a java image which happens to have maven in it? https://github.com/redhat-developer/codeready-workspaces/blob/master/dependencies/che-devfile-registry/devfiles/02_java-maven/devfile.yaml#L18

Why not do a check like this?

# substring check for "java" or "maven" in ${base_image} if [[ ${base_image} = *"maven"* ]] || [[ ${base_image} = *"java"* ]]; then

Also this assumes that there's a special maven/Dockerfile to use for maven-based images... which we're not doing in downstream CRW... so this would possibly break the ability to use the same build_images.sh script. But TBH I'm not sure if we're actually using any of that.

So... +1, this seems OK.

I just don't like your "if first 5 chars of string = 'maven'" check, but YMMV. :)

I continue to be strongly against this sort of patch in the arbitrary user patch section. If we want an image with MAVEN_CONFIG unset then we should build one explicitly.

Also worth noting that this workaround is specific to the dockerhub community maven image, and is not necessary in e.g. an image where you install maven yourself.

nickboldt

+1, probably OK with no impact on downstream as our stack/sidecar images are hard forks from this upstream Alpine stuff as we have to use UBI

nickboldt · 2019-12-09T19:57:04Z

Should we do a similar change to the devfile.yaml files for these?

https://github.com/redhat-developer/codeready-workspaces/blob/master/dependencies/che-devfile-registry/devfiles/00_java-eap-maven/devfile.yaml#L20-L21
https://github.com/redhat-developer/codeready-workspaces/blob/master/dependencies/che-devfile-registry/devfiles/01_java-eap-thorntail/devfile.yaml#L20-L21
https://github.com/redhat-developer/codeready-workspaces/blob/master/dependencies/che-devfile-registry/devfiles/02_java-maven/devfile.yaml#L20-L21

And do I need to apply ENV MAVEN_CONFIG= in this dockerfile?

http://pkgs.devel.redhat.com/cgit/containers/codeready-workspaces-stacks-java/tree/Dockerfile?h=crw-2.0-rhel-8#n82

amisevsk

I'm strongly against running a image-specific patch in a general script. If we want custom images we should build custom images in their own repo.

If all that is required is setting MAVEN_CONFIG= then why can't this be done in the devfile, or alternatively, why can't we use a different maven image?

amisevsk · 2019-12-09T22:05:27Z

arbitrary-users-patch/build_images.sh

@@ -31,7 +33,11 @@ while read -r line; do
  base_image_name=$(echo "$line" | tr -s ' ' | cut -f 1 -d ' ')
  base_image=$(echo "$line" | tr -s ' ' | cut -f 2 -d ' ')
  echo "Building ${NAME_FORMAT}/${base_image_name}:${TAG} based on $base_image ..."
-  docker build -t "${NAME_FORMAT}/${base_image_name}:${TAG}" --no-cache --build-arg FROM_IMAGE="$base_image" "${SCRIPT_DIR}"/
+  script_dir=${SCRIPT_DIR}
+  if [[ ${base_image:0:5} = ${MAVEN_BASE} ]] ; then


I continue to be strongly against this sort of patch in the arbitrary user patch section. If we want an image with MAVEN_CONFIG unset then we should build one explicitly.

svor · 2019-12-10T08:12:05Z

@amisevsk My point was to unset MAVEN_CONFIG for both images che-java11-maven and che-java8-maven in one place. Probably I agree with you that it wasn't the best choice to patch them in a general script by adding maven specific logic.
So, if it's OK for you to unset MAVEN_CONFIG in all devfiles where we are using che-java11-maven and che-java8-maven, I'll do that

nickboldt · 2019-12-10T12:37:07Z

What if we built a pair of small, UBI 8 based images that work on OpenShift and include either JDK 8 or 11?

Then you would not be dependent on Alpine, would get the benefits of CVE fixes, and would provide something that could more closely be used for downstream too.

Also the JDK 11 image could be the basis for quarkus devfiles.

amisevsk · 2019-12-10T13:49:26Z

I'm happy with either option

Updating devfiles makes sense to me; we're using a particular image for the maven dependency, so the devfiles have to have a workaround for that particular image.
We can update the image we use to avoid having to unset MAVEN_CONFIG, by building a ubi-based image.

Both also have (minor) downsides:

Updating our devfiles works but might be a sticking point for users trying to reuse our "che-maven" image (they would have to figure out the same fix for their devfile, if they don't use one of ours as a base).
It's not clear how we build our UBI-based image, since we don't have a repo + CI for doing this currently.

nickboldt · 2019-12-10T18:46:52Z

It's not clear how we build our UBI-based image, since we don't have a repo + CI for doing this currently.

Yes, and that's a problem. How can you rely on an image if you have no idea how to build it? :(

How about we use quay.io/eclipse/che-quarkus:nightly as the JDK 11 / maven image?

That's the one that's ref'd in #144

I also don't know how it's built but i do see it as the last line in the list in https://github.com/eclipse/che-devfile-registry/blob/master/arbitrary-users-patch/base_images

svor · 2019-12-11T07:19:53Z

How about we use quay.io/eclipse/che-quarkus:nightly as the JDK 11 / maven image?

As a base image it's quay.io/quarkus/centos-quarkus-maven:19.2.1 and it provides Java8, GraalVM, Maven, Podman and Buildah.

Signed-off-by: svor <vsvydenk@redhat.com>

svor · 2019-12-11T09:09:29Z

@amisevsk So, as we decided in the issue eclipse-che/che#13926 (comment), I set empty value for MAVEN_CONFIG in devfiles where the sample uses mvnw utility.
If it's OK for now, i'll provide same fix for https://github.com/redhat-developer/devfile/tree/master/getting-started and https://github.com/redhat-developer/codeready-workspaces/tree/master/dependencies/che-devfile-registry/devfiles.

amisevsk

LGTM generally. Tested java-web-spring and java-maven -- mvnw works without issue now.

amisevsk · 2019-12-11T15:02:51Z

devfiles/apache-camel-springboot/devfile.yaml

@@ -26,8 +26,8 @@ components:
    alias: maven
    image: quay.io/eclipse/che-java8-maven:nightly
    env:
-      - name: MAVEN_CONFIG
-        value: "/home/user/.m2"
+      - name: MAVEN_CONFIG	


Some stray whitespace here

amisevsk · 2019-12-11T15:10:12Z

devfiles/java-maven/devfile.yaml

@@ -18,8 +18,6 @@ components:
    alias: maven
    image: quay.io/eclipse/che-java11-maven:nightly
    env:
-      - name: MAVEN_CONFIG
-        value: /home/user/.m2


I think we should also unset MAVEN_CONFIG here, since the base image has MAVEN_CONFIG=/root/.m2 by default. Our sample project doesn't use mvnw but it might trip up someone trying to import their own project.

Unset for all maven based devfiles

Signed-off-by: svor <vsvydenk@redhat.com>

amisevsk

LGTM, thanks.

* set memory limit for php-intelephense plugin Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com> * Increase memory limit for php-intelephense plugin Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

Patch maven base images to unset MAVEN_CONFIG env

0498657

Signed-off-by: svor <vsvydenk@redhat.com>

svor requested review from ibuziuk and amisevsk December 9, 2019 17:54

svor requested review from l0rd, nickboldt, rhopp, skabashnyuk and vparfonov as code owners December 9, 2019 17:54

svor self-assigned this Dec 9, 2019

nickboldt reviewed Dec 9, 2019

View reviewed changes

nickboldt approved these changes Dec 9, 2019

View reviewed changes

amisevsk requested changes Dec 9, 2019

View reviewed changes

Set empty MAVEN_CONFIG in devfiles where samples use mvnw

c73b041

Signed-off-by: svor <vsvydenk@redhat.com>

amisevsk approved these changes Dec 11, 2019

View reviewed changes

svor added 2 commits December 11, 2019 18:08

update all maven based images to unset MAVEN_CONFIG

030c53f

Signed-off-by: svor <vsvydenk@redhat.com>

code cleanup

e870e2e

Signed-off-by: svor <vsvydenk@redhat.com>

svor mentioned this pull request Dec 11, 2019

Unset MAVEN_CONFIG env for all maven based containers redhat-developer/devfile#56

Merged

amisevsk approved these changes Dec 11, 2019

View reviewed changes

svor mentioned this pull request Dec 11, 2019

Remove MAVEN_CONFIG env from maven based containers redhat-developer/devspaces#208

Merged

svor merged commit 2eb32da into eclipse-che:master Dec 13, 2019

amisevsk mentioned this pull request Dec 16, 2019

Can't build the offline-registry image of che-devfile-registry / offline-registry. eclipse-che/che#15483

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unset MAVEN_CONFIG env in Maven based images #160

Unset MAVEN_CONFIG env in Maven based images #160

svor commented Dec 9, 2019

nickboldt Dec 9, 2019 •

edited

Loading

amisevsk Dec 9, 2019

amisevsk Dec 9, 2019

nickboldt left a comment

nickboldt commented Dec 9, 2019

amisevsk left a comment

amisevsk Dec 9, 2019

svor commented Dec 10, 2019

nickboldt commented Dec 10, 2019

amisevsk commented Dec 10, 2019

nickboldt commented Dec 10, 2019

svor commented Dec 11, 2019 •

edited

Loading

svor commented Dec 11, 2019

amisevsk left a comment

amisevsk Dec 11, 2019

amisevsk Dec 11, 2019

svor Dec 11, 2019

amisevsk left a comment

Unset MAVEN_CONFIG env in Maven based images #160

Unset MAVEN_CONFIG env in Maven based images #160

Conversation

svor commented Dec 9, 2019

What does this PR do?

What issues does this PR fix or reference?

nickboldt Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

amisevsk Dec 9, 2019

Choose a reason for hiding this comment

amisevsk Dec 9, 2019

Choose a reason for hiding this comment

nickboldt left a comment

Choose a reason for hiding this comment

nickboldt commented Dec 9, 2019

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk Dec 9, 2019

Choose a reason for hiding this comment

svor commented Dec 10, 2019

nickboldt commented Dec 10, 2019

amisevsk commented Dec 10, 2019

nickboldt commented Dec 10, 2019

svor commented Dec 11, 2019 • edited Loading

svor commented Dec 11, 2019

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk Dec 11, 2019

Choose a reason for hiding this comment

amisevsk Dec 11, 2019

Choose a reason for hiding this comment

svor Dec 11, 2019

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

nickboldt Dec 9, 2019 •

edited

Loading

svor commented Dec 11, 2019 •

edited

Loading