-
Notifications
You must be signed in to change notification settings - Fork 95
feat: listvalues endpoint WIP #875
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: notebooks-v2
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,8 @@ type WorkspaceKindListEnvelope Envelope[[]models.WorkspaceKind] | |
|
|
||
| type WorkspaceKindEnvelope Envelope[models.WorkspaceKind] | ||
|
|
||
| type ListValuesEnvelope Envelope[models.ListValuesResponse] | ||
|
|
||
| // GetWorkspaceKindHandler retrieves a specific workspace kind by name. | ||
| // | ||
| // @Summary Get workspace kind | ||
|
|
@@ -237,3 +239,70 @@ func (a *App) CreateWorkspaceKindHandler(w http.ResponseWriter, r *http.Request, | |
| responseEnvelope := &WorkspaceKindCreateEnvelope{Data: createdWorkspaceKind} | ||
| a.createdResponse(w, r, responseEnvelope, location) | ||
| } | ||
|
|
||
| // ListValuesHandler returns filtered imageConfig and podConfig options for a WorkspaceKind. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on other PR review comments I have made - I think we need to update annotations to reflect other There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in #972 |
||
| // | ||
| // @Summary List values for workspace kind options | ||
| // @Description Returns filtered imageConfig and podConfig options based on the provided context. This endpoint is used by the workspace creation wizard to show compatible options. | ||
| // @Tags workspacekinds | ||
| // @ID listValues | ||
| // @Accept json | ||
| // @Produce json | ||
| // @Param name path string true "Name of the workspace kind" extensions(x-example=jupyterlab) | ||
| // @Param body body models.ListValuesRequest true "Request body with optional context filters" | ||
| // @Success 200 {object} ListValuesEnvelope "Successful operation. Returns filtered options with rule_effects." | ||
| // @Failure 400 {object} ErrorEnvelope "Bad Request. Invalid workspace kind name or request body." | ||
| // @Failure 401 {object} ErrorEnvelope "Unauthorized. Authentication is required." | ||
| // @Failure 403 {object} ErrorEnvelope "Forbidden. User does not have permission to access the workspace kind." | ||
| // @Failure 404 {object} ErrorEnvelope "Not Found. Workspace kind does not exist." | ||
| // @Failure 500 {object} ErrorEnvelope "Internal server error. An unexpected error occurred on the server." | ||
| // @Router /workspacekinds/{name}/podtemplate/options/listvalues [post] | ||
| func (a *App) ListValuesHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { | ||
| name := ps.ByName(ResourceNamePathParam) | ||
|
|
||
| // validate path parameters | ||
| var valErrs field.ErrorList | ||
| valErrs = append(valErrs, helper.ValidateWorkspaceKindName(field.NewPath(ResourceNamePathParam), name)...) | ||
| if len(valErrs) > 0 { | ||
| a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) | ||
| return | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to be consistent with the other API endpoints in After path param validation - given we are parsing a request body - we need need to validate content-type here: you can see this pattern in effect (for instance) here: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in #972 |
||
| // parse request body | ||
| var requestBody models.ListValuesRequest | ||
| if err := a.DecodeJSON(r, &requestBody); err != nil { | ||
| a.badRequestResponse(w, r, err) | ||
| return | ||
| } | ||
|
Comment on lines
+271
to
+276
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After decoding the request payload - we want to make sure we are validating the contents. Existing codebase strives for a clear/repeatable pattern in how this is accomplished:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would follow/adhere to existing patterns when calling For instance, in Lets structure similarly in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in #972 (WIP for comments below) |
||
|
|
||
| // =========================== AUTH =========================== | ||
| authPolicies := []*auth.ResourcePolicy{ | ||
| auth.NewResourcePolicy( | ||
| auth.ResourceVerbGet, | ||
| &kubefloworgv1beta1.WorkspaceKind{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: name}, | ||
| }, | ||
| ), | ||
| } | ||
| if success := a.requireAuth(w, r, authPolicies); !success { | ||
| return | ||
| } | ||
| // ============================================================ | ||
|
|
||
| // get the workspace kind | ||
| workspaceKind, err := a.repositories.WorkspaceKind.GetWorkspaceKind(r.Context(), name) | ||
| if err != nil { | ||
| if errors.Is(err, repository.ErrWorkspaceKindNotFound) { | ||
| a.notFoundResponse(w, r) | ||
| return | ||
| } | ||
| a.serverErrorResponse(w, r, err) | ||
| return | ||
| } | ||
|
|
||
| // build the response with rule_effects and context filtering | ||
| response := models.BuildListValuesResponse(workspaceKind, requestBody.Data.Context) | ||
|
|
||
| responseEnvelope := &ListValuesEnvelope{Data: response} | ||
| a.dataResponse(w, r, responseEnvelope) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -175,3 +175,75 @@ func buildOptionRedirect(redirect *kubefloworgv1beta1.OptionRedirect) *OptionRed | |||||||
| Message: message, | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // BuildListValuesResponse transforms a WorkspaceKind into a ListValuesResponse | ||||||||
| // by adding rule_effects to each option value and applying context filters | ||||||||
| func BuildListValuesResponse(wsk WorkspaceKind, context *ListValuesContext) ListValuesResponse { | ||||||||
| imageValues := buildImageConfigValuesWithRules(wsk.PodTemplate.Options.ImageConfig, context) | ||||||||
| podValues := buildPodConfigValuesWithRules(wsk.PodTemplate.Options.PodConfig, context) | ||||||||
|
|
||||||||
| return ListValuesResponse{ | ||||||||
| ImageConfig: ImageConfigWithRules{ | ||||||||
| Default: wsk.PodTemplate.Options.ImageConfig.Default, | ||||||||
| Values: imageValues, | ||||||||
| }, | ||||||||
| PodConfig: PodConfigWithRules{ | ||||||||
| Default: wsk.PodTemplate.Options.PodConfig.Default, | ||||||||
| Values: podValues, | ||||||||
| }, | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| func buildImageConfigValuesWithRules(imageConfig ImageConfig, context *ListValuesContext) []ImageConfigValueWithRules { | ||||||||
| values := []ImageConfigValueWithRules{} | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in #972 |
||||||||
|
|
||||||||
| for _, v := range imageConfig.Values { | ||||||||
| // Filter by context if imageConfig.id is specified | ||||||||
| if context != nil && context.ImageConfig != nil { | ||||||||
| if v.Id != context.ImageConfig.Id { | ||||||||
| continue // skip this value | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // Transform and add rule_effects | ||||||||
| values = append(values, ImageConfigValueWithRules{ | ||||||||
| Id: v.Id, | ||||||||
| DisplayName: v.DisplayName, | ||||||||
| Description: v.Description, | ||||||||
| Labels: v.Labels, | ||||||||
| Hidden: v.Hidden, | ||||||||
| Redirect: v.Redirect, | ||||||||
| ClusterMetrics: v.ClusterMetrics, | ||||||||
| RuleEffects: RuleEffects{UiHide: false}, // Always false for stub | ||||||||
| }) | ||||||||
| } | ||||||||
|
|
||||||||
| return values | ||||||||
| } | ||||||||
|
|
||||||||
| func buildPodConfigValuesWithRules(podConfig PodConfig, context *ListValuesContext) []PodConfigValueWithRules { | ||||||||
| values := []PodConfigValueWithRules{} | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in #972 |
||||||||
|
|
||||||||
| for _, v := range podConfig.Values { | ||||||||
| // Filter by context if podConfig.id is specified | ||||||||
| if context != nil && context.PodConfig != nil { | ||||||||
| if v.Id != context.PodConfig.Id { | ||||||||
| continue // skip this value | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // Transform and add rule_effects | ||||||||
| values = append(values, PodConfigValueWithRules{ | ||||||||
| Id: v.Id, | ||||||||
| DisplayName: v.DisplayName, | ||||||||
| Description: v.Description, | ||||||||
| Labels: v.Labels, | ||||||||
| Hidden: v.Hidden, | ||||||||
| Redirect: v.Redirect, | ||||||||
| ClusterMetrics: v.ClusterMetrics, | ||||||||
| RuleEffects: RuleEffects{UiHide: false}, // Always false for stub | ||||||||
| }) | ||||||||
| } | ||||||||
|
|
||||||||
| return values | ||||||||
| } | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This it TOTALLY MY FAULT and I humbly apologize for the oversight, but we use The following
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in #972
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given existing pattern we are starting to form (with Use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in #972 |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -109,3 +109,70 @@ const ( | |||||||||||||||||||||||||||||||||||||||
| RedirectMessageLevelWarning RedirectMessageLevel = "Warning" | ||||||||||||||||||||||||||||||||||||||||
| RedirectMessageLevelDanger RedirectMessageLevel = "Danger" | ||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type ListValuesRequest struct { | ||||||||||||||||||||||||||||||||||||||||
| Data ListValuesRequestData `json:"data"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type ListValuesRequestData struct { | ||||||||||||||||||||||||||||||||||||||||
| Context *ListValuesContext `json:"context,omitempty"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+112
to
+120
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Please note that I changed the remaining model here from In api/workspacekinds_handler.go: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in #972 |
||||||||||||||||||||||||||||||||||||||||
| type ListValuesContext struct { | ||||||||||||||||||||||||||||||||||||||||
| Namespace *ContextNamespace `json:"namespace,omitempty"` | ||||||||||||||||||||||||||||||||||||||||
| PodConfig *ContextPodConfig `json:"podConfig,omitempty"` | ||||||||||||||||||||||||||||||||||||||||
| ImageConfig *ContextImageConfig `json:"imageConfig,omitempty"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type ContextNamespace struct { | ||||||||||||||||||||||||||||||||||||||||
| Name string `json:"name"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type ContextPodConfig struct { | ||||||||||||||||||||||||||||||||||||||||
| Id string `json:"id"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type ContextImageConfig struct { | ||||||||||||||||||||||||||||||||||||||||
| Id string `json:"id"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type ListValuesResponse struct { | ||||||||||||||||||||||||||||||||||||||||
| ImageConfig ImageConfigWithRules `json:"imageConfig"` | ||||||||||||||||||||||||||||||||||||||||
| PodConfig PodConfigWithRules `json:"podConfig"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type ImageConfigWithRules struct { | ||||||||||||||||||||||||||||||||||||||||
| Default string `json:"default"` | ||||||||||||||||||||||||||||||||||||||||
| Values []ImageConfigValueWithRules `json:"values"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type ImageConfigValueWithRules struct { | ||||||||||||||||||||||||||||||||||||||||
| Id string `json:"id"` | ||||||||||||||||||||||||||||||||||||||||
| DisplayName string `json:"displayName"` | ||||||||||||||||||||||||||||||||||||||||
| Description string `json:"description"` | ||||||||||||||||||||||||||||||||||||||||
| Labels []OptionLabel `json:"labels"` | ||||||||||||||||||||||||||||||||||||||||
| Hidden bool `json:"hidden"` | ||||||||||||||||||||||||||||||||||||||||
| Redirect *OptionRedirect `json:"redirect,omitempty"` | ||||||||||||||||||||||||||||||||||||||||
| ClusterMetrics clusterMetrics `json:"clusterMetrics,omitempty"` | ||||||||||||||||||||||||||||||||||||||||
| RuleEffects RuleEffects `json:"rule_effects"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type PodConfigWithRules struct { | ||||||||||||||||||||||||||||||||||||||||
| Default string `json:"default"` | ||||||||||||||||||||||||||||||||||||||||
| Values []PodConfigValueWithRules `json:"values"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type PodConfigValueWithRules struct { | ||||||||||||||||||||||||||||||||||||||||
| Id string `json:"id"` | ||||||||||||||||||||||||||||||||||||||||
| DisplayName string `json:"displayName"` | ||||||||||||||||||||||||||||||||||||||||
| Description string `json:"description"` | ||||||||||||||||||||||||||||||||||||||||
| Labels []OptionLabel `json:"labels"` | ||||||||||||||||||||||||||||||||||||||||
| Hidden bool `json:"hidden"` | ||||||||||||||||||||||||||||||||||||||||
| Redirect *OptionRedirect `json:"redirect,omitempty"` | ||||||||||||||||||||||||||||||||||||||||
| ClusterMetrics clusterMetrics `json:"clusterMetrics,omitempty"` | ||||||||||||||||||||||||||||||||||||||||
| RuleEffects RuleEffects `json:"rule_effects"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| type RuleEffects struct { | ||||||||||||||||||||||||||||||||||||||||
| UiHide bool `json:"ui_hide"` | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should discuss if
ListValuesis a "good enough" prefix vs. something (sadly wayy more verbose and) specific.PodTemplateOptionsListValuesHard to know right now - but if/as we add non
PodTemplatedata structures in the future - its possibleListValuesis misleadingly vague.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andyatmiami Yes I agree, this will be more detailed even though it is a bit verbose.