Skip to content

Conversation

@alokdangre
Copy link

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:

  • Removed DeferUpdates field from WorkspaceSpec in CRD and backend models.
  • Removed logic in workspace_controller.go that automatically mutated Workspace.Spec based on redirects.
  • Updated backend API validation and models to exclude DeferUpdates.
  • Updated frontend to remove DeferUpdates checkbox and logic.
  • Regenerated CRD manifests and deepcopy code.
  • Regenerated frontend API types.
  • Updated tests in controller, backend, and frontend.

This ensures that workspace updates (redirects) are only applied when explicitly requested by the user, providing a safer and more predictable update process.

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]>
@github-project-automation github-project-automation bot moved this to Needs Triage in Kubeflow Notebooks Dec 14, 2025
@google-oss-prow google-oss-prow bot added the area/backend area - related to backend components label Dec 14, 2025
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andyatmiami, ederign for approval. For more information see the Kubernetes Code Review Process.

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

Details 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

@google-oss-prow google-oss-prow bot added area/controller area - related to controller components area/frontend area - related to frontend components area/v2 area - version - kubeflow notebooks v2 size/L labels Dec 14, 2025
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]>
@alokdangre
Copy link
Author

Screen.Recording.2025-12-14.202605.mp4

@paulovmr
Copy link

/ok-to-test

Copy link

@caponetto caponetto left a 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

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)

Suggested change
4e1c149
b81728498cbd567be42482f941a97ac551f11253

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

Copy link
Author

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

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.

Copy link
Author

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

Copy link
Author

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

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]>
@google-oss-prow google-oss-prow bot added size/XXL and removed size/L labels Dec 16, 2025
- 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]>
@alokdangre alokdangre force-pushed the feat/remove-workspace-auto-redirect branch 2 times, most recently from 9450c94 to 896170d Compare December 16, 2025 15:58
@google-oss-prow google-oss-prow bot added size/L and removed size/XXL labels Dec 16, 2025
@alokdangre alokdangre force-pushed the feat/remove-workspace-auto-redirect branch 3 times, most recently from 46aabbc to b546654 Compare December 16, 2025 16:38
- 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/backend area - related to backend components area/controller area - related to controller components area/frontend area - related to frontend components area/v2 area - version - kubeflow notebooks v2 ok-to-test size/L

Projects

Status: Needs Triage

Development

Successfully merging this pull request may close these issues.

3 participants