feat: add passthrough config mechanism for Connect, Package Manager, and Workbench#75
feat: add passthrough config mechanism for Connect, Package Manager, and Workbench#75ian-flores wants to merge 3 commits intomainfrom
Conversation
…and Workbench Add escape-hatch passthrough fields to all three product configs so users can set arbitrary config values without waiting for operator releases. Connect & Package Manager: - Add `additional` map[string]string on config structs - Keys use "Section.Key" format (e.g., "Server.DataDir") - Passthrough values override typed fields when both are set - Rewrite GenerateGcfg() to use intermediate representation for merge Workbench: - Add `additionalConfigs` map[string]string on WorkbenchIniConfig and WorkbenchSessionIniConfig - Keys are config file names (e.g., "rserver.conf") - Raw content appended after generated config per file Site CRD propagation added for all three products. Also fixes typo: WorkbenchLauncherKubnernetesResourcesConfigSection → WorkbenchLauncherKubernetesResourcesConfigSection
|
Claude finished @ian-flores's task —— View job Review Complete
Submitted a review with 2 issues to address:
Overall, the passthrough mechanism is well-designed with good test coverage. |
There was a problem hiding this comment.
Review Summary
This PR implements a well-designed passthrough config mechanism for all three products. The design choices are sound: Section.Key format for gcfg products (Connect/PM) and file-based appending for Workbench's multiple config formats.
Issues to Address
1. Non-deterministic map iteration (Connect & Package Manager)
api/core/v1beta1/connect_config.go:310andapi/core/v1beta1/package_manager_config.go:98- Iterating over
Additionalmap has non-deterministic ordering in Go - When multiple keys target the same new section, their order within that section will vary between runs
- Suggestion: Sort the keys before processing:
keys := make([]string, 0, len(configStruct.Additional))
for key := range configStruct.Additional {
keys = append(keys, key)
}
sort.Strings(keys)
for _, key := range keys {
// process
}2. Potential nil pointer dereference (Workbench controller)
internal/controller/core/site_controller_workbench.go:402and:407- If
targetWorkbench.Spec.Config.WorkbenchIniConfigorWorkbenchSessionIniConfigis nil, the assignment will panic - Suggestion: Add nil guards:
if site.Spec.Workbench.AdditionalConfigs != nil {
if targetWorkbench.Spec.Config.WorkbenchIniConfig == nil {
targetWorkbench.Spec.Config.WorkbenchIniConfig = &v1beta1.WorkbenchIniConfig{}
}
targetWorkbench.Spec.Config.WorkbenchIniConfig.AdditionalConfigs = site.Spec.Workbench.AdditionalConfigs
}Positives
- Good test coverage: Edge cases like malformed keys, empty maps, nil maps, and override scenarios are well tested
- Typo fix:
WorkbenchLauncherKubnernetesResourcesConfigSection→WorkbenchLauncherKubernetesResourcesConfigSectionis a good cleanup - Consistent naming:
AdditionalConfigfor single-file products vsAdditionalConfigs/AdditionalSessionConfigsfor Workbench makes the behavior clear SplitN(..., 2)approach: Correctly handles multi-dot keys likeGood.Section.Key- Shared
sectionEntrypattern: Well-structured for maintaining key ordering; duplication between Connect/PM is acceptable for now
Notes
- The Workbench
AdditionalConfigsmap iteration (workbench_config.go:173) also has non-deterministic order, but since each filename is independent (not affecting order within a file), this is lower priority - CRD updates, deepcopy, and client-go generated code all look correct
…lm chart - Sort Additional map keys before iterating in Connect and PM GenerateGcfg() for deterministic output - Add nil guards for WorkbenchIniConfig and WorkbenchSessionIniConfig in site controller before propagating additional configs - Regenerate Helm chart CRDs to match kustomize
WorkbenchIniConfig and WorkbenchSessionIniConfig are embedded value types (not pointers) in WorkbenchConfig, so they cannot be nil. Remove the incorrect nil guards that caused compilation errors. Also fix formatting flagged by CI.
|
thanks for creating this! we have long felt the pain of needing this escape hatch. I think this would unblock @brendanhcullen, who needs support for setting Connect |
Summary
additionalmap withSection.Keyformat, passthrough overrides typed fieldsadditionalConfigsmap with config file name keys, raw content appended per fileWorkbenchLauncherKubnernetesResourcesConfigSection→WorkbenchLauncherKubernetesResourcesConfigSectionMotivation
The operator currently defines ~25-33% of each product's config values as typed Go struct fields. Adding a new setting requires modifying 4+ files, running codegen, and releasing a new operator version. This passthrough mechanism lets users set any config value immediately.
Design
Connect & Package Manager (gcfg format)
Section.Keyformat matching gcfg structureWorkbench (multiple config files)
Test plan
go test ./api/core/v1beta1/...)