Skip to content

Conversation

sleshchenko
Copy link
Member

What does this PR do?

It's back-porting for #329
So, the main purpose of this PR is introduce the latest web terminal plugin.

In addition it enables building v1alphax container image, which previously we built manually after merge.

What issues does this PR fix or reference?

back-ports #329

Is it tested? How?

Not yet. but I'm going to test that samples/web-terminal.yaml works as expected.

repository: devfile/devworkspace-controller
dockerfile: ./build/Dockerfile
tags: next
tags: v1alphax
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this branch represents the web terminal operator should we just publish to https://quay.io/repository/wto/web-terminal-operator instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

You reminded me the stuff I planed but forgot - default IMG value in Makefile.
Technically we can, but how good it will be if we'll use WTO image as default value for DWO v1alphax.

I like more the way we have now, when main branch of WTO depends on DWO:alpha https://github.com/redhat-developer/web-terminal-operator/blob/main/manifests/web-terminal.clusterserviceversion.yaml#L38
While quay.io/repository/wto/web-terminal-operator is supposed to host stuff built on brew.

Copy link
Contributor

Choose a reason for hiding this comment

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

My whole idea with https://issues.redhat.com/projects/WTO/issues/WTO-78 was to make it so that we no longer had to do anything manually after we finish a release (including pushing images built from brew). If we continue with using the images from brew we will always have manual steps after a release. Though, when we create a release I can probably just have a github action on web-terminal-operator that tags the current version of quay.io/devfile/devworkspace-controller:v1alphax and then push that to quay.io/wto/devworkspace-controller so it shouldn't be an issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, for me WTO stuff must be built in brew, otherwise it's not WTO.
It's like if we build Che x branch and push it as CRW.

So, I'm proceeding with the current flow, which does not change anything and automates image build, and separately we can think more about how we can avoid manual post release activities

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do it that way though the quay.io/wto bundle will only ever refer to the images on the redhat registry and never the ones in quay.io/wto. The only way to refer to the quay.io/wto images would to build another bundle in a github action, but if we do that then it's also not built in brew and also not technically wto

Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM so long as @JPinkney's concerns are addressed.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, sleshchenko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [amisevsk,sleshchenko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sleshchenko sleshchenko merged commit a4368a7 into devfile:v1.0.0-alphax Apr 14, 2021
@sleshchenko sleshchenko deleted the latestTerminal branch April 14, 2021 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants