Skip to content
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

Convert Web Terminal Operator to depend on DevWorkspace Operator #80

Merged
merged 13 commits into from
Jun 9, 2021

Conversation

amisevsk
Copy link
Contributor

What does this PR do?

Removes duplication of CRDs/images from DevWorkspace Operator and configures this operator to depend on the DevWorkspace Operator.

What issues does this PR fix or reference?

Part of devfile/devworkspace-operator#407

Is it tested? How?

Simple: apply the catalogsource below which references bundles built from this PR and devfile/devworkspace-operator#438

cat <<EOF | oc apply -f -
apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: custom-devworkspace-demo-catalog
  namespace: openshift-marketplace
spec:
  sourceType: grpc
  image: quay.io/amisevsk/devworkspace-demo-index:dev
  publisher: Angel Misevski
  displayName: DevWorkspace + Web Terminal integration demo
EOF

Manual:

  1. Add catalogsource for DWO
    1. Checkout Add requirements for working with OLM devfile/devworkspace-operator#438 locally
    2. Export DWO_BUNDLE_IMG and DWO_INDEX_IMG as appropriate
    3. Run make generate_olm_bundle_image generate_olm_index_image
    4. Add catalogsource: make register_catalogsource
  2. Add new catalogsource for DWO
    1. Build bundle and index: make build
    2. Register catalogsource: make register_catalogsource
  3. Install operator through OperatorHub UI in OpenShift. Note: there will be two options for "Web Terminal Operator". The correct one has Version 1.3.0

Screencast:

install-wto.mp4

Copy link
Contributor

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
I'm just not sure about initialization-resource. It works fine for Che and CRW, where user is proposed to create CheCluster CR, but I don't this suites well for our use-case.

manifests/web-terminal.clusterserviceversion.yaml Outdated Show resolved Hide resolved
manifests/web-terminal.clusterserviceversion.yaml Outdated Show resolved Hide resolved
amisevsk added 4 commits June 2, 2021 13:53
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
pkg/config/constants.go Show resolved Hide resolved
@@ -1,77 +1,57 @@
SHELL := bash
.SHELLFLAGS = -ec

WTO_IMG ?= quay.io/wto/web-terminal-operator:latest
BUNDLE_IMG ?= quay.io/wto/web-terminal-operator-metadata:next
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One day we should rename it to WTO_BUNDLE_IMG

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I tried to leave the existing variables functionality untouched. Other future improvements:

  • make build builds the bundle and index, make build_controller_image builds the "controller"
  • Building the controller image is relegated to a separate makefile, when it should maybe be the reverse, as we have in DWO

build/bin/entrypoint Outdated Show resolved Hide resolved
build/dockerfiles/deployment.Dockerfile Outdated Show resolved Hide resolved
build/makefiles/deployment.mk Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
web-terminal-sample.yaml Show resolved Hide resolved
@@ -1,2 +1,3 @@
dependencies
.idea
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

manifests/web-terminal.clusterserviceversion.yaml Outdated Show resolved Hide resolved
@@ -357,6 +190,8 @@ spec:
name: David Festal
- email: jpinkney@redhat.com
name: Josh Pinkney
- email: amisevsk@redhat.com
name: Angel Misevski
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the line below... Since we need to request users to delete and install it, maybe we should remove replaces: but it's just dymmy assumption that may not work at all. Maybe there is another way OLM suggests for such case.

metadata/dependencies.yaml Outdated Show resolved Hide resolved
return client.Create(ctx, specDWT)
}

// TODO: Figure out way to sync DWTs for updates to WTO.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just update it if it exists? )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid that because we have no way of knowing if the DWTs are different because they were created by a previous version of WTO or if they're different because a user modified them on purpose :).

Either way, it's not a problem we need to fix until the second WTO release after this change 😄

strategy: deployment
installModes:
- supported: false
type: OwnNamespace
- supported: false
- supported: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to test how it will behave if we disable multinamespace and probably drop it, since it's the just we drop it, since anyway we force users to reinstall WTO from scratch.

Copy link
Contributor

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good enough so, I'm OK with listing TODOs items and merging as is.

amisevsk and others added 9 commits June 8, 2021 22:03
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Add a basic program that ensures DevWorkspaceTemplates used by Web
Terminal are present in the openshift-operators namespace.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
singlenamespace mode

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
@amisevsk
Copy link
Contributor Author

amisevsk commented Jun 9, 2021

Rebased and autosquashed.

@sleshchenko sleshchenko merged commit 8966d50 into redhat-developer:main Jun 9, 2021
@amisevsk amisevsk deleted the dwo-integration branch June 10, 2021 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants