Skip to content

fix/opv2-feedback #315

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.

Sign up for GitHub

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

Open
collinol wants to merge 3 commits into
base: main
Choose a base branch
from
+254 −43
Open

fix/opv2-feedback #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a81151
fix/opv2-feedback
collinol Aug 5, 2026
8d4ddd7
Merge branch 'main' into fix/opv2-feedback
collinol Aug 5, 2026
a4fcc62
cleanup comments
collinol Aug 6, 2026
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
161 changes: 120 additions & 41 deletions api/v1/weightsandbiases_conversion_mapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ func applyValueMappings(src *WeightsAndBiases, dst *appsv2.WeightsAndBiases) err
if err := mapLegacyOverrides(values, dst); err != nil {
return err
}
// Peer-of-global: reads both the Altinity subchart section and global.clickhouse.
if err := mapClickHouse(values, dst); err != nil {
return err
}

globalMap, found, err := unstructured.NestedMap(values, "global")
if err != nil {
Expand Down Expand Up @@ -126,9 +130,6 @@ func applyGlobalMappings(globalMap map[string]interface{}, dst *appsv2.WeightsAn
if err := mapBucket(globalMap, dst); err != nil {
return err
}
if err := mapClickHouse(globalMap, dst); err != nil {
return err
}

return nil
}
Expand Down Expand Up @@ -535,66 +536,115 @@ var clickHouseFields = []struct {
{"password", func(c *appsv2.ClickHouseConnection, s corev1.SecretKeySelector) { c.Password = s }},
}

// mapClickHouse routes v1 global.clickhouse to externalClickhouse (like
// mapMySQL); external is asserted only when a connection field is present.
func mapClickHouse(globalMap map[string]interface{}, dst *appsv2.WeightsAndBiases) error {
chMap, found, err := unstructured.NestedMap(globalMap, "clickhouse")
// clickHouseInstallPaths are the v1 locations of the ClickHouse install flag,
// in precedence order: the Altinity subchart section wins over global.
var clickHouseInstallPaths = [][]string{
{"clickhouse", "install"},
{"global", "clickhouse", "install"},
}

// clickHouseSource pairs a v1 values section with its path, so errors name the
// section the offending key actually came from.
type clickHouseSource struct {
path string
m map[string]interface{}
}

// mapClickHouse routes v1 ClickHouse config to externalClickhouse. Connection
// fields come from global.clickhouse, falling back to the top-level clickhouse
// section (the Altinity subchart values) for installs that configured them
// there.
func mapClickHouse(values map[string]interface{}, dst *appsv2.WeightsAndBiases) error {
globalCH, _, err := unstructured.NestedMap(values, "global", "clickhouse")
if err != nil {
return fmt.Errorf("spec.values.global.clickhouse: %w", err)
}
if !found || len(chMap) == 0 {
topCH, _, err := unstructured.NestedMap(values, "clickhouse")
if err != nil {
return fmt.Errorf("spec.values.clickhouse: %w", err)
}
if len(globalCH) == 0 && len(topCH) == 0 {
return nil
}

install, installSet := firstClickHouseInstallFlag(values)
if installSet && install {
return nil
}

sources := []clickHouseSource{
{"spec.values.global.clickhouse", globalCH},
{"spec.values.clickhouse", topCH},
}

conn := &appsv2.ClickHouseConnection{}
remaining := map[string]string{}
sawField := false

for _, f := range clickHouseFields {
raw, ok := chMap[f.v1Key]
for _, src := range sources {
raw, ok := src.m[f.v1Key]
if !ok {
continue
}
ref, literal, classifyErr := classifyValueFromOrLiteral(raw)
if classifyErr != nil {
return fmt.Errorf("%s.%s: %w", src.path, f.v1Key, classifyErr)
}
if ref != nil {
f.setRef(conn, *ref)
sawField = true
break
}
// An empty value is no value: let the next source supply it.
if literal != "" {
remaining[f.v1Key] = literal
sawField = true
break
}
}
}

for _, src := range sources {
ps, ok, err := unstructured.NestedMap(src.m, "passwordSecret")
if err != nil {
return fmt.Errorf("%s.passwordSecret: %w", src.path, err)
}
if !ok {
continue
}
ref, literal, classifyErr := classifyValueFromOrLiteral(raw)
if classifyErr != nil {
return fmt.Errorf("spec.values.global.clickhouse.%s: %w", f.v1Key, classifyErr)
name, _, err := unstructured.NestedString(ps, "name")
if err != nil {
return fmt.Errorf("%s.passwordSecret.name: %w", src.path, err)
}
switch {
case ref != nil:
f.setRef(conn, *ref)
sawField = true
case literal != "":
remaining[f.v1Key] = literal
sawField = true
if name == "" || conn.Password.Name != "" {
continue
}
}

if ps, ok, err := unstructured.NestedMap(chMap, "passwordSecret"); err != nil {
return fmt.Errorf("spec.values.global.clickhouse.passwordSecret: %w", err)
} else if ok {
name, _, err := unstructured.NestedString(ps, "name")
key, _, err := unstructured.NestedString(ps, "passwordKey")
if err != nil {
return fmt.Errorf("spec.values.global.clickhouse.passwordSecret.name: %w", err)
return fmt.Errorf("%s.passwordSecret.passwordKey: %w", src.path, err)
}
alreadyHasPassword := conn.Password.Name != ""
if name != "" && !alreadyHasPassword {
key, _, err := unstructured.NestedString(ps, "passwordKey")
if err != nil {
return fmt.Errorf("spec.values.global.clickhouse.passwordSecret.passwordKey: %w", err)
}
if key == "" {
key = defaultClickHousePasswordSecretKey
}
conn.Password = corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{Name: name},
Key: key,
}
delete(remaining, "password")
sawField = true
if key == "" {
key = defaultClickHousePasswordSecretKey
}
conn.Password = corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{Name: name},
Key: key,
}
delete(remaining, "password")
sawField = true
break
}

if !sawField {
if installSet && !install {
return fmt.Errorf(
"spec.values: ClickHouse install=false but no connection found under "+
"global.clickhouse or clickhouse (%s, passwordSecret); set "+
"spec.clickhouse.%s.externalClickhouse explicitly",
clickHouseFieldNames(), appsv2.DefaultInstanceName,
)
}
return nil
}

Expand All @@ -608,6 +658,35 @@ func mapClickHouse(globalMap map[string]interface{}, dst *appsv2.WeightsAndBiase
return nil
}

// firstClickHouseInstallFlag returns the install flag from the first v1
// location that sets it. Non-boolean values are treated as unset rather than failing
func firstClickHouseInstallFlag(values map[string]interface{}) (install bool, found bool) {
for _, path := range clickHouseInstallPaths {
raw, ok, err := unstructured.NestedFieldNoCopy(values, path...)
if err != nil || !ok {
continue
}
s, isScalar := scalarToString(raw)
if !isScalar {
continue
}
parsed, parseErr := strconv.ParseBool(s)
if parseErr != nil {
continue
}
return parsed, true
Comment on lines +669 to +677

@coderabbitai coderabbitai Bot Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect which scalar types can reach strconv.ParseBool.
rg -n -C 8 '^func scalarToString\b|scalarToString\(' api/v1

# Inspect current install-flag coverage and numeric fixtures.
rg -n -C 4 'firstClickHouseInstallFlag|ClickHouseInstall.*NonBool|"install": (0|1)' \
  api/v1/weightsandbiases_conversion_test.go \
  api/v1/weightsandbiases_conversion_mapping.go

Repository: wandb/operator

Length of output: 9308

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '--- mapping excerpts ---\n'
sed -n '665,682p;882,910p' api/v1/weightsandbiases_conversion_mapping.go

printf '\n--- install non-bool test excerpt ---\n'
sed -n '2126,2165p' api/v1/weightsandbiases_conversion_test.go

printf '\n--- parser behavior probe ---\n'
python3 - <<'PY'
valid = {"true", "false", "1", "0"}
values = ["true", "false", "1", "0", "2", "nonsense"]
print("strconv.ParseBool accepted:", *values[:])
for s in values:
    print(f"{s!r}: {s in valid}")
PY

Repository: wandb/operator

Length of output: 243

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- mapping excerpts ---'
sed -n '665,682p;882,910p' api/v1/weightsandbiases_conversion_mapping.go

printf '%s\n' ''
printf '%s\n' '--- install non-bool test excerpt ---'
sed -n '2126,2165p' api/v1/weightsandbiases_conversion_test.go

printf '%s\n' ''
printf '%s\n' '--- parser behavior probe ---'
python3 - <<'PY'
valid = {"true", "false", "1", "0"}
values = ["true", "false", "1", "0", "2", "nonsense"]
print("strconv.ParseBool accepted:", ", ".join(values))
for s in values:
    print(f"{s!r}: {s in valid}")
PY

Repository: wandb/operator

Length of output: 2267

Reject numeric ClickHouse install scalars before parsing.

scalarToString renders JSON numbers as strings such as "0" and "1", so firstClickHouseInstallFlag can return install: 1 as enabled. Keep the declared behavior that only true/false set the flag, and add coverage for numeric values remaining unset.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/v1/weightsandbiases_conversion_mapping.go` around lines 674 - 682, Update
firstClickHouseInstallFlag to reject numeric scalar values before calling
strconv.ParseBool, so only declared boolean true/false values set the install
flag. Preserve the existing handling for valid booleans and add coverage
confirming numeric values such as 0 and 1 leave the flag unset.

}
return false, false
}

func clickHouseFieldNames() string {
names := make([]string, 0, len(clickHouseFields))
for _, f := range clickHouseFields {
names = append(names, f.v1Key)
}
return strings.Join(names, ", ")
}

// redisFields maps each v1 global.redis.<key> to a *RedisConnection setter.
var redisFields = []struct {
v1Key string
Expand Down
136 changes: 134 additions & 2 deletions api/v1/weightsandbiases_conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1986,8 +1986,8 @@ func TestConvertTo_NoClickHouseLeavesEmpty(t *testing.T) {
require.NotContains(t, dst.Annotations, ClickHousePendingAnnotation)
}

// TestConvertTo_ClickHouseOnlyNonConnectionKeys: keys like replicated/install
// must not be misread as an external connection.
// TestConvertTo_ClickHouseOnlyNonConnectionKeys: keys like replicated must not
// be misread as an external connection. (install is meaningful — see below.)
func TestConvertTo_ClickHouseOnlyNonConnectionKeys(t *testing.T) {
dst := &appsv2.WeightsAndBiases{}
src := newV1(map[string]interface{}{
Expand All @@ -2003,3 +2003,135 @@ func TestConvertTo_ClickHouseOnlyNonConnectionKeys(t *testing.T) {
"only non-connection keys must not assert an external clickhouse")
require.NotContains(t, dst.Annotations, ClickHousePendingAnnotation)
}

// TestConvertTo_ClickHouseTopLevelSectionConnection: the Altinity subchart
// section supplies the connection when global.clickhouse doesn't.
func TestConvertTo_ClickHouseTopLevelSectionConnection(t *testing.T) {
dst := &appsv2.WeightsAndBiases{}
src := newV1(map[string]interface{}{
"clickhouse": map[string]interface{}{
"install": false,
"host": "ch.example.com",
"database": "wandb",
"password": map[string]interface{}{
"valueFrom": map[string]interface{}{
"secretKeyRef": map[string]interface{}{
"name": "ch-secret",
"key": "password",
},
},
},
},
})
require.NoError(t, src.ConvertTo(dst))

conn := dst.Spec.ClickHouse[appsv2.DefaultInstanceName].ExternalClickHouse
require.NotNil(t, conn, "install=false must assert an external clickhouse")
require.Equal(t, "ch-secret", conn.Password.Name)
require.Equal(t, "password", conn.Password.Key)

var pending map[string]string
require.NoError(t, json.Unmarshal([]byte(dst.Annotations[ClickHousePendingAnnotation]), &pending))
require.Equal(t, "ch.example.com", pending["host"])
require.Equal(t, "wandb", pending["database"])
}

// TestConvertTo_ClickHouseInstallFalseGlobalConnection: install lives in the
// subchart section while the connection lives under global.
func TestConvertTo_ClickHouseInstallFalseGlobalConnection(t *testing.T) {
dst := &appsv2.WeightsAndBiases{}
src := newV1(map[string]interface{}{
"clickhouse": map[string]interface{}{"install": false},
"global": map[string]interface{}{
"clickhouse": map[string]interface{}{"host": "ch.example.com"},
},
})
require.NoError(t, src.ConvertTo(dst))

require.NotNil(t, dst.Spec.ClickHouse[appsv2.DefaultInstanceName].ExternalClickHouse)
}

// TestConvertTo_ClickHouseGlobalWinsOverTopLevel: global.clickhouse is the
// app-facing connection, so it takes precedence per field.
func TestConvertTo_ClickHouseGlobalWinsOverTopLevel(t *testing.T) {
dst := &appsv2.WeightsAndBiases{}
src := newV1(map[string]interface{}{
"clickhouse": map[string]interface{}{
"install": false,
"host": "subchart.example.com",
"user": "subchart-user",
},
"global": map[string]interface{}{
"clickhouse": map[string]interface{}{"host": "global.example.com"},
},
})
require.NoError(t, src.ConvertTo(dst))

var pending map[string]string
require.NoError(t, json.Unmarshal([]byte(dst.Annotations[ClickHousePendingAnnotation]), &pending))
require.Equal(t, "global.example.com", pending["host"], "global.clickhouse wins")
require.Equal(t, "subchart-user", pending["user"], "subchart fills what global omits")
}

// TestConvertTo_ClickHouseInstallTrueStaysManaged: v1 owned ClickHouse, so the
// spec is left empty for the defaulter even though a connection is present.
func TestConvertTo_ClickHouseInstallTrueStaysManaged(t *testing.T) {
dst := &appsv2.WeightsAndBiases{}
src := newV1(map[string]interface{}{
"clickhouse": map[string]interface{}{"install": true},
"global": map[string]interface{}{
"clickhouse": map[string]interface{}{
"host": "clickhouse.default.svc.cluster.local",
"user": "wandb",
},
},
})
require.NoError(t, src.ConvertTo(dst))

require.Empty(t, dst.Spec.ClickHouse,
"install=true must not assert external; the defaulter makes it managed")
require.NotContains(t, dst.Annotations, ClickHousePendingAnnotation)
}

// TestConvertTo_ClickHouseInstallFalseNoConnectionFails: falling through to the
// defaulter here would silently provision managed ClickHouse.
func TestConvertTo_ClickHouseInstallFalseNoConnectionFails(t *testing.T) {
dst := &appsv2.WeightsAndBiases{}
src := newV1(map[string]interface{}{
"clickhouse": map[string]interface{}{
"install": false,
"replicated": true,
},
})
err := src.ConvertTo(dst)
require.Error(t, err)
require.Contains(t, err.Error(), "install=false but no connection found")
}

// TestConvertTo_ClickHouseInstallStringBool: helm values are frequently
// stringly-typed; "false" must behave like false.
func TestConvertTo_ClickHouseInstallStringBool(t *testing.T) {
dst := &appsv2.WeightsAndBiases{}
src := newV1(map[string]interface{}{
"clickhouse": map[string]interface{}{
"install": "false",
"host": "ch.example.com",
},
})
require.NoError(t, src.ConvertTo(dst))

require.NotNil(t, dst.Spec.ClickHouse[appsv2.DefaultInstanceName].ExternalClickHouse)
}

// TestConvertTo_ClickHouseInstallNonBoolIsUnset: an uninterpretable flag must
// not make a v1 object unservable; it falls back to connection presence.
func TestConvertTo_ClickHouseInstallNonBoolIsUnset(t *testing.T) {
dst := &appsv2.WeightsAndBiases{}
src := newV1(map[string]interface{}{
"clickhouse": map[string]interface{}{
"install": map[string]interface{}{"nested": "nonsense"},
},
})
require.NoError(t, src.ConvertTo(dst))
require.Empty(t, dst.Spec.ClickHouse)
}
Comment on lines +2009 to +2137

@coderabbitai coderabbitai Bot Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the configured Ginkgo/Gomega suites and their setup patterns.
fd 'suite_test\.go$|go\.mod$' .
rg -n -C 3 'RunSpecs|RegisterFailHandler|ginkgo|gomega' \
  --glob '*.go' --glob 'go.mod'

Repository: wandb/operator

Length of output: 1110

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Changed file candidates:\n'
git ls-files | rg '(^|/)weightsandbiases_conversion_test\.go$|suite_test\.go$' || true

printf '\nFile stats:\n'
wc -l $(git ls-files | rg '(^|/)weightsandbiases_conversion_test\.go$|suite_test\.go$' || true) | tail -20

printf '\nImports and test style in weightsandбиases_conversion_test.go:\n'
file=$(git ls-files | rg '(^|/)weightsandbiases_conversion_test\.go$' | head -1)
if [ -n "${file:-}" ]; then
  sed -n '1,80p' "$file"
  printf '\nFunction declarations in file:\n'
  rg -n '^\s*func\s+Test|Describe|Context|It|When|Ginkgo|Gomega|require|assert' "$file" || true
fi

printf '\nRelevant suite_test.go examples:\n'
for f in internal/controller/common/common_suite_test.go internal/controller/suite_test.go internal/webhook/v2/webhook_suite_test.go; do
  if [ -f "$f" ]; then
    echo "--- $f"
    sed -n '1,140p' "$f"
  fi
done

Repository: wandb/operator

Length of output: 46687

Move the added ClickHouse conversion cases into a Ginkgo/Gomega suite.

TestConvertTo_ClickHouse* uses direct func Test... *testing.T functions and Testify require... assertions. Add the cases to the configured Ginkbo/Gomega suite and use Gomega assertions.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/v1/weightsandbiases_conversion_test.go` around lines 2009 - 2137, Move
all added TestConvertTo_ClickHouse* cases into the configured Ginkgo suite,
replacing standalone testing.T functions with the suite’s established
Describe/It structure. Convert every Testify require assertion to equivalent
Gomega matchers and preserve the existing test scenarios and expectations.

Source: Coding guidelines

Loading