-
Notifications
You must be signed in to change notification settings - Fork 74
feat: remove automatic redirect application from Workspace controller #804
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
base: notebooks-v2
Are you sure you want to change the base?
feat: remove automatic redirect application from Workspace controller #804
Conversation
This commit removes the automatic redirect application logic from the Workspace controller and shifts to user-initiated configuration updates. Changes: - Remove auto-update block (lines 218-237) from workspace_controller.go that automatically applied redirects when workspace was paused - Remove DeferUpdates field from WorkspaceSpec CRD - Remove DeferUpdates from backend API models (WorkspaceCreate, Workspace) - Update documentation in sample WorkspaceKind YAML to reflect that redirects are now user-initiated via API - Update all test fixtures to remove DeferUpdates field - Regenerate CRD manifests The controller still computes redirect chains and populates status fields: - status.podTemplateOptions.imageConfig.desired - status.podTemplateOptions.imageConfig.redirectChain - status.podTemplateOptions.podConfig.desired - status.podTemplateOptions.podConfig.redirectChain - status.pendingRestart Users must now explicitly update their Workspace via the PUT API to apply configuration redirects, providing better control and audit trail. Signed-off-by: alokdangre <[email protected]>
Remove all references to deferUpdates from the frontend codebase: - Remove deferUpdates from WorkspaceFormProperties type - Remove deferUpdates checkbox from workspace form UI - Remove deferUpdates from workspace creation payload - Remove deferUpdates from all mock data and test fixtures - Update OpenAPI swagger.json to remove deferUpdates field - Regenerate frontend API types from updated swagger spec - Add missing axe-core dependency for eslint - Remove unused Checkbox import The frontend now correctly reflects that redirect updates must be user-initiated via the API rather than automatically applied. Signed-off-by: alokdangre <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Update swagger.version to point to the commit with updated swagger.json that properly removes deferUpdates field from the API types. Signed-off-by: alokdangre <[email protected]>
Screen.Recording.2025-12-14.202605.mp4 |
|
/ok-to-test |
caponetto
left a comment
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.
Thanks for your contribution @alokdangre!
I reviewed the frontend changes and left a comment inline.
| @@ -1 +1 @@ | |||
| 4f0a29dec0d3c9f0d0f02caab4dc84101bfef8b0 | |||
| 4e1c149 | |||
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.
Could you please use the latest commit hash we have in the repository for the swagger.json file?
It's b81728498cbd567be42482f941a97ac551f11253 (see here)
| 4e1c149 | |
| b81728498cbd567be42482f941a97ac551f11253 |
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.
I see the change I proposed here got reverted in the last commits
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.
b81728498cbd567be42482f941a97ac551f11253 this has deferUpdates in it , so when the ci generates types it fails becuase we don't use deferUpdates, so therefore i changed it to the f94d911fac3ec4f4f231fff926a4c7cd49c67b3c this commit where the swagger has no defeUpdates, so ci generated type check will pass
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.
Yeah, I see!
@andyatmiami that's why we generally have 2 PRs (frontend/backend) when there's update in the API.
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.
sorry, i added both at one because ,by adding backend only changes, the frontend workspace creation was broked thats why i added both at one
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.
@caponetto should i divide and make two pr
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.
No worries @alokdangre! Appreciate the proactivity! I think this is the first time we have faced this scenario.
My only concern about having a single PR is the need to reference the hash commit from one that is not technically present in this repository. I'll let @andyatmiami weigh in on that decision.
Co-authored-by: Guilherme Caponetto <[email protected]> Signed-off-by: Alok Dangre <[email protected]>
- Regenerate swagger docs (docs.go) to remove deferUpdates - Remove deferUpdates from sample workspace YAML - Update test comments to remove deferUpdates references - Update backend README example to remove deferUpdates - Update frontend package-lock.json Signed-off-by: alokdangre <[email protected]>
9450c94 to
896170d
Compare
46aabbc to
b546654
Compare
- Update swagger.version to point to current commit without deferUpdates - Regenerate frontend API types from updated swagger.json - Update package-lock.json Signed-off-by: alokdangre <[email protected]>
This PR removes the automatic application of redirects by the Workspace controller and transitions to a user-initiated update model via the API.
closes: #779
Changes:
DeferUpdatesfield fromWorkspaceSpecin CRD and backend models.workspace_controller.gothat automatically mutatedWorkspace.Specbased on redirects.DeferUpdates.DeferUpdatescheckbox and logic.This ensures that workspace updates (redirects) are only applied when explicitly requested by the user, providing a safer and more predictable update process.