Remove config.groupRef fallback and external_auth_config_ref enum#4834
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4834 +/- ##
==========================================
+ Coverage 69.14% 69.15% +0.01%
==========================================
Files 530 530
Lines 55278 55270 -8
==========================================
+ Hits 38220 38223 +3
+ Misses 14132 14120 -12
- Partials 2926 2927 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: kubernetes-expert, code-reviewer, toolhive-expert
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | crd-api.md needs rebase (telemetry note conflict) | 10/10 | MEDIUM | Rebase |
| 2 | Missing Validate() test for Config.Group-only rejection | 8/10 | MEDIUM | Discuss |
| 3 | No migration/upgrade documentation for breaking change | 7/10 | MEDIUM | Discuss |
| 4 | Stale test fixtures (DefaultValues, NamespaceIsolation) | 10/10 | LOW | Discuss |
| 5 | Redundant Config.Group in controller test fixtures | 10/10 | LOW | Info |
| 6 | GroupRef +optional despite being required at runtime | 9/10 | INFO | Info |
Overall
Clean deprecation removal that correctly eliminates all three aspects of the backwards-compatibility code: the Go constant, the kubebuilder enum value, and the fallback logic. The converter bridge (config.Group = vmcp.ResolveGroupName()) is well-placed and follows the established pattern of overriding authoritative fields after DeepCopy.
The main concern is that the PR branch appears to predate the telemetry deprecation note addition to crd-api.md on main, which will likely cause a merge conflict on the config field description line. The fix is straightforward — rebase and regenerate. The two MEDIUM test/docs findings are worth discussing but not blocking: adding a Validate() test for the specific breaking-change path would strengthen coverage, and migration guidance would help users upgrading from the deprecated patterns.
Additional Findings (not in diff)
- [LOW] Stale test fixtures (F4):
TestVirtualMCPServerDefaultValues(~line 140) andTestVirtualMCPServerNamespaceIsolation(~line 167) invirtualmcpserver_types_test.gostill construct fixtures using onlyConfig.GroupwithoutGroupRef. These tests don't callValidate()so they pass, but they represent stale patterns inconsistent with the rest of the PR. AddingGroupRefto these two tests would be a minimal fix.
Generated with Claude Code
d95da04 to
ddfc9e9
Compare
Remove the deprecated config.groupRef fallback from ResolveGroupName()
and Validate() on VirtualMCPServer. The converter now explicitly sets
config.Group from spec.groupRef so downstream runtime code still works.
Remove the deprecated DeprecatedBackendAuthTypeExternalAuthConfigRef
constant ("external_auth_config_ref") from the kubebuilder enum, the
converter's deprecation handling, and the Validate() switch case.
Update all test fixtures to use spec.GroupRef instead of Config.Group.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The field is no longer deprecated — it is actively used by the standalone CLI and populated by the operator converter from spec.groupRef. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The E2E tests were creating VirtualMCPServer objects with only Config.Group and no spec.groupRef, causing validation failures now that the config.groupRef fallback is removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ixtures Make GroupRef +kubebuilder:validation:Required on VirtualMCPServer (F6), matching the IncomingAuth pattern. The API server now rejects resources missing groupRef at admission time. Add TestVirtualMCPServer_Validate_RequiresGroupRef covering nil and empty-name rejection paths (F2). Fix stale DefaultValues and NamespaceIsolation fixtures to use GroupRef instead of Config.Group (F4). Remove redundant Config.Group from all controller test fixtures that already have GroupRef, and replace direct Spec.Config.Group reads with ResolveGroupName() (F5). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ddfc9e9 to
cbf98ef
Compare
22 E2E test files were creating VirtualMCPServer objects directly (not via helpers) without GroupRef. Now that the CRD schema requires groupRef at admission time, these all fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cbf98ef to
f35496e
Compare
21 integration test fixtures in test-integration/ were creating VirtualMCPServer objects without spec.groupRef, causing admission failures now that the field is required. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Remove deprecated backwards-compatibility code from VirtualMCPServer: the
config.groupReffallback inResolveGroupName()/Validate()and theexternal_auth_config_refsnake_case enum value fromBackendAuthConfig.Type. Also promotesspec.groupReffrom+optionalto+kubebuilder:validation:Required.Closes #4831
Medium level
ResolveGroupName()simplified to only returnspec.groupRef.name(no fallback toConfig.Group)Validate()now requiresspec.groupRef.nameinstead of accepting either fieldGroupRefpromoted from+optionalto+kubebuilder:validation:Required— the API server now rejects resources missinggroupRefat admission timeconfig.Group = vmcp.ResolveGroupName()after DeepCopy so downstream runtime code still worksDeprecatedBackendAuthTypeExternalAuthConfigRefconstant removed along with its deprecation warning, kubebuilder enum entry, and switch case handlingGroupRef; redundantConfig.Groupremoved from fixturesLow level
cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.goResolveGroupName(), tightenValidate(), makeGroupRefrequired, remove deprecated constant and enum value, clean up commentscmd/thv-operator/pkg/vmcpconfig/converter.goconfig.Group = vmcp.ResolveGroupName()after DeepCopy; remove deprecated snake_case handlingpkg/vmcp/config/config.goConfig.Groupcomment to reflect current usage (no longer deprecated)docs/operator/virtualmcpserver-api.mdexternalAuthConfigRef*_test.go(14 files)GroupRefto all VirtualMCPServer test fixtures, remove redundantConfig.Group, addValidate_RequiresGroupReftestcrd-api.mdMigration guide
This is a breaking change for users of the deprecated fields:
spec.config.groupRef→spec.groupRefBefore:
After:
spec.groupRefis now required — the API server will reject VirtualMCPServer resources that omit it. The oldspec.config.groupRefstring is no longer read by the operator.external_auth_config_ref→externalAuthConfigRefBefore:
After:
The snake_case value is no longer accepted by the CRD schema.
Type of change
Test plan
task lint-fixpassestask testpasses (operator unit tests)go build ./cmd/...passesTestVirtualMCPServer_Validate_RequiresGroupRefcovers nil and empty-name rejection pathsSpecial notes for reviewers
Config.Groupinpkg/vmcp/config/config.gois deliberately kept — it is still used by the standalone CLI path. The operator converter populates it fromspec.groupRefduring conversion.GroupRefis now+kubebuilder:validation:Required(withoutomitempty), matching theIncomingAuthpattern on the same struct. TheValidate()check remains as defense-in-depth for thegroupRef: {}edge case.Generated with Claude Code