che #14809 Enable CodeReady branding on the ConsoleLink elements created by the che operator with 'codeready' flavor #101

ibuziuk · 2019-10-22T17:59:20Z

ConsoleLink functionality works just fine

Verified against crc:

./run server:start -a operator -p crc -b console-openshift-console.apps-crc.testing -n crw-link --che-operator-image=quay.io/ibuziuk/che-operator:che-14809-new --che-operator-cr-yaml=crw.yaml

crw.yaml:

apiVersion: org.eclipse.che/v1
kind: CheCluster 
metadata:
  name: eclipse-che
spec:
  server:
    # server image used in Che deployment
    cheImage: 'quay.io/crw/server-rhel8'
    # tag of an image used in Che deployment
    cheImageTag: '2.0-377'
    # image:tag used in Devfile registry deployment
    devfileRegistryImage: 'quay.io/crw/devfileregistry-rhel8:2.0-159'
    # image:tag used in plugin registry deployment
    pluginRegistryImage: 'quay.io/crw/pluginregistry-rhel8:2.0-203'
    # defaults to `che`. When set to `codeready`, CodeReady Workspaces is deployed
    # the difference is in images, labels, exec commands
    cheFlavor: 'codeready'
    # when set to true the operator will attempt to get a secret in OpenShift router namespace
    # to add it to Java trust store of Che server. Requires cluster-admin privileges for operator service account
    selfSignedCert: true
    # TLS mode for Che. Make sure you either have public cert, or set selfSignedCert to true
    tlsSupport: true
    # protocol+hostname of a proxy server. Automatically added as JAVA_OPTS and https(s)_proxy
    # to Che server and workspaces containers
    # proxyURL: ''
    # port of a proxy server
    # proxyPort: ''
    # username for a proxy server
    # proxyUser: ''
    # password for a proxy user
    # proxyPassword: ''
    # a list of non-proxy hosts. Use | as delimiter, eg localhost|my.host.com|123.42.12.32
    # nonProxyHosts: ''
    # overrides for https://github.com/eclipse/che/blob/master/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties
    customCheProperties:
      CHE_WORKSPACE_SIDECAR_IMAGE__PULL__POLICY: 'Always'
      CHE_DOCKER_ALWAYS__PULL__IMAGE: 'true'
      # set by operator, probably don't need to override. Latest in https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L47-L51
      # CHE_INFRA_KUBERNETES_PVC_JOBS_IMAGE: 'registry.access.redhat.com/ubi8-minimal:8.0-213'
      CHE_WORKSPACE_PLUGIN__BROKER_INIT_IMAGE: 'quay.io/crw/pluginbrokerinit-rhel8:2.0-6'
      CHE_WORKSPACE_PLUGIN__BROKER_UNIFIED_IMAGE: 'quay.io/crw/pluginbroker-rhel8:2.0-5'
      CHE_SERVER_SECURE__EXPOSER_JWTPROXY_IMAGE: 'quay.io/crw/jwtproxy-rhel8:2.0-4'
      CHE_FACTORY_DEFAULT__EDITOR: 'eclipse/che-theia/7.3.0'
      CHE_FACTORY_DEFAULT__PLUGINS: 'eclipse/che-machine-exec-plugin/7.3.0'
      CHE_WORKSPACE_DEVFILE_DEFAULT__EDITOR: 'eclipse/che-theia/7.3.0'
      CHE_WORKSPACE_DEVFILE_DEFAULT__EDITOR_PLUGINS: 'eclipse/che-machine-exec-plugin/7.3.0'
    # when set to true, the operator skips deploying Postgres, and passes connection details of existing DB to Che server
    # otherwise a Postgres deployment is created
    # externalDb: false
    # Postgres deployment in format image:tag. Defaults to registry.redhat.io/rhscl/postgresql-96-rhel7 (see pkg/deploy/defaults.go for latest tag)
    # set by operator, probably don't need to override. Latest in https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L47-L51
    # postgresImage: 'registry.redhat.io/rhscl/postgresql-96-rhel7:1-47'
  # storage:
    # persistent volume claim strategy for Che server. Can be common (all workspaces PVCs in one volume),
    # per-workspace (one PVC per workspace for all declared volumes) and unique (one PVC per declared volume). Defaults to common
    # pvcStrategy: 'per-workspace'
    # size of a persistent volume claim for workspaces. Defaults to 1Gi
    # pvcClaimSize: '1Gi'
    # instruct Che server to launch a special pod to precreate a subpath in a PV
    # preCreateSubPaths: true
    # image:tag for preCreateSubPaths jobs
    # set by operator, probably don't need to override. Latest in https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L47-L51
    # pvcJobsImage: 'registry.access.redhat.com/ubi8-minimal:8.0-213'
  # auth:
    # instructs operator on whether or not to deploy Keycloak/RH SSO instance. When set to true provision connection details
    # externalIdentityProvider: false
    # retrieved from respective route/ingress unless explicitly specified in CR (when ExternalKeycloak is true)
    # instructs an Operator to enable OpenShift v3 identity provider in Keycloak,
    # as well as create respective oAuthClient and configure Che configMap accordingly
    # openShiftoAuth: false
    # image:tag used in Keycloak deployment
    # set by operator, probably don't need to override. Latest in https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L47-L51
    # identityProviderImage: 'registry.redhat.io/redhat-sso-7/sso73-openshift:1.0-15'

nickboldt · 2019-10-22T18:10:36Z

pkg/deploy/defaults.go

+	defaultConsoleLinkCheName              = "che"
+	defaultConsoleLinkCodeReadyName        = "codeready"
+	defaultConsoleLinkCheDisplayName       = "Eclipse Che"
+	defaultConsoleLinkCodeReadyDisplayName = "CodeReady Workspaces"


-1, for a few reasons...

a) "CodeReady" is a brand, not a product, so should NOT be used here. Options are "Red Hat CodeReady Workspaces", "CodeReady Workspaces", "Workspaces" or if an internal variable not exposed to the end user in the UI/UX, then CRW is ok. But...

b) the existing convention used in https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L40-L41 to differentiate upstream/downstream versions of variables is with "Upstream" prefix on the variable name, so we could keep using that, eg., defaultConsoleLinkUpstreamName and defaultConsoleLinkName

where does the ConsoleLinkName appear? if it's UI, it shouldn't be "codeready" but rather "workspaces" or "Workspaces" as that's the product's shortname. "CodeReady" includes Studio, Builder, Containers, and more, so we can't just slap that in Workspaces and OCP and assume people will think we mean the Che-based product.

cc @slemeur

@slemeur Could you provide some guidance here (related to point a: wording of the Display Name) ?

Related to comment b, it's right that the most general case for variables is to have the raw property name for CRW, and property name + "upstream" for Eclipse Che. So to increase consistency we could use this naming scheme here as well.

As for the consoleLinkName, I'm don't think it is displayed to the user, and changing it might break the OS 4.2 integration if they would be searching for the ConsoleLink by name.

In fact looking into the OpenShift 4.2 console, it is clear that we should not change the ConsoleLinkName:

https://github.com/openshift/console/blob/f5a2a8b0358398748f301af0c11bd74ac33badce/frontend/packages/dev-console/src/components/topology/topology-utils.ts#L27

nickboldt

overall looks good but David's the go expert in this repo. :) and I don't like the variable names, so -1 but he can overrule me.

nickboldt · 2019-10-22T18:12:11Z

pkg/deploy/defaults.go

-	DefaultConsoleLinkDisplayName = "Eclipse Che"
-	DefaultConsoleLinkSection     = "Red Hat Applications"
+	DefaultConsoleLinkSection              = "Red Hat Applications"
+	DefaultConsoleLinkImage                = "/dashboard/assets/branding/loader.svg"


is this the Che image? or the CRW image? I'm guessing if we've built the dashboard correctly in CRW it'll use the CRW one, but just wanted to ask.

@nickboldt variables names already contain CodeReady, so I just follow the standard that is in place e.g. defaultCodeReadyServerImageRepo https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L24 or defaultCodeReadyServerImageTag - https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L24

and I do not plan to do the refactoring. The rule of thumb is either to follow the convention that is already in place (even if you do not like) or proceed with the major refactoring

is this the Che image? or the CRW image?

I suppose it should use CRW image but I can not confirm since I can not verify

@nickboldt doesn't the CRW assembly replace the image with a CRW one during packaging ? And in this case is the relative path of the image still the same ?
If it's the case we wouldn't need to change the loader image.

ibuziuk · 2019-10-22T18:21:21Z

and I don't like the variable names

variables names already contain CodeReady, so I just follow the standard that is in place e.g. defaultCodeReadyServerImageRepo https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L24 or defaultCodeReadyServerImageTag - https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L24

and I do not plan to do the refactoring. The rule of thumb is either to follow the convention that is already in place (even if you do not like) or proceed with the major refactoring

nickboldt

Despite the fact that https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L23-L26 uses "CodeReady" as if it's a product, and https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L36-L52 uses the "Upstream" convention to differentiate Che from CRW config, ... +1.

You're right, refactoring can be dealt with later: eclipse-che/che#14957

nickboldt · 2019-10-22T20:12:06Z

Please remove the "WIP" marker before you merge this.

davidfestal · 2019-10-22T22:27:55Z

I plan to review tomorrow morning.

ibuziuk · 2019-10-23T08:18:21Z

@nickboldt well, I plan to verify it first before merging ;-)

davidfestal · 2019-10-23T21:01:01Z

pkg/deploy/defaults.go

-	DefaultConsoleLinkDisplayName = "Eclipse Che"
-	DefaultConsoleLinkSection     = "Red Hat Applications"
+	DefaultConsoleLinkSection              = "Red Hat Applications"
+	DefaultConsoleLinkImage                = "/dashboard/assets/branding/loader.svg"


@nickboldt doesn't the CRW assembly replace the image with a CRW one during packaging ? And in this case is the relative path of the image still the same ?
If it's the case we wouldn't need to change the loader image.

davidfestal · 2019-10-23T21:10:22Z

pkg/deploy/defaults.go

+	defaultConsoleLinkCheName              = "che"
+	defaultConsoleLinkCodeReadyName        = "codeready"
+	defaultConsoleLinkCheDisplayName       = "Eclipse Che"
+	defaultConsoleLinkCodeReadyDisplayName = "CodeReady Workspaces"


Related to comment b, it's right that the most general case for variables is to have the raw property name for CRW, and property name + "upstream" for Eclipse Che. So to increase consistency we could use this naming scheme here as well.

As for the consoleLinkName, I'm don't think it is displayed to the user, and changing it might break the OS 4.2 integration if they would be searching for the ConsoleLink by name.

In fact looking into the OpenShift 4.2 console, it is clear that we should not change the ConsoleLinkName:

https://github.com/openshift/console/blob/f5a2a8b0358398748f301af0c11bd74ac33badce/frontend/packages/dev-console/src/components/topology/topology-utils.ts#L27

davidfestal · 2019-10-23T21:17:04Z

and I don't like the variable names

variables names already contain CodeReady, so I just follow the standard that is in place e.g. defaultCodeReadyServerImageRepo https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L24 or defaultCodeReadyServerImageTag - https://github.com/eclipse/che-operator/blob/master/pkg/deploy/defaults.go#L24

and I do not plan to do the refactoring. The rule of thumb is either to follow the convention that is already in place (even if you do not like) or proceed with the major refactoring

In fact, afaik, the use of CodeReady in the default property names is more the exception (only for the che server component). At least as far as I'm concerned, I used only the raw vs xxxUpstreamxxx convention to add new defaults since I took ownership of the operator code.

davidfestal · 2019-10-23T21:21:18Z

PR is WIP since I have problems verifying it. Basically, deployment on OCP 4.2 with tls / self-signed certs is failing with:

Using embedded assembly.
Found a custom cert. Adding it to java trust store based on /usr/lib/jvm/java-1.8.0/jre/lib/security/cacerts
chmod: changing permissions of '/home/jboss/cacerts': Operation not permitted

I tried to deploy via:
chectl server:start -a operator -p openshift -b rhopp-qa.devcluster.openshift.com -n my-crw-branding --che-operator-image=quay.io/ibuziuk/che-operator:che-14809 --che-operator-cr-yaml=crw.yaml

@ibuziuk Did you try to deploy it simply through the OperatorHub (from the UI), and then change the docker image to your docker image in the deployed CSV, and operator deployment ?

Not sure CheCtl is the recommended install mode for Openhsift 4.2 that has OperatorHub inside it.

…ted by the che operator with 'codeready' flavor Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk · 2019-10-24T11:52:26Z

@nickboldt @davidfestal PR has been updated and verified against crc.

ConsoleLink functionality works just fine, but the CodeReady image was not applied:

@nickboldt will the logo issue be addressed as part of the https://issues.jboss.org/browse/CRW-315?focusedCommentId=13803743&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13803743
?

slemeur · 2019-10-24T12:12:30Z

The display name should be "CodeReady Workspaces".
Agree with @ibuziuk we need to apply the proper icon as well.

nickboldt · 2019-10-24T15:13:35Z

@nickboldt doesn't the CRW assembly replace the image with a CRW one during packaging ? And in this case is the relative path of the image still the same ?
If it's the case we wouldn't need to change the loader image.

I believe so, yes. @evidolob is investigating the branding problem now, hopefully fixed tomorrow. See eclipse-che/che#14982

I have no idea which icon is used for the devconsole dropdown but if it's one from the CRW assembly or the crw-theia assembly then we should be OK once branding is fixed.

ibuziuk · 2019-10-24T16:09:24Z

I have no idea which icon is used for the devconsole dropdown

The icon is /dashboard/assets/branding/loader.svg

@rhopp @davidfestal may I get your reviews ;-)

nickboldt · 2019-10-24T16:46:11Z

Ah, well for CRW 2 we're using https://github.com/redhat-developer/codeready-workspaces/blob/master/assembly/codeready-workspaces-assembly-dashboard-war/src/main/webapp/assets/branding/CodeReady_icon_loader.svg not /assets/branding/loader.svg as per https://github.com/redhat-developer/codeready-workspaces/blob/master/assembly/codeready-workspaces-assembly-dashboard-war/src/main/webapp/assets/branding/product.json.template#L7 but if we need to rename the file we can do so. :)

ibuziuk · 2019-10-24T16:48:06Z

@nickboldt would be great if you could do the image renaming ;-)
should I create an issue for that?

nickboldt · 2019-10-24T16:59:12Z

No need. Here's a PR: https://github.com/redhat-developer/codeready-workspaces/pull/189/files

See also eclipse-che/che#14982 and https://issues.jboss.org/browse/CRW-315?focusedCommentId=13804077&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13804077

ibuziuk · 2019-10-24T17:00:24Z

@nickboldt awesome, lemme know against which image I can re-test

davidfestal

Thanks Ilya.

nickboldt · 2019-10-24T20:34:49Z

Should be able to check new image names in 2.0-377 or later. Building now in https://codeready-workspaces-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/view/CRW_CI/view/Pipelines/job/crw_master/882/ then will trigger brew build. Should be there in an hour or so, unless something goes wacky.

https://quay.io/repository/crw/server-rhel8?tag=2.0&tab=tags

ibuziuk · 2019-10-25T10:41:31Z

@nickboldt tested against 2.0-377 and now the image is blank :/

sleshchenko · 2019-10-25T10:43:55Z

@ibuziuk If you use self-signed cert - it may be caused by an untrusted host. You can easily figure out it by discovering Developer Tools of browser.

ibuziuk · 2019-10-25T10:46:54Z

ah, that was it - works now

ibuziuk requested review from davidfestal, l0rd, nickboldt, rhopp and vparfonov as code owners October 22, 2019 17:59

l0rd mentioned this pull request Oct 22, 2019

Release Che v7.3.1 eclipse-che/che#14947

Closed

30 tasks

nickboldt reviewed Oct 22, 2019

View reviewed changes

nickboldt requested changes Oct 22, 2019

View reviewed changes

nickboldt approved these changes Oct 22, 2019

View reviewed changes

ibuziuk mentioned this pull request Oct 23, 2019

chectl: fail to start Che against crc (E_PLATFORM_NOT_READY) eclipse-che/che#14959

Closed

3 tasks

davidfestal requested changes Oct 23, 2019

View reviewed changes

che #14809 Enable CodeReady branding on the ConsoleLink elements crea…

da21c9e

…ted by the che operator with 'codeready' flavor Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk force-pushed the che-14809 branch from a20bee3 to da21c9e Compare October 24, 2019 11:06

ibuziuk changed the title ~~[WIP] che #14809 Enable CodeReady branding on the ConsoleLink elements created by the che operator with 'codeready' flavor~~ che #14809 Enable CodeReady branding on the ConsoleLink elements created by the che operator with 'codeready' flavor Oct 24, 2019

davidfestal approved these changes Oct 24, 2019

View reviewed changes

ibuziuk merged commit 4b0b485 into eclipse-che:master Oct 25, 2019

ibuziuk mentioned this pull request Oct 25, 2019

Enable branding on the ConsoleLink elements created by the che operator eclipse-che/che#14809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

che #14809 Enable CodeReady branding on the ConsoleLink elements created by the che operator with 'codeready' flavor #101

che #14809 Enable CodeReady branding on the ConsoleLink elements created by the che operator with 'codeready' flavor #101

ibuziuk commented Oct 22, 2019 •

edited

Loading

nickboldt Oct 22, 2019

davidfestal Oct 23, 2019

davidfestal Oct 23, 2019 •

edited

Loading

davidfestal Oct 23, 2019

nickboldt left a comment

nickboldt Oct 22, 2019

ibuziuk Oct 22, 2019 •

edited

Loading

ibuziuk Oct 22, 2019

davidfestal Oct 23, 2019

ibuziuk commented Oct 22, 2019

nickboldt left a comment

nickboldt commented Oct 22, 2019

davidfestal commented Oct 22, 2019

ibuziuk commented Oct 23, 2019

davidfestal Oct 23, 2019

davidfestal Oct 23, 2019

davidfestal commented Oct 23, 2019

davidfestal commented Oct 23, 2019

ibuziuk commented Oct 24, 2019 •

edited

Loading

slemeur commented Oct 24, 2019

nickboldt commented Oct 24, 2019

ibuziuk commented Oct 24, 2019

nickboldt commented Oct 24, 2019

ibuziuk commented Oct 24, 2019

nickboldt commented Oct 24, 2019

ibuziuk commented Oct 24, 2019

davidfestal left a comment

nickboldt commented Oct 24, 2019 •

edited

Loading

ibuziuk commented Oct 25, 2019

sleshchenko commented Oct 25, 2019

ibuziuk commented Oct 25, 2019

che #14809 Enable CodeReady branding on the ConsoleLink elements created by the che operator with 'codeready' flavor #101

che #14809 Enable CodeReady branding on the ConsoleLink elements created by the che operator with 'codeready' flavor #101

Conversation

ibuziuk commented Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfestal Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickboldt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuziuk Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuziuk commented Oct 22, 2019

nickboldt left a comment

Choose a reason for hiding this comment

nickboldt commented Oct 22, 2019

davidfestal commented Oct 22, 2019

ibuziuk commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfestal commented Oct 23, 2019

davidfestal commented Oct 23, 2019

ibuziuk commented Oct 24, 2019 • edited Loading

slemeur commented Oct 24, 2019

nickboldt commented Oct 24, 2019

ibuziuk commented Oct 24, 2019

nickboldt commented Oct 24, 2019

ibuziuk commented Oct 24, 2019

nickboldt commented Oct 24, 2019

ibuziuk commented Oct 24, 2019

davidfestal left a comment

Choose a reason for hiding this comment

nickboldt commented Oct 24, 2019 • edited Loading

ibuziuk commented Oct 25, 2019

sleshchenko commented Oct 25, 2019

ibuziuk commented Oct 25, 2019

ibuziuk commented Oct 22, 2019 •

edited

Loading

davidfestal Oct 23, 2019 •

edited

Loading

ibuziuk Oct 22, 2019 •

edited

Loading

ibuziuk commented Oct 24, 2019 •

edited

Loading

nickboldt commented Oct 24, 2019 •

edited

Loading