fix/manifest-tag - #319
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughManifest resolution failures now stop conversion and remain cached across cooldown retries. Versionless conversions still skip manifest fetching. The webhook now rejects ChangesVersion handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@api/v1/weightsandbiases_conversion_overrides_test.go`:
- Around line 446-449: Update the conversion test assertions after ConvertTo to
verify the global override value under LegacyOverridesGlobalKey contains the
HTTP_PROXY entry mapped to http://proxy, rather than only asserting that the
global key exists; retain the existing no-manifest-fetch call count assertion.
- Around line 417-449: Rewrite the conversion tests in
weightsandbiases_conversion_overrides_test.go to follow the repository’s suite
pattern: consolidate them under TestV1ConversionSuites(t *testing.T), initialize
RunSpecs, and organize cases as Ginkgo specs. Replace testing.T/Testify require
assertions with Ginkgo/Gomega assertions while preserving the existing
manifest-fetch and conversion behavior checks.
In `@internal/webhook/v2/weightsandbiases_webhook.go`:
- Around line 455-461: Update the version validation around the existing
latest-tag check to reject values with surrounding whitespace, or normalize the
version before manifest conversion so values such as " 0.83.1 " cannot reach
resolution unchanged; ensure whitespace-only values are also invalid. Add
regression tests covering both surrounding whitespace and whitespace-only server
versions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d7af12c-6af1-401f-8101-ba39c3ed4178
📒 Files selected for processing (5)
api/v1/weightsandbiases_conversion_overrides.goapi/v1/weightsandbiases_conversion_overrides_test.goapi/v1/weightsandbiases_conversion_test.gointernal/webhook/v2/weightsandbiases_webhook.gointernal/webhook/v2/weightsandbiases_webhook_test.go
| func TestConvertTo_LegacyOverridesManifestUnavailableNoAppSections(t *testing.T) { | ||
| SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) { | ||
| return serverManifest.Manifest{}, errors.New("registry unreachable") | ||
| }) | ||
| t.Cleanup(disableConversionManifestFetch) | ||
|
|
||
| dst := &appsv2.WeightsAndBiases{} | ||
| src := newV1(withVersion(map[string]interface{}{ | ||
| "global": map[string]interface{}{"host": "http://wandb.example.com"}, | ||
| })) | ||
| require.Error(t, src.ConvertTo(dst)) | ||
| } | ||
|
|
||
| // TestConvertTo_NoVersionSkipsManifestFetch: without a version there is nothing | ||
| // to resolve, so conversion proceeds and global env still converts. | ||
| func TestConvertTo_NoVersionSkipsManifestFetch(t *testing.T) { | ||
| var calls atomic.Int32 | ||
| SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) { | ||
| calls.Add(1) | ||
| return serverManifest.Manifest{}, errors.New("registry unreachable") | ||
| }) | ||
| t.Cleanup(disableConversionManifestFetch) | ||
|
|
||
| dst := &appsv2.WeightsAndBiases{} | ||
| src := newV1(map[string]interface{}{ | ||
| "global": map[string]interface{}{ | ||
| "env": map[string]interface{}{"HTTP_PROXY": "http://proxy"}, | ||
| }, | ||
| }) | ||
| require.NoError(t, src.ConvertTo(dst)) | ||
|
|
||
| overrides := dst.Spec.Wandb.LegacyOverrides | ||
| require.Contains(t, overrides, appsv2.LegacyOverridesGlobalKey) | ||
| require.NotContains(t, overrides, "api") | ||
| require.Contains(t, dst.Spec.Wandb.LegacyOverrides, appsv2.LegacyOverridesGlobalKey) | ||
| require.Equal(t, int32(0), calls.Load(), "no version means no manifest fetch") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'suite_test.go' .
rg -n -C 2 'github.com/onsi/ginkgo|github.com/onsi/gomega|RunSpecs|Describe\(|It\(' \
-g '*_test.go' -g 'go.mod' .Repository: wandb/operator
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- target file outline ---\n'
ast-grep outline api/v1/weightsandbiases_conversion_overrides_suite_test.go api/v1/weightsandbiases_conversion_overrides_test.go 2>/dev/null || true
printf '\n--- api/v1 suite files ---\n'
fd '_suite_test.go|test.go|weightsandbiases_conversion_overrides' api/v1 -t f | sort | sed -n '1,120p'
printf '\n--- target test imports and functions ---\n'
sed -n '1,90p' api/v1/weightsandbiases_conversion_overrides_test.go
rg -n 'func Test(Check|ConvertTo|.*LegacyOverrides|NoVersionSkips|DisableConversionManifest|SetConversionManifest)' api/v1/weightsandbiases_conversion_overrides_test.go
printf '\n--- ginkgo assertion usages in target if present ---\n'
rg -n 'g\\.' api/v1/weightsandbiases_conversion_overrides_test.go || true
rg -n 'require\\.|assert\\.|func test|var _ = Describe|var _ = DescribeTable|It\\(' api/v1/weightsandbiases_conversion_overrides_test.go || true
printf '\n--- deterministic parser-style check for assertion frameworks in target ---\n'
python3 - <<'PY'
from pathlib import Path
p=Path('api/v1/weightsandbiases_conversion_overrides_test.go')
s=p.read_text()
checks = {
'imports_ginkgo': '. "github.com/onsi/ginkgo/v2"' in s,
'imports_gomega': '. "github.com/onsi/gomega"' in s,
'imports_testing': 'testing' in s,
'has_require_nearest': True if 'require' in s else False,
'has_test_func': True if 'func Test' in s else False,
'uses_ginkgo_it': bool(re.search(r'\bIt\s*\(', s)) if (re := __import__('re')) else False,
'uses_gomega_expect': bool(re.search(r'\bExpect\s*\(', s)) if (re := __import__('re')) else False,
}
for k,v in checks.items():
print(f'{k}={v}')
print('require_count=', s.count('require.'))
print('ginkgo_functionlike_count=', s.count('var _ = '))
PYRepository: wandb/operator
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- target file imports ---\n'
sed -n '1,80p' api/v1/weightsandbiases_conversion_overrides_test.go
printf '\n--- target test bodies around added scenarios ---\n'
sed -n '390,460p' api/v1/weightsandbiases_conversion_overrides_test.go
printf '\n--- nearby suite imports for model ---\n'
for f in api/v1/*_suite_test.go api/v1/*conversion*suite*; do
[ -e "$f" ] || continue
echo "--- $f ---"
sed -n '1,40p' "$f"
doneRepository: wandb/operator
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n api/v1/weightsandbiases_conversion_overrides_test.go | sed -n '1,120p'
printf '\n--- target lines 390-460 ---\n'
cat -n api/v1/weightsandbiases_conversion_overrides_test.go | sed -n '390,460p'
printf '\n--- api/v1 related suite files ---\n'
fd '_suite_test.go|weightsandbiases_conversion_overrides' api/v1 -t f | sort
printf '\n--- framework usage in target test ---\n'
python3 - <<'PY'
from pathlib import Path
s = Path('api/v1/weightsandbiases_conversion_overrides_test.go').read_text()
print('imports_ginkgo=', '. "github.com/onsi/ginkgo/v2"' in s)
print('imports_gomega=', '. "github.com/onsi/gomega"' in s)
print('imports_testing=', 'testing' in s)
print('has_require=', s.count('require.') > 0)
print('has_func_test=', 'func Test' in s)
print('has_ginkgo_it=', 'var _ = Describe(' in s or 'It(' in s)
print('has_gomega_expect=', 'Expect(' in s)
PYRepository: wandb/operator
Length of output: 8675
Use the repository suite pattern for conversion tests.
api/v1/weightsandbiases_conversion_overrides_test.go still defines func TestConvertTo_... and uses testing.T/Testify require, while the package has no Ginkgo suite. Rewrite these tests with func TestV1ConversionSuites(t *testing.T) plus RunSpecs, and replace assertions with Ginkgo/Gomega.
🤖 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_overrides_test.go` around lines 417 - 449,
Rewrite the conversion tests in weightsandbiases_conversion_overrides_test.go to
follow the repository’s suite pattern: consolidate them under
TestV1ConversionSuites(t *testing.T), initialize RunSpecs, and organize cases as
Ginkgo specs. Replace testing.T/Testify require assertions with Ginkgo/Gomega
assertions while preserving the existing manifest-fetch and conversion behavior
checks.
Source: Coding guidelines
| require.NoError(t, src.ConvertTo(dst)) | ||
|
|
||
| overrides := dst.Spec.Wandb.LegacyOverrides | ||
| require.Contains(t, overrides, appsv2.LegacyOverridesGlobalKey) | ||
| require.NotContains(t, overrides, "api") | ||
| require.Contains(t, dst.Spec.Wandb.LegacyOverrides, appsv2.LegacyOverridesGlobalKey) | ||
| require.Equal(t, int32(0), calls.Load(), "no version means no manifest fetch") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the converted global environment value.
The key-presence assertion passes if conversion creates an empty global override. Assert that HTTP_PROXY and http://proxy exist in dst.Spec.Wandb.LegacyOverrides[appsv2.LegacyOverridesGlobalKey].
🤖 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_overrides_test.go` around lines 446 - 449,
Update the conversion test assertions after ConvertTo to verify the global
override value under LegacyOverridesGlobalKey contains the HTTP_PROXY entry
mapped to http://proxy, rather than only asserting that the global key exists;
retain the existing no-manifest-fetch call count assertion.
| if strings.TrimSpace(wandb.Spec.Wandb.Version) == "latest" { | ||
| errors = append(errors, field.Invalid( | ||
| field.NewPath("spec").Child("wandb").Child("version"), | ||
| wandb.Spec.Wandb.Version, | ||
| "must be pinned to a published server version; no server-manifest is published for the \"latest\" tag", | ||
| )) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject or normalize whitespace around server versions.
The condition trims wandb.Spec.Wandb.Version only for comparison. It preserves the raw value. Therefore, " " and " 0.83.1 " can pass admission and reach manifest resolution as invalid tags. Reject surrounding whitespace or normalize the value before conversion. Add regression tests for both cases.
Proposed validation
+ version := strings.TrimSpace(wandb.Spec.Wandb.Version)
- if strings.TrimSpace(wandb.Spec.Wandb.Version) == "latest" {
+ if version == "latest" {
errors = append(errors, field.Invalid(
field.NewPath("spec").Child("wandb").Child("version"),
wandb.Spec.Wandb.Version,
"must be pinned to a published server version; no server-manifest is published for the \"latest\" tag",
))
+ } else if version != wandb.Spec.Wandb.Version {
+ errors = append(errors, field.Invalid(
+ field.NewPath("spec").Child("wandb").Child("version"),
+ wandb.Spec.Wandb.Version,
+ "must not contain leading or trailing whitespace",
+ ))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if strings.TrimSpace(wandb.Spec.Wandb.Version) == "latest" { | |
| errors = append(errors, field.Invalid( | |
| field.NewPath("spec").Child("wandb").Child("version"), | |
| wandb.Spec.Wandb.Version, | |
| "must be pinned to a published server version; no server-manifest is published for the \"latest\" tag", | |
| )) | |
| } | |
| version := strings.TrimSpace(wandb.Spec.Wandb.Version) | |
| if version == "latest" { | |
| errors = append(errors, field.Invalid( | |
| field.NewPath("spec").Child("wandb").Child("version"), | |
| wandb.Spec.Wandb.Version, | |
| "must be pinned to a published server version; no server-manifest is published for the \"latest\" tag", | |
| )) | |
| } else if version != wandb.Spec.Wandb.Version { | |
| errors = append(errors, field.Invalid( | |
| field.NewPath("spec").Child("wandb").Child("version"), | |
| wandb.Spec.Wandb.Version, | |
| "must not contain leading or trailing whitespace", | |
| )) | |
| } |
🤖 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 `@internal/webhook/v2/weightsandbiases_webhook.go` around lines 455 - 461,
Update the version validation around the existing latest-tag check to reject
values with surrounding whitespace, or normalize the version before manifest
conversion so values such as " 0.83.1 " cannot reach resolution unchanged;
ensure whitespace-only values are also invalid. Add regression tests covering
both surrounding whitespace and whitespace-only server versions.
1 participant
Pin the conversion to a published manifest version for the cutover instead of relying on
latestSummary by CodeRabbit
Bug Fixes
Validation
latestserver version are now rejected.