feat: slim down GET /workspaces payload and remove unused types - #1253
feat: slim down GET /workspaces payload and remove unused types#1253Snehadas2005 wants to merge 4 commits into
Conversation
| Name string `json:"name"` | ||
| } | ||
|
|
||
| const UnknownHomeMountPath = "__UNKNOWN_HOME_MOUNT_PATH__" |
There was a problem hiding this comment.
Can we move this to a const def at the top of this file like other constants.
|
Hi @thesuperzapper! I have updated the file by moving the constant to the top for better clarity. If there are any other adjustments or enhancements needed, please let me know. |
andyatmiami
left a comment
There was a problem hiding this comment.
this is looking really good - some minor cleanup/alignment issues - hopefully straightforward! let me know if you have questions!
and, out of curiosity - while this absolutely is technically a breaking API change - i'm wondering if you actually tried spinning up a Tilt cluster on this codebase to see if anything in the UI actually/noticably breaks?
I know we removed the "collapsible drawer" from the UI prior to this work - so it could very well be that nothing is relying on the functionality we are breaking. I would appreciate if you could comment on the PR if/where we see breaking behavior in the frontend - so I can coordinate with @thaorell if/as necessary to stage some additional changes so we can cleanly merge this PR.
Thanks!
There was a problem hiding this comment.
I think with the introduction of these types in podtemplate/details - we can now actually REMOVE the types we had originally added to common/
- admittedly it was a weird dance - but the right one to ensure at any given time code was optimal!
So I think the following can be cleaned up from common/types.go (can't directly comment on it since that file isn't presently included in this PR):
const UnknownHomeMountPathtype PodVolumeInfo structtype PodSecretInfo struct
PodMetadata SHOULD REMAIN in common/types.go
| package details | ||
|
|
||
| import ( | ||
| ptr "k8s.io/utils/ptr" |
There was a problem hiding this comment.
Every other file in the backend imports this package without an alias since the package name is already ptr. The explicit alias is redundant and inconsistent with codebase convention.
Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com>
Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com>
Co-authored-by: Andy Stoneberg <astonebe@redhat.com> Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com>
…meInfo, and PodSecretInfo Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com>
0cd46bd to
813f996
Compare
|
[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 |
|
/remove-label area/controller |
|
@Snehadas2005: The label(s) `/remove-label area/controller
DetailsIn 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 @andyatmiami! I have updated the PR with the requested cleanups:
Regarding the Regarding Tilt & Frontend Testing: |
|
/hold i'm going to get a frontend PR raised to ensure merging this doesn't break frontend (but note I have reviewed/tested this PR and will be ready to "sign off" once we get update: |
Migrate the workspace details drawer to fetch volumes, labels, and
annotations from the dedicated GET /workspaces/{ns}/{name}/podtemplate/details
endpoint (PR kubeflow#1119) instead of reading them from the list-item payload.
This prepares the frontend for PR kubeflow#1253, which removes pendingRestart,
podTemplate.podMetadata, and podTemplate.volumes from the list response.
New hook — useWorkspaceDetails:
- Follows the established useFetchState pattern (useCallback → useFetchState)
- Fetches DetailsWorkspaceDetails on-demand when a workspace is selected
- Returns [data, loaded, error, refresh] matching the standard tuple
Overview tab — labels from details endpoint:
- WorkspaceDetailsOverview now receives details/detailsLoaded props
- Labels read from details?.podMetadata.labels instead of the list item
- Shows a Spinner while the details request is in-flight
Resources tab — volumes from details endpoint:
- WorkspaceResources now receives details/detailsLoaded props
- Home volume and data volumes read from details?.volumes instead of
the list item, with Spinner placeholders during loading
Activity tab — pendingRestart computed from redirect chains:
- Removes direct use of workspace.pendingRestart (will be dropped by kubeflow#1253)
- Derives the same boolean from redirectChain length on podConfig and
imageConfig, which is semantically equivalent
Start/Stop modals — drop pendingRestart guard:
- workspacePendingUpdate condition simplified from
`workspace?.pendingRestart && (redirectChain checks)` to
`!!workspace && (redirectChain checks)`
- The !!workspace guard provides explicit null narrowing for TypeScript
Mock builders — backward-compatible refactoring:
- buildMockWorkspaceUpdateFromWorkspace accepts optional podMetadata
and volumes args, falling back to list-item fields when not provided,
so existing callers are unaffected
- New buildMockWorkspaceDetails builder for the details endpoint response
- mockNotebookApis details mock derives data from the workspace list
item for consistency with other mock endpoints
Cypress tests — details endpoint intercepts:
- Add interceptApi type overload for the podtemplate/details route
- Add details endpoint intercepts in setupWorkspaceActionTest and the
multi-workspace drawer switching test to prevent unhandled requests
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Andy Stoneberg <astonebe@redhat.com>
This PR slims down the payload returned by the workspace list API endpoint (
GET /workspaces) by removing fields that are now served on-demand by the/podtemplate/detailsendpoint. This reduces payload size and establishes a clean separation between list views and details overlays.Specifically:
pendingRestartfromWorkspaceListItem.podMetadataandvolumesfrom thePodTemplateproperty in the list responses.Warning
This is a breaking API change for the frontend. The frontend must migrate to retrieve details metadata via
GET /podtemplate/detailsand computependingRestartclient-side from the options redirect chains.Key Changes
PendingRestartfield fromWorkspaceListItem(internal/models/workspaces/types.go).PodMetadataandVolumesfields fromPodTemplate(internal/models/workspaces/types.go).PodVolumesstruct.PodVolumeInfoandPodSecretInfotointernal/models/workspaces/podtemplate/details/types.go.BuildHomeVolume(),BuildDataVolumes(), andBuildSecretVolumes()fromcommon/funcs.gotodetails/funcs.goto prevent any reference from list endpoint modules.swagger.jsonanddocs.go) to reflect the new models.workspaces_handler_test.goto remove outdated list payload assertions.Verification
go build -o tmp_build_bin ./cmd/main.go.go test ./...and verified that all 124 API test specifications passed.swagger.jsonanddocs.goschema.closes: #1030
related: #885