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
Bug 1844938: Migrate to DevWorkspace CR for the terminal #5687
Bug 1844938: Migrate to DevWorkspace CR for the terminal #5687
Conversation
@amisevsk: This pull request references Bugzilla bug 1844938, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi @amisevsk. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@christianvogt: This pull request references Bugzilla bug 1844938, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
kind: 'DevWorkspace', | ||
label: 'Devworkspace', | ||
labelPlural: 'workspaces', | ||
apiGroup: 'apiextensions.k8s.io', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems strange: from apiextensions.k8s.io
to devfile.io
. Is it a CR or a CRD ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Good point -- I'll look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The frontend needs to create/edit DevWorkspace instances, so devfile.io
is the correct option here. You may be thinking of the earlier functionality that checked if the CRD was defined on the cluster for enabling/disabling the feature -- this has been replaced by a back-end endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK
frontend/packages/console-app/src/components/cloud-shell/cloud-shell-utils.ts
Outdated
Show resolved
Hide resolved
/ok-to-test |
/retest |
@amisevsk: This pull request references Bugzilla bug 1844938, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1 similar comment
@amisevsk: This pull request references Bugzilla bug 1844938, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
export const CLOUD_SHELL_CREATOR_LABEL = 'org.eclipse.che.workspace/creator'; | ||
export const CLOUD_SHELL_IMMUTABLE_ANNOTATION = 'org.eclipse.che.workspace/immutable'; | ||
export const CLOUD_SHELL_CREATOR_LABEL = 'controller.devfile.io/creator'; | ||
export const CLOUD_SHELL_IMMUTABLE_ANNOTATION = 'controller.devfile.io/restricted-access'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should it be better to name it CLOUD_SHELL_RESTRICTED_ANNOTATION ?
There are 2 other files to be changed in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I'm renaming the constants, should I name them WEB_TERMINAL_*
as well, given the naming change? (cloud-shell -> web-terminal
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amisevsk i'd keep it as cloud shell right now for consistency in the code as the entire feature is using cloud shell.
We don't surface cloud shell
to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to CLOUD_SHELL_RESTRICTED_ANNOTATION
5c1b240
to
8681df6
Compare
front end changes lgtm I tested the PR on cluster bot today and the DevWorkspace ran as expected. |
ae9bf77
to
9425cda
Compare
From testing with @christianvogt, using a |
Update the custom resource used for providing the OpenShift Web terminal to the DevWorkspace API to be more compatible going forward. Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
9425cda
to
50dbb66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
We should follow up about the propagation policy
apiVersion: 'v1alpha1', | ||
abbr: 'DW', | ||
namespaced: true, | ||
crd: true, | ||
plural: 'devworkspaces', | ||
propagationPolicy: 'Foreground', | ||
propagationPolicy: 'Background', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should determine what isn't getting garbage collected. This doesn't necessarily fix the underlying problem since there's still some resource that isn't deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to be related to kubernetes/kubernetes#90512
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue eclipse-che/che#17146 with more details, but I still have no idea what resource isn't being cleaned up here -- all owned objects are removed but e.g. the deployment is stuck with a foregroundDeletion
finalizer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured this one out -- the workspace controller webhooks were blocking the removal of the foregroundDeletion
finalizer. This is fixed by devfile/devworkspace-operator#107.
Is there a preference between foreground and background deletion here? The default for cli tools is background
.
/cherry-pick release-4.5.z |
@christianvogt: once the present PR merges, I will cherry-pick it on top of release-4.5.z in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM =)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, sleshchenko, spadgett 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:
Approvers can indicate their approval by writing |
@amisevsk: All pull requests linked via external trackers have merged: openshift/console#5687. Bugzilla bug 1844938 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@christianvogt: cannot checkout release-4.5.z: error checking out release-4.5.z: exit status 1. output: error: pathspec 'release-4.5.z' did not match any file(s) known to git. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.5 |
@rohitkrai03: new pull request created: #5730 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Update the custom resource used for providing the OpenShift Web terminal to the DevWorkspace API to be more compatible going forward. This PR moves the GVK and spec used for terminals from
to
as is reflected in operator PR devfile/devworkspace-operator#92
There are additional compatibility changes included as well (e.g. required webhooks are renamed)
How to test
DevWorkspace
( devfile 2.0 ) API devfile/devworkspace-operator#92 (using built image)I've built the changes in this PR as image
docker.io/amisevsk/console:devworkspace-api
to simplify testing.