feat: auto-generate Edit Config page from Site CRD (#39)#79
feat: auto-generate Edit Config page from Site CRD (#39)#79ian-flores wants to merge 11 commits intomainfrom
Conversation
- Add isCRDPresent() helper to check if CRDs exist before setting up controllers - Conditionally setup Site and Flightdeck controllers only when CRDs exist - Update flightdeck kube client to return empty Site when CRD doesn't exist - Make Ready endpoint treat missing CRDs as non-fatal - Add tests for no-CRD scenarios Fixes #38
- Add CRDConfigPage that uses Go reflection to parse Site CRD spec - Dynamically render configuration fields from CRD structure - Organize fields into sections (Basic, Product, Advanced) - Skip zero values and internal fields for cleaner display - Add tests for CRD config rendering - Replace hard-coded SiteConfigPage with dynamic CRDConfigPage This allows the configuration page to automatically adapt when the Site CRD changes, eliminating the need to manually update the UI code.
|
Claude finished @ian-flores's task —— View job Reviewing PR
Review submitted with feedback on error handling, field categorization, and minor code improvements. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Nice implementation of auto-generated config from the Site CRD. The reflection-based approach is a good solution for keeping the UI in sync with CRD changes. Here's my feedback:
Important
flightdeck/internal/kube.go:119 - Error string matching is brittle
if strings.Contains(errStr, "the server could not find the requested resource") ||
strings.Contains(errStr, "no matches for kind") {These strings may change across Kubernetes versions. Consider using k8s.io/apimachinery/pkg/api/errors functions like errors.IsNotFound() or checking for specific API error types instead.
flightdeck/html/crdconfig.go:349-354 - Hardcoded field categorization
The productNames and advancedNames lists are hardcoded, which partially defeats the purpose of auto-generating from the CRD. When new fields are added, someone needs to remember to update these lists. Consider:
- Documenting this limitation
- Using struct tags in the CRD to annotate category
- Defaulting unknown fields to a sensible category
Suggestions
flightdeck/html/crdconfig.go:366-371 - No-op ReplaceAll calls
name = strings.ReplaceAll(name, "AWS", "AWS")
name = strings.ReplaceAll(name, "VPC", "VPC")These are no-ops. If the intent is to preserve acronyms, this approach doesn't accomplish that since the code below still processes them character-by-character.
flightdeck/html/crdconfig.go:10-11 - Dot imports
Using dot imports makes it unclear where Node, Text, Class, Div, etc. come from. Consider using named imports unless dot imports are an established pattern in this codebase.
flightdeck/html/crdconfig.go:131 - Nested append
allFields := append(append(basicFields, productFields...), advancedFields...)This works but could be clearer with explicit allocation or slices.Concat.
cmd/team-operator/main.go:168-172 - Log level for missing CRDs
The "Site CRD not found" message is at Info level. Consider Warn level so operators are more aware the controller isn't running.
flightdeck/internal/kube_test.go:68 - Incomplete happy path test
The "Site found - returns site" test case doesn't properly mock a successful response, so it doesn't actually verify the happy path.
flightdeck/html/crdconfig.go:400 & crdconfig_test.go:139 & kube_test.go:109 - Missing newlines at end of files.
Overall
The approach is sound and achieves the goal of reducing manual UI maintenance when the CRD changes. The tests cover the core functionality well. The main areas for improvement are around error handling robustness and the partially-hardcoded field categorization.
- Use apierrors.IsNotFound() instead of brittle string matching - Remove no-op ReplaceAll calls in formatFieldName - Use slices.Concat instead of nested append - Document hardcoded field categorization lists - Change missing CRD log level from Info to Warn - Update tests to use real k8s API error types
- Add max-w-7xl layout wrapper matching homepage - Render products as card sections instead of nested tables - Use Posit design tokens for borders and backgrounds - Switch to bottom-only borders for cleaner table appearance - Render nested structs as indented sections, not subtables
apiextensionsv1.JSON fields (e.g., userSettings, vsCodeUserSettings) were rendering as raw byte arrays. Add formatValue helper that detects structs with Raw []byte fields and decodes them to strings.
- Pointer fields like *int now show their value instead of memory address
- Map values containing structs are recursively expanded instead of
printing Go struct literals like {default 1 2560 0}
- Add deref helper for consistent pointer dereferencing
- Handle pointer fields in renderNestedStruct
Map values like userSettings contain apiextensionsv1.JSON structs which were being recursively expanded, showing Raw byte arrays as individual numbers. Now checks formatValue first to decode them.
|
Confirmed properly rendering in ganso01-staging and then reverted back to main. 
|
1 participant
Summary
flightdeckshould consume the SiteSpec Resource Definition and Auto-Generate the Edit-Config page #39Changes
CRDConfigPagethat uses Go reflection to parse and render the Site CRD specTesting
flightdeck/html/crdconfig_test.goBenefits
Fixes #39