[dashboard] Add storageClass dropdown for stateful apps#2131
Conversation
Replace plain text input with an API-backed listInput dropdown for
storageClass fields across all applications that expose them.
The dropdown fetches available StorageClasses from the cluster via
/api/clusters/{cluster}/k8s/apis/storage.k8s.io/v1/storageclasses,
following the same pattern as the instanceType dropdown for VMInstance.
Top-level spec.storageClass: ClickHouse, Harbor, HTTPCache, Kubernetes,
MariaDB, MongoDB, NATS, OpenBAO, Postgres, Qdrant, RabbitMQ, Redis, VMDisk.
Nested paths: FoundationDB (spec.storage.storageClass),
Kafka (spec.kafka.storageClass and spec.zookeeper.storageClass).
Co-Authored-By: Claude <[email protected]>
Signed-off-by: Kirill Ilin <[email protected]>
sircthulhu
requested review from
IvanHunters,
androndo,
kvaps,
lexfrei and
lllamnyp
as code owners
dosubot
bot
added
size:M
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience for configuring stateful applications within the dashboard. By transitioning Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThe changes add support for API-backed storageClass dropdowns across multiple Kubernetes resource kinds in the dashboard custom forms, enabling dynamic selection of available StorageClasses via the cluster's StorageClasses API. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature by replacing the free-text input for storageClass with a dropdown populated from the cluster's available StorageClasses. The implementation correctly handles different application kinds with varying schema structures. The accompanying tests provide good coverage for the new logic. I've included a couple of suggestions to improve the maintainability of the code, primarily around reducing code duplication between the implementation and tests, and making the test code more robust.
| "ClickHouse", "Harbor", "HTTPCache", "Kubernetes", "MariaDB", "MongoDB", | ||
| "NATS", "OpenBAO", "Postgres", "Qdrant", "RabbitMQ", "Redis", "VMDisk", |
There was a problem hiding this comment.
This list of kinds is a duplicate of the list in the case statement in applyListInputOverrides in customformsoverride.go. This duplication can lead to maintenance issues where one list is updated but not the other. To improve this, consider defining this list in a single place (e.g., a package-level variable) and reusing it in both the implementation and the tests. This would make the code more robust to future changes.
| schema := map[string]any{} | ||
| applyListInputOverrides(schema, kind, map[string]any{}) | ||
|
|
||
| specProps := schema["properties"].(map[string]any)["spec"].(map[string]any)["properties"].(map[string]any) |
There was a problem hiding this comment.
This long chain of type assertions and map lookups is fragile and repeated across the new tests. For example, this line is already quite long, and the one in TestApplyListInputOverrides_StorageClassFoundationDB (line 258) is even longer. A small change in the schema structure could break these tests in hard-to-debug ways. Consider creating a helper function within the test file to safely navigate the schema and retrieve nested properties. This would make the tests cleaner, more readable, and more robust.
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/dashboard/customformsoverride_test.go (1)
286-299: HardenassertStorageClassListInputto validate key mappings too.Right now it checks only
typeandvalueUri; a regression inkeysToValue/keysToLabelwould still pass.✅ Suggested assertion extension
func assertStorageClassListInput(t *testing.T, field map[string]any) { t.Helper() if field["type"] != "listInput" { t.Errorf("expected type listInput, got %v", field["type"]) } customProps, ok := field["customProps"].(map[string]any) if !ok { t.Fatal("customProps not found") } expectedURI := "/api/clusters/{cluster}/k8s/apis/storage.k8s.io/v1/storageclasses" if customProps["valueUri"] != expectedURI { t.Errorf("expected valueUri %s, got %v", expectedURI, customProps["valueUri"]) } + expected0, expected1 := "metadata", "name" + if got, ok := customProps["keysToValue"].([]any); !ok || len(got) != 2 || got[0] != expected0 || got[1] != expected1 { + t.Errorf("expected keysToValue [metadata name], got %v", customProps["keysToValue"]) + } + if got, ok := customProps["keysToLabel"].([]any); !ok || len(got) != 2 || got[0] != expected0 || got[1] != expected1 { + t.Errorf("expected keysToLabel [metadata name], got %v", customProps["keysToLabel"]) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/dashboard/customformsoverride_test.go` around lines 286 - 299, The test helper assertStorageClassListInput only checks field["type"] and customProps["valueUri"], so it won't catch regressions in keysToValue/keysToLabel mappings; update assertStorageClassListInput to also assert that customProps contains the expected keysToValue and keysToLabel entries (e.g., ensure customProps["keysToValue"] and customProps["keysToLabel"] exist, are the expected strings or arrays used by the UI, and map the correct fields), and fail the test with descriptive errors if those keys are missing or incorrect so regressions in keysToValue/keysToLabel are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/dashboard/customformsoverride.go`:
- Around line 218-231: The new overrides replace any existing OpenAPI default
for storageClass when assigning storageClassListInput(), so preserve an existing
default value from the target schema node before overwriting: for each place you
set specProps["storageClass"], storageProps["storageClass"],
kafkaProps["storageClass"], and zkProps["storageClass"], read the current
props["storageClass"] (if present) and copy its "default" into the object
returned by storageClassListInput() (e.g. new := storageClassListInput(); if
old, ok := props["storageClass"].(map[string]interface{}); ok { if d, has :=
old["default"]; has { new["default"] = d } }; props["storageClass"] = new),
using ensureSchemaPath and storageClassListInput as shown.
---
Nitpick comments:
In `@internal/controller/dashboard/customformsoverride_test.go`:
- Around line 286-299: The test helper assertStorageClassListInput only checks
field["type"] and customProps["valueUri"], so it won't catch regressions in
keysToValue/keysToLabel mappings; update assertStorageClassListInput to also
assert that customProps contains the expected keysToValue and keysToLabel
entries (e.g., ensure customProps["keysToValue"] and customProps["keysToLabel"]
exist, are the expected strings or arrays used by the UI, and map the correct
fields), and fail the test with descriptive errors if those keys are missing or
incorrect so regressions in keysToValue/keysToLabel are caught.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/controller/dashboard/customformsoverride.gointernal/controller/dashboard/customformsoverride_test.go
| case "ClickHouse", "Harbor", "HTTPCache", "Kubernetes", "MariaDB", "MongoDB", | ||
| "NATS", "OpenBAO", "Postgres", "Qdrant", "RabbitMQ", "Redis", "VMDisk": | ||
| specProps := ensureSchemaPath(schema, "spec") | ||
| specProps["storageClass"] = storageClassListInput() | ||
|
|
||
| case "FoundationDB": | ||
| storageProps := ensureSchemaPath(schema, "spec", "storage") | ||
| storageProps["storageClass"] = storageClassListInput() | ||
|
|
||
| case "Kafka": | ||
| kafkaProps := ensureSchemaPath(schema, "spec", "kafka") | ||
| kafkaProps["storageClass"] = storageClassListInput() | ||
| zkProps := ensureSchemaPath(schema, "spec", "zookeeper") | ||
| zkProps["storageClass"] = storageClassListInput() |
There was a problem hiding this comment.
Potential regression: storageClass defaults are dropped by the new overrides.
Line 221/225/229 assign a fresh listInput schema but never carry over an existing OpenAPI default, unlike the VMInstance handling at Line 200-Line 204. If any affected CRD defines a default storageClass, the form will stop pre-populating it.
💡 Proposed fix (preserve default when available)
case "ClickHouse", "Harbor", "HTTPCache", "Kubernetes", "MariaDB", "MongoDB",
"NATS", "OpenBAO", "Postgres", "Qdrant", "RabbitMQ", "Redis", "VMDisk":
specProps := ensureSchemaPath(schema, "spec")
- specProps["storageClass"] = storageClassListInput()
+ field := storageClassListInput()
+ if prop, _ := openAPIProps["storageClass"].(map[string]any); prop != nil {
+ if def := prop["default"]; def != nil {
+ field["default"] = def
+ }
+ }
+ specProps["storageClass"] = field🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/dashboard/customformsoverride.go` around lines 218 - 231,
The new overrides replace any existing OpenAPI default for storageClass when
assigning storageClassListInput(), so preserve an existing default value from
the target schema node before overwriting: for each place you set
specProps["storageClass"], storageProps["storageClass"],
kafkaProps["storageClass"], and zkProps["storageClass"], read the current
props["storageClass"] (if present) and copy its "default" into the object
returned by storageClassListInput() (e.g. new := storageClassListInput(); if
old, ok := props["storageClass"].(map[string]interface{}); ok { if d, has :=
old["default"]; has { new["default"] = d } }; props["storageClass"] = new),
using ensureSchemaPath and storageClassListInput as shown.
2 participants
What this PR does
Replaces the plain text input for
storageClassfields with anAPI-backed dropdown listing available StorageClasses from the cluster.
Follows the same pattern as the
instanceTypedropdown for VMInstance.Affected applications:
spec.storageClass: ClickHouse, Harbor, HTTPCache,Kubernetes, MariaDB, MongoDB, NATS, OpenBAO, Postgres, Qdrant,
RabbitMQ, Redis, VMDisk
spec.storage.storageClass: FoundationDBspec.kafka.storageClass/spec.zookeeper.storageClass: KafkaRelease note
Summary by CodeRabbit
Release Notes
New Features
Tests