Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 70 additions & 14 deletions api/core/v1beta1/connect_config.go
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Workbench, you key the file names and then the actual config values are multi-line strings that get appended. I think it represents KISS wonderfully and there's no cleverness/complexity to worry about. Why not do the same for the Package Manager and Connect configs? Skip the file keys (since there's only a single config), but still just have the value as a multi-line string and append to the end of the generated config. You'd get to skip all the code complexity in here with ordering, de-duping, etc. And it "just works" because of how the parsing of the gcfg works... it'll automatically combine "list" values, and it'll keep latest for scalar values. There's cleverness in doing the keys with dot-notation, but I think it makes things harder rather than easier. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched both Connect and PM to string appending, matching the Workbench pattern. Deleted ~300 lines of section-merge logic in the process.

Also took the naming nit: renamed additionaladditionalConfig across the config structs and JSON tags to align with the existing site-level field names.

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package v1beta1

import (
"fmt"

"reflect"
"strings"
)
Expand Down Expand Up @@ -31,6 +30,12 @@ type ConnectConfig struct {
// see the GenerateGcfg method for our custom handling
RPackageRepository map[string]RPackageRepositoryConfig `json:"RPackageRepositories,omitempty"`
TableauIntegration *ConnectTableauIntegrationConfig `json:"TableauIntegration,omitempty"`

// AdditionalConfig allows appending arbitrary gcfg config content not covered by typed fields.
// The value is appended verbatim after the generated config. gcfg parsing naturally handles
// conflicts: list values are combined, scalar values use the last occurrence.
// +optional
AdditionalConfig string `json:"additionalConfig,omitempty"`
}

type RPackageRepositoryConfig struct {
Expand Down Expand Up @@ -197,35 +202,61 @@ func (configStruct *ConnectConfig) GenerateGcfg() (string, error) {

var builder strings.Builder

// Build an intermediate representation: ordered sections with key-value pairs.
// We use ordered slices to preserve the deterministic output order from reflection.
type sectionEntry struct {
name string
keys []string // ordered key names (for non-slice values)
values map[string]string // key → value
slices map[string][]string // key → multiple values (for gcfg multi-value keys)
}
sections := []sectionEntry{}
sectionIndex := map[string]int{} // section name → index in sections slice

configStructValsPtr := reflect.ValueOf(configStruct)
configStructVals := reflect.Indirect(configStructValsPtr)

for i := 0; i < configStructVals.NumField(); i++ {
fieldName := configStructVals.Type().Field(i).Name
fieldValue := configStructVals.Field(i)

// Skip the AdditionalConfig string — we handle it at the end
if fieldName == "AdditionalConfig" {
continue
}

if fieldValue.IsNil() {
continue
}

sectionStructVals := reflect.Indirect(fieldValue)

// This is to handle the case of the RPackageRepositories
// Handle the RPackageRepositories map (named sections)
if fieldValue.Kind() == reflect.Map {
iter := sectionStructVals.MapRange()

for iter.Next() {
repoName := iter.Key()
repoValue := iter.Value()

builder.WriteString("\n[" + fieldName + " \"" + fmt.Sprintf("%v", repoName) + "\"" + "]\n")

qualifiedName := fieldName + " \"" + fmt.Sprintf("%v", repoName) + "\""
entry := sectionEntry{
name: qualifiedName,
values: map[string]string{},
slices: map[string][]string{},
}
if repoValue.Kind() == reflect.Struct {
builder.WriteString("Url = " + fmt.Sprintf("%v", repoValue.FieldByName("Url")) + "\n")
url := fmt.Sprintf("%v", repoValue.FieldByName("Url"))
entry.keys = append(entry.keys, "Url")
entry.values["Url"] = url
}
sections = append(sections, entry)
// Named sections are not indexed for override — passthrough uses "Section.Key" format
}
} else {
builder.WriteString("\n[" + fieldName + "]\n")
entry := sectionEntry{
name: fieldName,
values: map[string]string{},
slices: map[string][]string{},
}

for j := 0; j < sectionStructVals.NumField(); j++ {
sectionFieldName := sectionStructVals.Type().Field(j).Name
Expand All @@ -235,29 +266,54 @@ func (configStruct *ConnectConfig) GenerateGcfg() (string, error) {
if sectionFieldValue.Kind() == reflect.Ptr {
if !sectionFieldValue.IsNil() {
derefValue := sectionFieldValue.Elem()
// Always write pointer fields when they're not nil, even if empty string
builder.WriteString(fmt.Sprintf("%v", sectionFieldName) + " = " + fmt.Sprintf("%v", derefValue) + "\n")
entry.keys = append(entry.keys, sectionFieldName)
entry.values[sectionFieldName] = fmt.Sprintf("%v", derefValue)
}
// Skip nil pointers entirely
continue
}

if sectionStructVals.Field(j).String() != "" {
if sectionFieldValue.Kind() == reflect.Slice {
var vals []string
for k := 0; k < sectionFieldValue.Len(); k++ {
arrayValue := sectionFieldValue.Index(k).String()
if arrayValue != "" {
builder.WriteString(fmt.Sprintf("%v", sectionFieldName) + " = " + fmt.Sprintf("%v", arrayValue) + "\n")
vals = append(vals, arrayValue)
}
}

if len(vals) > 0 {
entry.keys = append(entry.keys, sectionFieldName)
entry.slices[sectionFieldName] = vals
}
} else {
builder.WriteString(fmt.Sprintf("%v", sectionFieldName) + " = " + fmt.Sprintf("%v", sectionFieldValue) + "\n")
entry.keys = append(entry.keys, sectionFieldName)
entry.values[sectionFieldName] = fmt.Sprintf("%v", sectionFieldValue)
}
}
}

sectionIndex[fieldName] = len(sections)
sections = append(sections, entry)
}
}

// Render sections to gcfg format
for _, section := range sections {
builder.WriteString("\n[" + section.name + "]\n")
for _, key := range section.keys {
if vals, isSlice := section.slices[key]; isSlice {
for _, v := range vals {
builder.WriteString(key + " = " + v + "\n")
}
} else if val, ok := section.values[key]; ok {
builder.WriteString(key + " = " + val + "\n")
}
}
}

if configStruct.AdditionalConfig != "" {
builder.WriteString(configStruct.AdditionalConfig)
}

return builder.String(), nil
}
44 changes: 44 additions & 0 deletions api/core/v1beta1/connect_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,47 @@ func TestConnectConfig_CustomScope(t *testing.T) {
require.Contains(t, str, "OpenIDConnectIssuer = https://example.com")
require.NotContains(t, str, "CustomScope")
}

func TestConnectConfig_AdditionalConfig(t *testing.T) {
// Test basic string append
cfg := ConnectConfig{
Server: &ConnectServerConfig{
Address: "some-address.com",
},
AdditionalConfig: "\n[NewSection]\nNewKey = custom-value\n",
}
str, err := cfg.GenerateGcfg()
require.Nil(t, err)
require.Contains(t, str, "[Server]")
require.Contains(t, str, "Address = some-address.com")
require.Contains(t, str, "[NewSection]")
require.Contains(t, str, "NewKey = custom-value")
}

func TestConnectConfig_AdditionalConfigOverride(t *testing.T) {
// gcfg last-write-wins: appended scalar overrides typed field
cfg := ConnectConfig{
Server: &ConnectServerConfig{
Address: "typed-address.com",
},
AdditionalConfig: "\n[Server]\nAddress = passthrough-address.com\n",
}
str, err := cfg.GenerateGcfg()
require.Nil(t, err)
// Both values appear in the output; gcfg uses last occurrence
require.Contains(t, str, "Address = typed-address.com")
require.Contains(t, str, "Address = passthrough-address.com")
}

func TestConnectConfig_AdditionalConfigEmpty(t *testing.T) {
// Empty string has no effect
cfg := ConnectConfig{
Server: &ConnectServerConfig{
Address: "some-address.com",
},
AdditionalConfig: "",
}
str, err := cfg.GenerateGcfg()
require.Nil(t, err)
require.Contains(t, str, "Address = some-address.com")
}
61 changes: 57 additions & 4 deletions api/core/v1beta1/package_manager_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,50 @@ type PackageManagerConfig struct {
Repos *PackageManagerReposConfig `json:"Repos,omitempty"`
Cran *PackageManagerCRANConfig `json:"CRAN,omitempty"`
Debug *PackageManagerDebugConfig `json:"Debug,omitempty"`

// AdditionalConfig allows appending arbitrary gcfg config content not covered by typed fields.
// The value is appended verbatim after the generated config. gcfg parsing naturally handles
// conflicts: list values are combined, scalar values use the last occurrence.
// +optional
AdditionalConfig string `json:"additionalConfig,omitempty"`
}

func (configStruct *PackageManagerConfig) GenerateGcfg() (string, error) {

var builder strings.Builder

// Build an intermediate representation: ordered sections with key-value pairs.
// We use ordered slices to preserve the deterministic output order from reflection.
type sectionEntry struct {
name string
keys []string // ordered key names (for non-slice values)
values map[string]string // key → value
slices map[string][]string // key → multiple values (for gcfg multi-value keys)
}
sections := []sectionEntry{}
sectionIndex := map[string]int{} // section name → index in sections slice

configStructValsPtr := reflect.ValueOf(configStruct)
configStructVals := reflect.Indirect(configStructValsPtr)

for i := 0; i < configStructVals.NumField(); i++ {
fieldName := configStructVals.Type().Field(i).Name
fieldValue := configStructVals.Field(i)

// Skip the AdditionalConfig string — we handle it at the end
if fieldName == "AdditionalConfig" {
continue
}

if fieldValue.IsNil() {
continue
}

builder.WriteString("\n[" + fieldName + "]\n")
entry := sectionEntry{
name: fieldName,
values: map[string]string{},
slices: map[string][]string{},
}

sectionStructVals := reflect.Indirect(fieldValue)

Expand All @@ -45,19 +71,46 @@ func (configStruct *PackageManagerConfig) GenerateGcfg() (string, error) {

if sectionStructVals.Field(j).String() != "" {
if sectionFieldValue.Kind() == reflect.Slice {
var vals []string
for k := 0; k < sectionFieldValue.Len(); k++ {
arrayValue := sectionFieldValue.Index(k).String()
if arrayValue != "" {
builder.WriteString(fmt.Sprintf("%v", sectionFieldName) + " = " + fmt.Sprintf("%v", arrayValue) + "\n")
vals = append(vals, arrayValue)
}
}

if len(vals) > 0 {
entry.keys = append(entry.keys, sectionFieldName)
entry.slices[sectionFieldName] = vals
}
} else {
builder.WriteString(fmt.Sprintf("%v", sectionFieldName) + " = " + fmt.Sprintf("%v", sectionFieldValue) + "\n")
entry.keys = append(entry.keys, sectionFieldName)
entry.values[sectionFieldName] = fmt.Sprintf("%v", sectionFieldValue)
}
}
}

sectionIndex[fieldName] = len(sections)
sections = append(sections, entry)
}

// Render sections to gcfg format
for _, section := range sections {
builder.WriteString("\n[" + section.name + "]\n")
for _, key := range section.keys {
if vals, isSlice := section.slices[key]; isSlice {
for _, v := range vals {
builder.WriteString(key + " = " + v + "\n")
}
} else if val, ok := section.values[key]; ok {
builder.WriteString(key + " = " + val + "\n")
}
}
}

if configStruct.AdditionalConfig != "" {
builder.WriteString(configStruct.AdditionalConfig)
}

return builder.String(), nil
}

Expand Down
28 changes: 28 additions & 0 deletions api/core/v1beta1/package_manager_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,31 @@ func TestPackageManagerConfig_GenerateGcfg(t *testing.T) {
require.Contains(t, str, "/some/path")
require.Contains(t, str, "/another/path")
}

func TestPackageManagerConfig_AdditionalConfig(t *testing.T) {
// Test basic string append
cfg := PackageManagerConfig{
Server: &PackageManagerServerConfig{
Address: "some-address.com",
},
AdditionalConfig: "\n[NewSection]\nNewKey = custom-value\n",
}
str, err := cfg.GenerateGcfg()
require.Nil(t, err)
require.Contains(t, str, "[Server]")
require.Contains(t, str, "Address = some-address.com")
require.Contains(t, str, "[NewSection]")
require.Contains(t, str, "NewKey = custom-value")
}

func TestPackageManagerConfig_AdditionalConfigEmpty(t *testing.T) {
cfg := PackageManagerConfig{
Server: &PackageManagerServerConfig{
Address: "some-address.com",
},
AdditionalConfig: "",
}
str, err := cfg.GenerateGcfg()
require.Nil(t, err)
require.Contains(t, str, "Address = some-address.com")
}
20 changes: 19 additions & 1 deletion api/core/v1beta1/site_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ type InternalPackageManagerSpec struct {
// AzureFiles configures Azure Files integration for persistent storage
// +optional
AzureFiles *AzureFilesConfig `json:"azureFiles,omitempty"`

// AdditionalConfig allows appending arbitrary gcfg config content to the generated config.
// +optional
AdditionalConfig string `json:"additionalConfig,omitempty"`
}

type InternalConnectSpec struct {
Expand Down Expand Up @@ -275,6 +279,10 @@ type InternalConnectSpec struct {
// for Connect off-host execution
// +optional
AdditionalRuntimeImages []ConnectRuntimeImageSpec `json:"additionalRuntimeImages,omitempty"`

// AdditionalConfig allows appending arbitrary gcfg config content to the generated config.
// +optional
AdditionalConfig string `json:"additionalConfig,omitempty"`
}

type DatabaseSettings struct {
Expand Down Expand Up @@ -402,6 +410,16 @@ type InternalWorkbenchSpec struct {

// JupyterConfig contains Jupyter configuration for Workbench
JupyterConfig *WorkbenchJupyterConfig `json:"jupyterConfig,omitempty"`

// AdditionalConfigs allows appending arbitrary content to Workbench server config files.
// Keys are config file names (e.g., "rserver.conf", "launcher.conf").
// +optional
AdditionalConfigs map[string]string `json:"additionalConfigs,omitempty"`

// AdditionalSessionConfigs allows appending arbitrary content to Workbench session config files.
// Keys are config file names (e.g., "rsession.conf", "repos.conf").
// +optional
AdditionalSessionConfigs map[string]string `json:"additionalSessionConfigs,omitempty"`
}

type InternalWorkbenchExperimentalFeatures struct {
Expand Down Expand Up @@ -442,7 +460,7 @@ type InternalWorkbenchExperimentalFeatures struct {
VsCodeExtensionsDir string `json:"vsCodeExtensionsDir,omitempty"`

// ResourceProfiles for use by Workbench. If not provided, a default will be used
ResourceProfiles map[string]*WorkbenchLauncherKubnernetesResourcesConfigSection `json:"resourceProfiles,omitempty"`
ResourceProfiles map[string]*WorkbenchLauncherKubernetesResourcesConfigSection `json:"resourceProfiles,omitempty"`

// CpuRequestRatio defines the ratio of CPU requests to limits for session pods
// Value must be a decimal number between 0 and 1 (e.g., "0.6" means requests are 60% of limits)
Expand Down
Loading