feat(ws): update list workspacekinds JSON payloads #138
Conversation
google-oss-prow
bot
added
do-not-merge/work-in-progress
size/L
and removed
size/M
labels
|
New output: |
thesuperzapper
changed the title
There was a problem hiding this comment.
@yehudit1987 thank you so much for the PR and I've asked a few small changes. Sorry for the delay on this PR. I took some long time of PTO and I'm now back to work.
I'll make that the next iterations of review it will be much more quick.
| if item.Spec.Spawner.Hidden != nil { | ||
| hidden = *item.Spec.Spawner.Hidden | ||
| } | ||
| labels := GetOrDefaultWithRecovery(&item.Spec.PodTemplate.PodMetadata.Labels, make(map[string]string)) |
There was a problem hiding this comment.
@yehudit1987 can you please remove the indirection and avoid the abstraction in this case?
In golang (and in your codebase), we usually prefer to do the null check like:
podMetadataLabels := item.Spec.PodTemplate.PodMetadata.Labels
if podMetadataLabels == nil {
podMetadataLabels = map[string]string{}
}
Because it's more idiomatic.
| if item.Spec.Spawner.DeprecationMessage != nil { | ||
| deprecationMessage = *item.Spec.Spawner.DeprecationMessage | ||
| } | ||
| deprecated := GetOrDefaultWithRecovery(item.Spec.Spawner.Deprecated, false) |
| "testing" | ||
| ) | ||
|
|
||
| func TestGetOrDefaultWithRecovery(t *testing.T) { |
There was a problem hiding this comment.
we will not need this test class (in favor of explicit null check)
| "fmt" | ||
| ) | ||
|
|
||
| // GetOrDefaultWithRecovery safely retrieves the value, returning the default value if a panic occurs or the value is nil. |
There was a problem hiding this comment.
We will not need this code in favor of an explicit null check.
| PodConfig string `json:"pod_config"` | ||
| Resources ResourceModel `json:"resources"` | ||
| } | ||
| func BuildImageConfigValues(item *kubefloworgv1beta1.WorkspaceKind) []ImageConfigValue { |
There was a problem hiding this comment.
Do we need to export this method?
| Deprecated bool `json:"deprecated"` | ||
| DeprecationMessage string `json:"deprecation_message"` | ||
| Hidden bool `json:"hidden"` | ||
| func BuildPodConfigValues(item *kubefloworgv1beta1.WorkspaceKind) []PodConfigValue { |
There was a problem hiding this comment.
Do we need to export this method?
Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>
yehudit1987
force-pushed
the
notebook-v2/#128
branch
from
3275185 to
44eaec0
Compare
|
Thank you @yehudit1987 ! looks good for me. |
|
Sample output. // 20250115091628
// http://localhost:4000/api/v1/workspacekinds/jupyterlab
{
"data": {
"name": "jupyterlab",
"display_name": "JupyterLab Notebook",
"description": "A Workspace which runs JupyterLab in a Pod",
"deprecated": false,
"deprecation_message": "This WorkspaceKind will be removed on 20XX-XX-XX, please use another WorkspaceKind.",
"hidden": false,
"icon": {
"url": "https://jupyter.org/assets/favicons/apple-touch-icon-152x152.png"
},
"logo": {
"url": "https://upload.wikimedia.org/wikipedia/commons/3/38/Jupyter_logo.svg"
},
"pod_template": {
"pod_metadata": {
"labels": {
"my-workspace-kind-label": "my-value"
},
"annotations": {
"my-workspace-kind-annotation": "my-value"
}
},
"volume_mounts": {
"home": "/home/jovyan"
},
"options": {
"image_config": {
"default": "jupyterlab_scipy_190",
"values": [
{
"id": "jupyterlab_scipy_180",
"displayName": "jupyter-scipy:v1.8.0",
"labels": {
"python_version": "3.11"
},
"hidden": true,
"redirect": {
"to": "jupyterlab_scipy_190",
"message": {
"text": "This update will change...",
"level": "Info"
}
}
},
{
"id": "jupyterlab_scipy_190",
"displayName": "jupyter-scipy:v1.9.0",
"labels": {
"python_version": "3.11"
},
"hidden": false
}
]
},
"pod_config": {
"default": "tiny_cpu",
"values": [
{
"id": "tiny_cpu",
"displayName": "Tiny CPU",
"description": "Pod with 0.1 CPU, 128 Mb RAM",
"labels": {
"cpu": "100m",
"memory": "128Mi"
}
},
{
"id": "small_cpu",
"displayName": "Small CPU",
"description": "Pod with 1 CPU, 2 GB RAM",
"labels": {
"cpu": "1000m",
"memory": "2Gi"
}
},
{
"id": "big_gpu",
"displayName": "Big GPU",
"description": "Pod with 4 CPU, 16 GB RAM, and 1 GPU",
"labels": {
"cpu": "4000m",
"gpu": "1",
"memory": "16Gi"
}
}
]
}
}
}
}
} |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ederign 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 |
|
@thesuperzapper this looks good for me and it is ready for your review. |
|
@yehudit1987 can you please fix the lint issue? |
|
/ok-to-test |
Signed-off-by: Yehudit Kerido <yehudit.kerido@nokia.com>
|
@yehudit1987 thanks for this PR, but please hold off for now, as @ederign and I are working on another one which changes the structure a bit. |
Resolution details for #128.
We investigate this issue with referring to payload example provided in #127 .
Seems the example is not relevant with all it's fields to workspacekind resource.
Currently the output for the sample yaml as resource input (jupyterlab_v1beta1_workspacekind.yaml) -
{ "data":[ { "name":"jupyterlab", "spawner":{ "display_name":"JupyterLab Notebook", "description":"A Workspace which runs JupyterLab in a Pod", "deprecated":false, "deprecation_message":"This WorkspaceKind will be removed on 20XX-XX-XX, please use another WorkspaceKind.", "hidden":false }, "pod_template":{ "pod_metadata":{ "labels":{ "my-workspace-kind-label":"my-value" }, "annotations":{ "my-workspace-kind-annotation":"my-value" } }, "image_config":{ "current":"jupyterlab_scipy_190", "desired":"jupyterlab_scipy_190", "redirect_chain":[ { "source":"jupyterlab_scipy_180", "target":"jupyterlab_scipy_190" } ] }, "pod_config":{ "current":"tiny_cpu", "desired":"tiny_cpu", "redirect_chain":[ { "source":"tiny_cpu", "target":"tiny_cpu" }, { "source":"tiny_cpu", "target":"small_cpu" }, { "source":"tiny_cpu", "target":"big_gpu" } ] } } } ] }