feat(ws): add http service paths to WS get on backend #213
Conversation
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
| Ports []ImagePort `json:"ports,omitempty"` | ||
| } | ||
|
|
||
| type ImagePort struct { |
There was a problem hiding this comment.
@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}/There was a problem hiding this comment.
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!
Signed-off-by: Yehudit Kerido <[email protected]>
Signed-off-by: Yehudit Kerido <[email protected]>
yehudit1987
force-pushed
the
add_missing_field
branch
from
db2a7aa to
8648291
Compare
Signed-off-by: Yehudit Kerido <[email protected]>
There was a problem hiding this comment.
@yehudit1987 thank you for the PR. Just two small comments there.
| if len(val.Spec.Ports) == 0 { | ||
| continue | ||
| } | ||
| firstPort := val.Spec.Ports[0] |
There was a problem hiding this comment.
// 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?
There was a problem hiding this comment.
Or we deliberatelly are going to always use the first port here?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@ederign I think it's a mistake (mine). Will fix it soon.
There was a problem hiding this comment.
No worries @yehudit1987 ! Thank you for the PR
| if displayName == "" { | ||
| displayName = val.Id | ||
| } | ||
| basePath := "/workspace" |
There was a problem hiding this comment.
Just out of my curiosity, where this /workspace path comes from?
|
/assign @ederign |
Signed-off-by: Yehudit Kerido <[email protected]>
Signed-off-by: Yehudit Kerido <[email protected]>
thesuperzapper
changed the title
This PR is for adding missing ports field to workspace model.