fix(ws): update JSON payload for listing workspaces #134

mohamedch7 · 2024-11-26T22:11:50Z

Fix: Update Workspace Model and JSON Payload Structure

This PR addresses issue #127 by refactoring the workspace model and adjusting the JSON payload structure to align with the updated API requirements.

Changes Made

Workspace Model Updates:
- Introduced new fields: WorkspaceKind, PodTemplate, Activity, and Volumes.
- Updated nested models (PodMetadata, ImageConfig, PodConfig) for improved modularity.
Handler Updates:
- Refactored test cases in workspaces_handler_test.go to validate the new model and payload structure.
Repository Adjustments:
- Updated the CreateWorkspace method in workspaces.go to reflect the new model structure.
- Ensured compatibility with nested fields during workspace creation.
Payload Adjustments:
- Ensured responses match the updated API specification.

Impact

Aligns backend structure with updated API expectations.

Testing

Unit tests in workspaces_handler_test.go updated to include new cases.

ederign · 2024-11-27T12:16:26Z

@mohamedch7 thank you so much for the contribution! I'll look it before end of the week.

thesuperzapper · 2024-11-28T05:28:29Z

@mohamedch7 once we merge #137 (hopefully tomorrow) please rebase this PR so you get the new linting (and make sure your PR passes a make lint).

ederign

Hi @mohamedch7 thank you so much for the PR. I left some required changes for your PR.
Could you review all the fields according to:
https://github.com/thesuperzapper/kubeflow-notebooks-v2-design/blob/main/crds/workspace-kind.yaml
and https://github.com/thesuperzapper/kubeflow-notebooks-v2-design/blob/main/crds/workspace.yaml

ederign · 2024-12-02T19:46:33Z

workspaces/backend/internal/models/workspaces.go

+		 Namespace: item.Namespace,
+		 WorkspaceKind: WorkspaceKind{
+			 Name: item.Spec.Kind,
+			 Type: item.Spec.PodTemplate.Options.ImageConfig,


This should be POD_TEMPLATE.
https://github.com/thesuperzapper/kubeflow-notebooks-v2-design/blob/main/crds/workspace-kind.yaml#L51

workspaces/backend/internal/models/workspaces.go

ederign · 2024-12-02T19:47:24Z

workspaces/backend/internal/models/workspaces.go

+		 PodTemplate: PodTemplate{
+			 PodMetadata: &PodMetadata{
+				 Labels:      item.Spec.PodTemplate.PodMetadata.Labels,
+				 Annotations: item.Spec.PodTemplate.PodMetadata.Annotations,


"labels": null,
"annotations": null

ederign · 2024-12-02T19:47:59Z

workspaces/backend/internal/models/workspaces.go

+			 },
+			 Volumes: &Volumes{
+				 Home: &DataVolumeModel{
+					 PvcName:   item.Spec.PodTemplate.Volumes.Data[0].PVCName,


Home is not Volumes.Data[0], but instead defined by kind:
the mount path is defined in the WorkspaceKind under
## spec.podTemplate.volumeMounts.home

Should fetch WorkspaceKind then use it, right ?
57d4485

ederign · 2024-12-02T19:48:32Z

workspaces/backend/internal/models/workspaces.go

+					 MountPath: item.Spec.PodTemplate.Volumes.Data[0].MountPath,
+					 ReadOnly:  *item.Spec.PodTemplate.Volumes.Data[0].ReadOnly,
+				 },
+				 Data: dataVolumes,


Make sure to don't list 'home' here.

ederign · 2024-12-02T19:54:27Z

workspaces/backend/internal/models/workspaces.go

+			 },
+			 PodConfig: &PodConfig{
+				 Current:       item.Spec.PodTemplate.Options.PodConfig,
+				 Desired:       item.Spec.PodTemplate.Options.PodConfig,


It should be:
Pod Template Options:
Image Config:
Desired: jupyterlab_scipy_190

(on status)

ederign · 2024-12-02T19:58:24Z

Also, make sure to rebase it to include the new linting etc.

thesuperzapper · 2024-12-03T16:12:03Z

@mohamedch7 you should be able to rebase it to head of notebooks-v2 now, and then make sure make lint and make test pass.

ederign

@mohamedch7 I'm sorry for the delay on this PR. I was in a long PTO but I'm back.

I just asked here a small changes. I'll review as soon as you send the PR. And please, don't forget to sign the commits.

ederign · 2025-01-13T19:38:07Z

workspaces/backend/api/workspaces_handler_test.go

-				models.NewWorkspaceModelFromWorkspace(workspace1),
-				models.NewWorkspaceModelFromWorkspace(workspace2),
-				models.NewWorkspaceModelFromWorkspace(workspace3),
+				models.NewWorkspaceModelFromWorkspace(ctx, k8sClient, workspace1),


Our model should only be responsible for translate k8s object for our DTOs/pojos. We should not add any extra logic there. Could you please pass the kind as a parameter?

ederign · 2025-01-13T19:38:14Z

workspaces/backend/api/workspaces_handler_test.go

-				models.NewWorkspaceModelFromWorkspace(workspace2),
-				models.NewWorkspaceModelFromWorkspace(workspace3),
+				models.NewWorkspaceModelFromWorkspace(ctx, k8sClient, workspace1),
+				models.NewWorkspaceModelFromWorkspace(ctx, k8sClient, workspace2),


same as above

ederign · 2025-01-13T19:38:20Z

workspaces/backend/api/workspaces_handler_test.go

-				models.NewWorkspaceModelFromWorkspace(workspace3),
+				models.NewWorkspaceModelFromWorkspace(ctx, k8sClient, workspace1),
+				models.NewWorkspaceModelFromWorkspace(ctx, k8sClient, workspace2),
+				models.NewWorkspaceModelFromWorkspace(ctx, k8sClient, workspace3),


same as above

ederign · 2025-01-13T19:38:31Z

workspaces/backend/api/workspaces_handler_test.go

@@ -256,8 +256,8 @@ var _ = Describe("Workspaces Handler", func() {
 			Expect(k8sClient.Get(ctx, workspaceKey2, workspace2)).To(Succeed())

 			expectedWorkspaces := []models.WorkspaceModel{
-				models.NewWorkspaceModelFromWorkspace(workspace1),
-				models.NewWorkspaceModelFromWorkspace(workspace2),
+				models.NewWorkspaceModelFromWorkspace(ctx, k8sClient, workspace1),


same as above.

ederign · 2025-01-13T19:38:37Z

workspaces/backend/api/workspaces_handler_test.go

-				models.NewWorkspaceModelFromWorkspace(workspace1),
-				models.NewWorkspaceModelFromWorkspace(workspace2),
+				models.NewWorkspaceModelFromWorkspace(ctx, k8sClient, workspace1),
+				models.NewWorkspaceModelFromWorkspace(ctx, k8sClient, workspace2),


same as above.

ederign · 2025-01-13T19:40:13Z

workspaces/backend/internal/models/workspaces.go

-	t := time.Unix(item.Status.Activity.LastActivity, 0)
-	formattedLastActivity := t.Format("2006-01-02 15:04:05 MST")
+func NewWorkspaceModelFromWorkspace(ctx context.Context, cl client.Client, item *kubefloworgv1beta1.Workspace) WorkspaceModel {
+	wsk := &kubefloworgv1beta1.WorkspaceKind{}


move this lines to repository.

ederign · 2025-01-13T19:40:36Z

workspaces/backend/internal/repositories/workspaces.go

@@ -69,7 +69,7 @@ func (r *WorkspaceRepository) GetWorkspaces(ctx context.Context, namespace strin

 	workspacesModels := make([]models.WorkspaceModel, len(workspaceList.Items))
 	for i, item := range workspaceList.Items {
-		workspaceModel := models.NewWorkspaceModelFromWorkspace(&item)
+		workspaceModel := models.NewWorkspaceModelFromWorkspace(ctx, r.client, &item)


same as others.

ederign · 2025-01-13T19:40:44Z

workspaces/backend/internal/repositories/workspaces.go

@@ -53,7 +53,7 @@ func (r *WorkspaceRepository) GetWorkspace(ctx context.Context, namespace string
 		return models.WorkspaceModel{}, err
 	}

-	workspaceModel := models.NewWorkspaceModelFromWorkspace(workspace)
+	workspaceModel := models.NewWorkspaceModelFromWorkspace(ctx, r.client, workspace)


same as others.

ederign · 2025-01-13T19:41:00Z

workspaces/backend/internal/repositories/workspaces.go

@@ -100,34 +100,34 @@ func (r *WorkspaceRepository) CreateWorkspace(ctx context.Context, workspaceMode
 			Name:      workspaceModel.Name,
 			Namespace: workspaceModel.Namespace,
 			// TODO: the pod and workspace labels should be separated


Can you please remove my old TODO here?

workspaces/backend/internal/repositories/workspaces.go

yehudit1987 · 2025-01-19T12:15:43Z

Hi @mohamedch7, please, don't forget to sign the commits, you have unsigned commits.

ederign · 2025-01-22T16:05:12Z

@mohamedch7, can you please sign the commit?

Signed-off-by: mohamedch7 <[email protected]>

thesuperzapper

In the interest of moving this forward, I will approve.

There is one big issue I found (see comments) but other than that I expect we will be rewriting a lot of this anyway, so its good to have a base to work from.

thesuperzapper · 2025-01-24T21:08:29Z

workspaces/backend/internal/repositories/workspaces.go

-		workspaceModel := models.NewWorkspaceModelFromWorkspace(&item)
-		workspacesModels[i] = workspaceModel
+		kind := &kubefloworgv1beta1.WorkspaceKind{}
+		if err := r.client.Get(ctx, client.ObjectKey{Name: item.Spec.Kind}, kind); err != nil {


It's not guaranteed that a WorkspaceKind will actually exist, even if it has been referenced by a Workspace (things can happen which bypass the validation webhook).

So in a follow-up PR, we need to gracefully handle Workspaces with missing WorkspaceKinds (e.g. still list them but indicate that we can't fully process them).

thesuperzapper · 2025-01-24T21:09:23Z

workspaces/backend/internal/repositories/workspaces.go

-		workspaceModel := models.NewWorkspaceModelFromWorkspace(&item)
-		workspacesModels[i] = workspaceModel
+		kind := &kubefloworgv1beta1.WorkspaceKind{}
+		if err := r.client.Get(ctx, client.ObjectKey{Name: item.Spec.Kind}, kind); err != nil {


Same idea here, WSK might be missing.

thesuperzapper · 2025-01-24T21:09:26Z

workspaces/backend/internal/repositories/workspaces.go

-	workspaceModel := models.NewWorkspaceModelFromWorkspace(workspace)
-	return workspaceModel, nil
+	kind := &kubefloworgv1beta1.WorkspaceKind{}
+	if err := r.client.Get(ctx, client.ObjectKey{Name: workspace.Spec.Kind}, kind); err != nil {


Same idea here, WSK might be missing.

thesuperzapper · 2025-01-24T21:09:39Z

workspaces/backend/internal/repositories/workspaces.go

@@ -140,7 +144,12 @@ func (r *WorkspaceRepository) CreateWorkspace(ctx context.Context, workspaceMode
 		return models.WorkspaceModel{}, err
 	}

-	return models.NewWorkspaceModelFromWorkspace(workspace), nil
+	kind := &kubefloworgv1beta1.WorkspaceKind{}
+	if err := r.client.Get(ctx, client.ObjectKey{Name: workspace.Spec.Kind}, kind); err != nil {


Same idea here, WSK might be missing.

google-oss-prow · 2025-01-24T21:11:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ederign, thesuperzapper

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:

~~OWNERS~~ [thesuperzapper]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

thesuperzapper · 2025-01-24T21:42:23Z

/lgtm

google-oss-prow bot requested review from kimwnasptd and thesuperzapper November 26, 2024 22:11

google-oss-prow bot added the size/L label Nov 26, 2024

ederign suggested changes Dec 2, 2024

View reviewed changes

mohamedch7 force-pushed the update-workspace-json branch from 4a4c56f to fd1fb92 Compare December 19, 2024 12:44

ederign suggested changes Jan 13, 2025

View reviewed changes

ederign approved these changes Jan 22, 2025

View reviewed changes

mohamedch7 added 9 commits January 23, 2025 17:11

fix(ws): update JSON payload for listing workspaces

3381d6a

Signed-off-by: mohamedch7 <[email protected]>

fix(ws): fix workspaceKind type to POD_TEMPLATE

2a13248

Signed-off-by: mohamedch7 <[email protected]>

fix(ws): ensure podMetadata labels/annotations are not null

4ae5e20

Signed-off-by: mohamedch7 <[email protected]>

fix(ws): fetch and use WorkspaceKind for determining home mount paths

e8fbf72

Signed-off-by: mohamedch7 <[email protected]>

fix(ws): fix Volumes.Home values and tests

1bde18b

Signed-off-by: mohamedch7 <[email protected]>

fix(ws): fix PodConfig desired value

810dbd8

Signed-off-by: mohamedch7 <[email protected]>

chore: remove lint warnings by cleaning up comments and formatting

8713bab

Signed-off-by: mohamedch7 <[email protected]>

fix(ws): refactor workspace model creation to include WorkspaceKind

6d10fa0

Signed-off-by: mohamedch7 <[email protected]>

fix(ws): Move NewWorkspaceModelFromWorkspace from models to repositories

5bc2a17

Signed-off-by: mohamedch7 <[email protected]>

mohamedch7 force-pushed the update-workspace-json branch from fbc34d7 to 5bc2a17 Compare January 23, 2025 16:12

thesuperzapper approved these changes Jan 24, 2025

View reviewed changes

google-oss-prow bot added the approved label Jan 24, 2025

google-oss-prow bot assigned thesuperzapper Jan 24, 2025

google-oss-prow bot added the lgtm label Jan 24, 2025

google-oss-prow bot merged commit d06762d into kubeflow:notebooks-v2 Jan 24, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ws): update JSON payload for listing workspaces #134

fix(ws): update JSON payload for listing workspaces #134

mohamedch7 commented Nov 26, 2024

ederign commented Nov 27, 2024

thesuperzapper commented Nov 28, 2024

ederign left a comment

ederign Dec 2, 2024

mohamedch7 Dec 18, 2024

ederign Dec 2, 2024

ederign Dec 2, 2024

mohamedch7 Dec 18, 2024 •

edited

Loading

ederign Dec 2, 2024

ederign Dec 2, 2024

ederign Dec 2, 2024

mohamedch7 Dec 19, 2024

ederign commented Dec 2, 2024

thesuperzapper commented Dec 3, 2024

ederign left a comment

ederign Jan 13, 2025

mohamedch7 Jan 15, 2025

ederign Jan 13, 2025

ederign Jan 13, 2025

ederign Jan 13, 2025

ederign Jan 13, 2025

ederign Jan 13, 2025

ederign Jan 13, 2025

ederign Jan 13, 2025

ederign Jan 13, 2025

yehudit1987 commented Jan 19, 2025 •

edited

Loading

ederign commented Jan 22, 2025

thesuperzapper left a comment

thesuperzapper Jan 24, 2025

thesuperzapper Jan 24, 2025

thesuperzapper Jan 24, 2025

thesuperzapper Jan 24, 2025

google-oss-prow bot commented Jan 24, 2025

thesuperzapper commented Jan 24, 2025

fix(ws): update JSON payload for listing workspaces #134

fix(ws): update JSON payload for listing workspaces #134

Conversation

mohamedch7 commented Nov 26, 2024

Fix: Update Workspace Model and JSON Payload Structure

Changes Made

Impact

Testing

ederign commented Nov 27, 2024

thesuperzapper commented Nov 28, 2024

ederign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedch7 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ederign commented Dec 2, 2024

thesuperzapper commented Dec 3, 2024

ederign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yehudit1987 commented Jan 19, 2025 • edited Loading

ederign commented Jan 22, 2025

thesuperzapper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-prow bot commented Jan 24, 2025

thesuperzapper commented Jan 24, 2025

mohamedch7 Dec 18, 2024 •

edited

Loading

yehudit1987 commented Jan 19, 2025 •

edited

Loading