Add rhche minishift addon #620

l0rd · 2018-04-04T01:57:56Z

What does this PR do?

Change the default way to deploy rhche on minishift: using an addon. Deploying rhche will be just a matter of:

    minishift addons install openshift/minishift-addons/rhche-prerequisites
    minishift addons apply rhche-prerequisites
    minishift addons install openshift/minishift-addons/rhche
    minishift addons apply rhche --addon-env KEYCLOAK_DOCKER_IMAGE=eivantsov/keycloak

What issues does this PR fix or reference?

#489

How have you tested this PR?

I have run this on minishift 1.15. Note that fabric8 auth, api and f8proxy are not part of the deployment and therefore rhche fails to start (cannot retrieve Che token). That should be fixed introducing some stubs of these services in a second PR

garagatyi

Looks good, but I was not able to try it out. Addon points to a non-existing template file for Postgres. BTW looks like there are some copy-paste errors, please, take a look at inlined comments.

garagatyi · 2018-04-04T08:28:34Z

openshift/minishift-addon/rhche/rhche.addon

@@ -0,0 +1,38 @@
+# Name: rhche
+# Description: Setup and Configure Red Hat Che Template
+# Url: https://www.eclipse.org/che/docs/setup/openshift/index.html


Is this link supposed to point to rhche repo?

Fixed thanks

garagatyi · 2018-04-04T08:29:22Z

openshift/minishift-addon/rhche/rhche.addon

+# Description: Setup and Configure Red Hat Che Template
+# Url: https://www.eclipse.org/che/docs/setup/openshift/index.html
+# NAMESPACE, CHE_DOCKER_IMAGE, OPENSHIFT_TOKEN, GITHUB_CLIENT_ID, GITHUB_CLIENT_SECRET
+# NAMESPACE=mini-che, CHE_DOCKER_IMAGE=eclipse/che-server:latest, OPENSHIFT_TOKEN=NULL, GITHUB_CLIENT_ID=changeme, GITHUB_CLIENT_SECRET=changeme


Why do we use upstream values here?

I have removed the 2 lines above. These were just comments that I forgot to remove.

garagatyi · 2018-04-04T08:30:37Z

openshift/minishift-addon/rhche/rhche.addon

+# NAMESPACE, CHE_DOCKER_IMAGE, OPENSHIFT_TOKEN, GITHUB_CLIENT_ID, GITHUB_CLIENT_SECRET
+# NAMESPACE=mini-che, CHE_DOCKER_IMAGE=eclipse/che-server:latest, OPENSHIFT_TOKEN=NULL, GITHUB_CLIENT_ID=changeme, GITHUB_CLIENT_SECRET=changeme
+# Required-Vars: NAMESPACE, CHE_DOCKER_IMAGE, CHE_VERSION, KEYCLOAK_DOCKER_IMAGE
+# Var-Defaults: NAMESPACE=rhche, CHE_DOCKER_IMAGE=registry.devshift.net/che/rh-che-server, CHE_VERSION=latest, KEYCLOAK_DOCKER_IMAGE=eclipse/che-keycloak


Is this image repo/tag what is produced by build script?

that is what is produced by the official CI

garagatyi · 2018-04-04T08:31:42Z

openshift/minishift-addon/rhche/rhche.addon

+echo [CHE] Deploying Postgres
+oc new-app -f templates/multi/postgres-template.yaml -n #{NAMESPACE}
+
+echo [CHE] Waiting 30s for Keycloak to be up and running


Postgres, not Keycloak

Fixed thanks

garagatyi · 2018-04-04T08:35:49Z

openshift/minishift-addon/rhche/rhche.addon

+oc new-app -f templates/multi/keycloak-template.yaml -p IMAGE_KEYCLOAK=#{KEYCLOAK_DOCKER_IMAGE} -p ROUTING_SUFFIX=#{routing-suffix} -n #{NAMESPACE}
+
+echo [CHE] Waiting 60s for Keycloak to be up and running
+sleep 60


From my experience with keycloak on OCP in upstream Che waiting some amount of time for keycloak is not very reliable. It fails from time to time because keycloak not yet fully started. But since we don't have an ability to run scripts here I see only one option is to merge all the apps into a single template and let services find each other. It is a kubernetes/openshift deployment way in any case I suppose.

I have taken in consideration your feedback and I have splitted the addon in 2 separate addons (I have not put all in one yaml because you may want to use an existing keycloak/postgre).

garagatyi · 2018-04-04T08:36:39Z

openshift/minishift-addon/rhche/rhche.addon.remove

@@ -0,0 +1,17 @@
+# Name: rhche
+# Description: Remove add on
+# Url: https://www.eclipse.org/che/docs/setup/openshift/index.html


Rh-che repo?

yes fixed thanks

garagatyi · 2018-04-04T08:55:42Z

openshift/minishift-addon/rhche/templates/rh-che.app.yaml

+            value: ${CHE_FABRIC8_AUTH_ENDPOINT}
+          - name: CHE_FABRIC8_MULTICLUSTER_OSO_PROXY_URL
+            value: ${CHE_FABRIC8_MULTICLUSTER_OSO_PROXY_URL}
+          - name: CHE_DOCKER_ENABLE__CONTAINER__STOP__DETECTOR


Are you sure that this docker var is needed in rhche?

I agree with you. It doesn't make much sense actually but we are using it in the "official" yaml https://github.com/redhat-developer/rh-che/blob/rh-che6/openshift/rh-che.app.yaml#L171.

It is unused in che6. So I would suggest to remove it from your PR and from the current deployment configuration eclipse-che/che#9344

That is not really related to this PR but more to Uncle Bob boyscout rule :-). I have fixed that on both yaml files.

garagatyi · 2018-04-04T08:58:25Z

openshift/minishift-addon/rhche/templates/rh-che.secret.yaml

+    group: io.fabric8.online.packages
+  name: rhche
+data:
+  che.jdbc.url: "amRiYzpwb3N0Z3Jlc3FsOi8vcG9zdGdyZXM6NTQzMi9kYmNoZQ=="


Why not set jbbc values to NULL?

If I set che.jdbc.url: "NULL" here I am afraid that Che will use that value as the URL and won't be able to connect.

It shouldn't. Here is my PR that was intended to simplify our deployment configs eclipse-che/che#9047

Nice! I have updated the PR

garagatyi · 2018-04-04T09:15:24Z

openshift/minishift-addon/rhche/rhche.addon

+oc adm policy add-role-to-user admin developer -n #{NAMESPACE}
+
+echo [CHE] Deploying Postgres
+oc new-app -f templates/multi/postgres-template.yaml -n #{NAMESPACE}


There is no such a file in the branch

Thanks. I have added the files

garagatyi · 2018-04-04T09:19:29Z

I understand that I would not be able to start it successfully because there are no mocked services but still looks like some parts are still missing.

l0rd · 2018-04-05T11:18:09Z

@garagatyi I have updated the PR. Note that I have splitted the addon in 2 separate addons and the keycloak image to use now is eivantsov/keycloak so to test the PR:

minishift addons install openshift/minishift-addons/rhche-prerequisites
minishift addons apply rhche-prerequisites
minishift addons install openshift/minishift-addons/rhche
minishift addons apply rhche --addon-env KEYCLOAK_DOCKER_IMAGE=eivantsov/keycloak

davidfestal · 2018-04-05T16:09:56Z

openshift/minishift-addons/rhche/templates/rh-che.app.yaml

@@ -202,11 +202,6 @@ objects:
            value: ${CHE_FABRIC8_AUTH_ENDPOINT}
          - name: CHE_FABRIC8_MULTICLUSTER_OSO_PROXY_URL
            value: ${CHE_FABRIC8_MULTICLUSTER_OSO_PROXY_URL}
-          - name: CHE_DOCKER_ENABLE__CONTAINER__STOP__DETECTOR


Are you sure that setting CHE_DOCKER_ENABLE__CONTAINER__STOP__DETECTOR to false isn't necessary anymore ? At some stage it was necessary to avoid some errors.

It is not used in upstream Che at all

ScrewTSW · 2018-04-10T11:14:26Z

[test]

ScrewTSW · 2018-04-10T12:20:33Z

[test]

ScrewTSW · 2018-04-10T13:24:29Z

[test]

garagatyi

Looks good. Eager to see the state of minishift addon when we would be able to develop everything locally!

garagatyi · 2018-04-10T13:32:51Z

openshift/minishift-addons/rhche-prerequisites/rhche-prerequisites.addon

+# Url: https://github.com/redhat-developer/rh-che
+# Required-Vars: NAMESPACE, KEYCLOAK_DOCKER_IMAGE
+# Var-Defaults: NAMESPACE=rhche, KEYCLOAK_DOCKER_IMAGE=eclipse/che-keycloak
+# OpenShift-Version: >=3.5.0


@l0rd I had issues with running Che deployments on oc 3.6 some time ago. Have you tried it with oc 3.5?

garagatyi · 2018-04-10T13:36:16Z

openshift/minishift-addons/rhche/templates/todo.md

@@ -0,0 +1,5 @@
+# Todo
+
+- Split the addon in 2 addons: rhche-prereq and rhche


This line looks not actual anymore

garagatyi · 2018-04-10T13:36:42Z

openshift/rh-che.app.yaml

@@ -168,11 +168,6 @@ objects:
              configMapKeyRef:
                key: che-fabric8-multicluster-oso-proxy-url
                name: rhche
-          - name: CHE_DOCKER_ENABLE__CONTAINER__STOP__DETECTOR


Thanks for cleaning this up

ScrewTSW · 2018-04-11T06:24:59Z

There is something that is unnerving to me.
The jenkins job fails with an unexpected token in jenkins-env.
Why would the env variables contain something different than all the other pull requests?
Could this possibly, after it's merged, cause all the sub-sequential builds to fail?

ScrewTSW · 2018-04-11T06:25:39Z

@l0rd ^

l0rd · 2018-04-11T08:35:43Z

@ScrewTSW maybe it's just because I need to rebase this PR on top of your new modifications? Let me try

l0rd · 2018-04-11T08:54:41Z

@ScrewTSW are you 100% sure that the script works on other PRs after your last changes?

The problem looks to be here: this command creates a script (export_env_variables) that contains a ( (parenthesis).

ScrewTSW · 2018-04-11T09:31:07Z

Yes, because I've run [test] on them after the fix

ScrewTSW · 2018-04-11T09:31:35Z

I have no idea how could a ( get into env variables

rhopp · 2018-04-11T09:39:00Z

I've run simple test of the line you suggested:

$ cat jenkins-env 
KEYCLOAK_TOKEN=testToken
BUILD_NUBMER=buildNubmer
JOB_NAME=jobName
RH_CHE_USERNAME=username
RH_CHE_PASS=pass

$ grep -E "(KEYCLOAK|BUILD_NUMBER|JOB_NAME|RH_CHE)" ./jenkins-env | sed 's/^/export /g' | sed 's/= /=/g' > ./export_env_variables

$ cat export_env_variables 
export KEYCLOAK_TOKEN=testToken
export JOB_NAME=jobName
export RH_CHE_USERNAME=username
export RH_CHE_PASS=pass

It seems, that the line works exactly as expected.

What I would suggest is not to block this PR on this (I cannot think a way how this particular PR could break the script) and (in another PR) edit the script to write the variable names(not values - could contain secret info), which are grepped from the jenkins-env. This way we will have knowledge what variables we are exporting and where to look for potencial issues when this happens next time.
WDYT?

ScrewTSW · 2018-04-11T09:42:42Z

Will try to have a look and add the debug info into the script

ScrewTSW · 2018-04-12T07:12:28Z

@l0rd I think you can merge for now.

ibuziuk · 2018-04-12T07:19:05Z

[test]

ibuziuk · 2018-04-12T07:40:47Z

@ScrewTSW is it still not clear why the build is failing with:
export_env_variables: line 8: syntax error near unexpected token (`
?

ScrewTSW · 2018-04-19T07:35:36Z

@ibuziuk I really apologize for the delay. No, I wasn't able to find any reason.
I guess we can merge and see what happens.

ScrewTSW · 2018-04-19T07:35:41Z

[test]

ScrewTSW · 2018-04-19T11:58:16Z

@l0rd @ibuziuk this is so strange :D I've rebased #645 onto mario's branch and there are no issues with the export. it's only this PR that is failing with the error...

Let's call it black magic and merge this PR.

l0rd requested review from davidfestal, ibuziuk and garagatyi April 4, 2018 01:58

benoitf added status/code-review target/branch kind/enhancement labels Apr 4, 2018

garagatyi suggested changes Apr 4, 2018

View reviewed changes

l0rd force-pushed the rhche-minishift-addon-part1 branch from e49876a to fd5ac50 Compare April 5, 2018 08:55

davidfestal approved these changes Apr 5, 2018

View reviewed changes

davidfestal reviewed Apr 5, 2018

View reviewed changes

garagatyi approved these changes Apr 10, 2018

View reviewed changes

l0rd added 3 commits April 11, 2018 10:49

Add rhche minishift addon

d57a78a

Updates after Alex review

93aaa9b

Changes after Alex second feedback round

856325b

l0rd force-pushed the rhche-minishift-addon-part1 branch from 37d0044 to 856325b Compare April 11, 2018 08:50

ibuziuk approved these changes Apr 12, 2018

View reviewed changes

l0rd mentioned this pull request Apr 17, 2018

Use templates only to deploy Che to OpenShift eclipse-che/che#9190

Merged

ScrewTSW mentioned this pull request Apr 19, 2018

WIP:Adding sed on env to grab variable names #645

Closed

l0rd added 3 commits April 23, 2018 18:03

Changes to match new Che upstream templates

f715edd

Tibor changes to .ci/cico_build_and_deploy_custom_rh-che.sh

dd70f0c

Remove todo.md

96f0fa2

l0rd merged commit 0cdf78f into redhat-developer:rh-che6 Apr 23, 2018

l0rd deleted the rhche-minishift-addon-part1 branch April 23, 2018 22:59

benoitf removed the status/code-review label Apr 23, 2018

		@@ -0,0 +1,5 @@
		# Todo

		- Split the addon in 2 addons: rhche-prereq and rhche

Add rhche minishift addon #620

Add rhche minishift addon #620

Conversation

l0rd commented Apr 4, 2018 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

How have you tested this PR?

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi commented Apr 4, 2018

l0rd commented Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScrewTSW commented Apr 10, 2018

ScrewTSW commented Apr 10, 2018

ScrewTSW commented Apr 10, 2018

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScrewTSW commented Apr 11, 2018 • edited Loading

ScrewTSW commented Apr 11, 2018

l0rd commented Apr 11, 2018

l0rd commented Apr 11, 2018

ScrewTSW commented Apr 11, 2018

ScrewTSW commented Apr 11, 2018 • edited Loading

rhopp commented Apr 11, 2018

ScrewTSW commented Apr 11, 2018

ScrewTSW commented Apr 12, 2018

ibuziuk commented Apr 12, 2018

ibuziuk commented Apr 12, 2018 • edited Loading

ScrewTSW commented Apr 19, 2018

ScrewTSW commented Apr 19, 2018

ScrewTSW commented Apr 19, 2018 • edited Loading

l0rd commented Apr 4, 2018 •

edited

Loading

l0rd commented Apr 5, 2018 •

edited

Loading

ScrewTSW commented Apr 11, 2018 •

edited

Loading

ScrewTSW commented Apr 11, 2018 •

edited

Loading

ibuziuk commented Apr 12, 2018 •

edited

Loading

ScrewTSW commented Apr 19, 2018 •

edited

Loading