feat(ws): add http service paths to WS get on backend #213

yehudit1987 · 2025-02-16T07:46:02Z

This PR is for adding missing ports field to workspace model.

thesuperzapper · 2025-02-20T16:48:35Z

/ok-to-test

thesuperzapper · 2025-02-20T17:49:58Z

workspaces/backend/internal/models/workspaces/types.go

+	Ports         []ImagePort    `json:"ports,omitempty"`
+}
+
+type ImagePort struct {


@yehudit1987 it might be more useful for the frontend if we do something more abstract like "services" (which initially only includes "HTTP" type services).

That makes it a bit easier to populate the front-end "connect" box, because rather than constructing the HTTP path in the front end we could just include the httpPath (relative to the UI base, not absolute) rather than constructing it in the front end.

Also, stuff like "port number" don't need to be included because this is hidden from the end user.

For example (written in YAML for clarity, but payload is JSON in app):

... services: - httpService: displayName: "XXXXX" httpPath: "/workspace/{namespace}/{workspace_name}/{port_id}/

Hey @thesuperzapper, thanks for the review! I totally agree with your suggestion—it makes more sense. I initially followed the frontend mock, but once the PR is approved, I'll update the frontend accordingly. Also, I’ve moved the location in the struct; hope that makes sense to you as well!

ederign

@yehudit1987 thank you for the PR. Just two small comments there.

ederign · 2025-02-27T12:32:31Z

workspaces/backend/internal/models/workspaces/funcs.go

+		if len(val.Spec.Ports) == 0 {
+			continue
+		}
+		firstPort := val.Spec.Ports[0]


// ports that the container listens on // - if multiple ports are defined, the user will see multiple "Connect" buttons // in a dropdown menu on the Workspace overview page // +kubebuilder:validation:MinItems:=1 // +listType:="map" // +listMapKey:="id" Ports []ImagePort `json:"ports"`

Shall we support here multiple ports here?

Or we deliberatelly are going to always use the first port here?

@thesuperzapper maybe you can chime in on this one. ^

I'm good to support just one for now and do a FUP PR on it.

@ederign I think it's a mistake (mine). Will fix it soon.

No worries @yehudit1987 ! Thank you for the PR

ederign · 2025-02-27T12:34:51Z

workspaces/backend/internal/models/workspaces/funcs.go

+		if displayName == "" {
+			displayName = val.Id
+		}
+		basePath := "/workspace"


Just out of my curiosity, where this /workspace path comes from?

ederign · 2025-02-27T13:02:53Z

/assign @ederign

Signed-off-by: Yehudit Kerido <[email protected]>

Signed-off-by: Mathew Wicks <[email protected]>

thesuperzapper · 2025-03-07T02:10:15Z

@ederign @yehudit1987 I have made significant changes in 4e91e38

I am happy to merge it now so we can unblock #188 (which will need to be rebased to remove the backend updates).

thesuperzapper · 2025-03-07T02:16:48Z

/lgtm
/approve

google-oss-prow · 2025-03-07T02:16:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

* Add missing ports field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add missing ports field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add services field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add services field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add services field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * mathew updates: 1 Signed-off-by: Mathew Wicks <[email protected]> --------- Signed-off-by: Yehudit Kerido <[email protected]> Signed-off-by: Mathew Wicks <[email protected]> Co-authored-by: Yehudit Kerido <[email protected]> Co-authored-by: Mathew Wicks <[email protected]>

* Add missing ports field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add missing ports field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add services field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add services field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add services field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * mathew updates: 1 Signed-off-by: Mathew Wicks <[email protected]> --------- Signed-off-by: Yehudit Kerido <[email protected]> Signed-off-by: Mathew Wicks <[email protected]> Co-authored-by: Yehudit Kerido <[email protected]> Co-authored-by: Mathew Wicks <[email protected]> Signed-off-by: CI Bot <[email protected]>

* Add missing ports field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add missing ports field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add services field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add services field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * Add services field to workspace model Signed-off-by: Yehudit Kerido <[email protected]> * mathew updates: 1 Signed-off-by: Mathew Wicks <[email protected]> --------- Signed-off-by: Yehudit Kerido <[email protected]> Signed-off-by: Mathew Wicks <[email protected]> Co-authored-by: Yehudit Kerido <[email protected]> Co-authored-by: Mathew Wicks <[email protected]>

google-oss-prow bot requested review from kimwnasptd and thesuperzapper February 16, 2025 07:46

google-oss-prow bot added the size/M label Feb 16, 2025

google-oss-prow bot added the ok-to-test label Feb 20, 2025

thesuperzapper reviewed Feb 20, 2025

View reviewed changes

yehudit1987 force-pushed the add_missing_field branch from db2a7aa to 8648291 Compare February 24, 2025 08:26

yehudit1987 marked this pull request as draft February 24, 2025 08:26

google-oss-prow bot added the do-not-merge/work-in-progress label Feb 24, 2025

yehudit1987 marked this pull request as ready for review February 24, 2025 13:33

google-oss-prow bot removed the do-not-merge/work-in-progress label Feb 24, 2025

google-oss-prow bot requested a review from thesuperzapper February 24, 2025 13:33

ederign suggested changes Feb 27, 2025

View reviewed changes

google-oss-prow bot assigned ederign Feb 27, 2025

thesuperzapper changed the title ~~feat(ws): Notebooks 2.0 // Backend// Add missing field~~ feat(ws): add http service paths to WS get on backend Feb 27, 2025

Yehudit Kerido and others added 6 commits March 6, 2025 14:31

Add missing ports field to workspace model

a17464b

Signed-off-by: Yehudit Kerido <[email protected]>

Add missing ports field to workspace model

ddd6ebb

Signed-off-by: Yehudit Kerido <[email protected]>

Add services field to workspace model

82b435d

Signed-off-by: Yehudit Kerido <[email protected]>

Add services field to workspace model

9ff2483

Signed-off-by: Yehudit Kerido <[email protected]>

Add services field to workspace model

63af631

Signed-off-by: Yehudit Kerido <[email protected]>

mathew updates: 1

4e91e38

Signed-off-by: Mathew Wicks <[email protected]>

thesuperzapper force-pushed the add_missing_field branch from 3f32965 to 4e91e38 Compare March 7, 2025 02:06

google-oss-prow bot assigned thesuperzapper Mar 7, 2025

google-oss-prow bot added the lgtm label Mar 7, 2025

google-oss-prow bot added the approved label Mar 7, 2025

google-oss-prow bot merged commit f216c3e into kubeflow:notebooks-v2 Mar 7, 2025
10 of 11 checks passed

github-project-automation bot moved this from Needs Triage to Done in Kubeflow Notebooks Mar 7, 2025

thesuperzapper mentioned this pull request Mar 7, 2025

test(ws): Add workspaces tests #188

Merged

This was referenced Jun 24, 2025

Membership request for Yehudit Kerido kubeflow/internal-acls#795

Open

Add @yehudit1987 as a member kubeflow/internal-acls#796

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(ws): add http service paths to WS get on backend #213

feat(ws): add http service paths to WS get on backend #213

Uh oh!

yehudit1987 commented Feb 16, 2025

Uh oh!

thesuperzapper commented Feb 20, 2025

Uh oh!

thesuperzapper Feb 20, 2025 •

edited

Loading

Uh oh!

yehudit1987 Feb 24, 2025

Uh oh!

ederign left a comment

Uh oh!

ederign Feb 27, 2025

Uh oh!

ederign Feb 27, 2025

Uh oh!

ederign Feb 27, 2025

Uh oh!

yehudit1987 Feb 27, 2025

Uh oh!

ederign Feb 27, 2025

Uh oh!

ederign Feb 27, 2025

Uh oh!

ederign commented Feb 27, 2025

Uh oh!

thesuperzapper commented Mar 7, 2025

Uh oh!

thesuperzapper commented Mar 7, 2025

Uh oh!

google-oss-prow bot commented Mar 7, 2025

Uh oh!

Uh oh!

Uh oh!

feat(ws): add http service paths to WS get on backend #213

feat(ws): add http service paths to WS get on backend #213

Uh oh!

Conversation

yehudit1987 commented Feb 16, 2025

Uh oh!

thesuperzapper commented Feb 20, 2025

Uh oh!

thesuperzapper Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yehudit1987 Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

ederign left a comment

Choose a reason for hiding this comment

Uh oh!

ederign Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

ederign Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

ederign Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

yehudit1987 Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

ederign Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

ederign Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

ederign commented Feb 27, 2025

Uh oh!

thesuperzapper commented Mar 7, 2025

Uh oh!

thesuperzapper commented Mar 7, 2025

Uh oh!

google-oss-prow bot commented Mar 7, 2025

Uh oh!

Uh oh!

Uh oh!

thesuperzapper Feb 20, 2025 •

edited

Loading