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

feat: [Draft] Allow DevWorkspaceID to be overridden via annotation #747

Merged
merged 2 commits into from
Feb 14, 2022

Conversation

amisevsk
Copy link
Collaborator

What does this PR do?

Allow overriding the default DevWorkspace ID (derived from the DevWorkspace CR's UUID) via the annotation

controller.devfile.io/devworkspace_id_override

with the caveats:

  • DWO attempts to check if the DevWorkspaceID is already in use for another workspace in the current namespace and fails the workspace if it does
  • The annotation is only read on workspace creation and ignored once workspace ID is set to avoid losing data
  • Controller webhooks are updated to explicitly block updates to .status.devworkspaceId

This PR is in draft status for testing if this feature is useful for migrating Che workspaces to the DevWorkspace controller.

Before merging, this PR needs a polishing pass. We should also decide on a max length for overridden workspace IDs, as the controller implicitly assumes the length of the ID is fixed in a few places (where workspace ID is used as part of a k8s object name, for example).

Changes can be tested using the image quay.io/amisevsk/devworkspace-controller:override-dw-id (for both controller and webhooks server)

What issues does this PR fix or reference?

N/A for now.

Is it tested? How?

No changes if annotation is not used; otherwise, workspace ID should be set to value of annotation.

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
    • v8-devworkspace-operator-e2e: DevWorkspace e2e test
    • v8-che-happy-path: Happy path for verification integration with Che

@openshift-ci
Copy link

openshift-ci bot commented Jan 19, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@amisevsk amisevsk requested a review from ibuziuk January 19, 2022 19:23
@amisevsk amisevsk changed the title feat: [Draft] Allow DevWorkspaceID to be overridden via annoation feat: [Draft] Allow DevWorkspaceID to be overridden via annotation Feb 4, 2022
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Copy link
Contributor

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

Verified. Works like a charm \o/

@openshift-ci
Copy link

openshift-ci bot commented Feb 10, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ibuziuk
Copy link
Contributor

ibuziuk commented Feb 10, 2022

We should also decide on a max length for overridden workspace IDs

I would propose 25 chars (current value of the ID, and the same length is used for che-server devfile v1 workspaces)

@ibuziuk
Copy link
Contributor

ibuziuk commented Feb 10, 2022

Related issue for UD - eclipse-che/che#21153

@amisevsk amisevsk marked this pull request as ready for review February 14, 2022 15:30
@amisevsk
Copy link
Collaborator Author

/test v8-devworkspace-operator-e2e

@amisevsk amisevsk merged commit 35bdd72 into devfile:main Feb 14, 2022
@amisevsk amisevsk deleted the configure-workspace-id branch February 14, 2022 18:06
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.

2 participants