WIP: cnf network: Add OVN RouteAdvertisement support #1129
Conversation
gkopels
requested review from
kononovn,
trewest,
achuzhoy,
cdvultur and
klaskosk
as code owners
WalkthroughAdd full OVN RouteAdvertisements CRD support: new API types, clientset, informers/listers, apply-configurations, deepcopy, builder and list helpers, unit tests, sync config entries for upstream ovn-kubernetes, and dependency updates in go.mod. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Builder as RouteAdvertisementBuilder
participant API as Kubernetes API
Caller->>Builder: NewRouteAdvertisementBuilder(apiClient,name,ads,...)
Builder-->>Caller: builder (Definition set) / error
Caller->>Builder: Create()
Builder->>API: GET RouteAdvertisement (key)
alt NotFound
Builder->>API: CREATE RouteAdvertisement
API-->>Builder: Created
Builder-->>Caller: Object set
else Found
API-->>Builder: Exists
Builder-->>Caller: error or no-op
end
Caller->>Builder: Get()/Exists()
Builder->>API: GET RouteAdvertisement
API-->>Builder: Object / NotFound
Builder-->>Caller: object/error or false
Caller->>Builder: Update()
Builder->>API: UPDATE RouteAdvertisement
API-->>Builder: Updated / error
Builder-->>Caller: builder / error
Caller->>Builder: Delete()
Builder->>API: GET RouteAdvertisement
alt Found
Builder->>API: DELETE RouteAdvertisement
API-->>Builder: Deleted
Builder-->>Caller: nil
else NotFound
Builder-->>Caller: nil
end
Caller->>API: ListRouteAdvertisements(apiClient, ns, opts)
API-->>Caller: RouteAdvertisementList
Caller-->>Caller: build []*RouteAdvertisementBuilder from items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)level=warning msg="The linter 'wsl' is deprecated (since v2.2.0) due to: new major version. Replaced by wsl_v5." ... [truncated 2298 characters] ... ked as explicit in vendor/modules.txt, but not explicitly required in go.mod\n\tk8s.io/[email protected]: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod\n\tk8s.io/[email protected]: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod\n\tk8s.io/[email protected]: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod\n\tk8s.io/[email protected]: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod\n\tsigs.k8s.io/[email protected]: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (11)
internal/sync/configs/ovn-k-config.yaml (1)
4-6: Prefer pinning to a commit for deterministic syncs instead of tracking “main”Tracking main can introduce non-reproducible builds. Consider pinning a specific commit SHA and updating intentionally via tooling.
Also applies to: 15-17
pkg/ovn/routeadvertisementlist.go (1)
13-38: Accept client.ListOption and allow multiple filters; set namespace via InNamespace; use Background contextThis broadens usability and simplifies option handling. Also pre-size the slice.
@@ -// ListRouteAdvertisements returns RouteAdvertisement inventory in the given namespace. -func ListRouteAdvertisements(apiClient *clients.Settings, nsname string, options ...runtimeClient.ListOptions) ([]*RouteAdvertisementBuilder, error) { +// ListRouteAdvertisements returns RouteAdvertisement inventory in the given namespace. +func ListRouteAdvertisements(apiClient *clients.Settings, nsname string, options ...runtimeClient.ListOption) ([]*RouteAdvertisementBuilder, error) { @@ - logMessage := fmt.Sprintf("Listing RouteAdvertisements in the namespace %s", nsname) - passedOptions := runtimeClient.ListOptions{} - - if len(options) == 1 { - passedOptions = options[0] - logMessage += fmt.Sprintf(" with the options %v", passedOptions) - } else if len(options) > 1 { - glog.V(100).Infof("error: more than one ListOptions was passed") - - return nil, fmt.Errorf("error: more than one ListOptions was passed") - } - - glog.V(100).Infof(logMessage) + logMessage := fmt.Sprintf("Listing RouteAdvertisements in the namespace %s", nsname) + if len(options) > 0 { + logMessage += " with additional list options" + } + glog.V(100).Infof(logMessage) @@ - routeAdvertisementList := &ovnv1.RouteAdvertisementList{} - passedOptions.Namespace = nsname - - err = apiClient.Client.List(context.TODO(), routeAdvertisementList, &passedOptions) + routeAdvertisementList := &ovnv1.RouteAdvertisementList{} + opts := append([]runtimeClient.ListOption{runtimeClient.InNamespace(nsname)}, options...) + err = apiClient.Client.List(context.Background(), routeAdvertisementList, opts...) @@ - var routeAdvertisementObjects []*RouteAdvertisementBuilder + routeAdvertisementObjects := make([]*RouteAdvertisementBuilder, 0, len(routeAdvertisementList.Items))Also applies to: 40-46, 53-66
pkg/ovn/routeadvertisementlist_test.go (2)
72-95: Optional: run subtests in parallelThese tests are independent; parallelizing speeds up CI.
- t.Run(testCase.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel()
98-108: Remove unused helper to keep tests leanbuildRouteAdvertisementWithName isn’t referenced.
-func buildRouteAdvertisementWithName(name string) *ovnv1.RouteAdvertisement { - return &ovnv1.RouteAdvertisement{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: defaultRouteAdvertisementNamespace, - }, - Spec: ovnv1.RouteAdvertisementSpec{ - Advertisements: defaultAdvertisements, - }, - } -}pkg/ovn/routeadvertisement_test.go (2)
20-117: Optional: mark tests parallel to shorten runtimeSafe to parallelize at subtest level; each uses isolated fake clients.
Example change (apply to each test function body):
func TestNewRouteAdvertisementBuilder(t *testing.T) { + t.Parallel()Also applies to: 119-164, 166-205, 207-232, 234-277, 279-325, 327-342, 344-364, 366-423
98-106: Inline dummy fixture; remove redundant helperSimplify by inlining the single-object slice.
- testSettings = clients.GetTestClients(clients.TestClientParams{ - K8sMockObjects: buildDummyRouteAdvertisement(), - }) + testSettings = clients.GetTestClients(clients.TestClientParams{ + K8sMockObjects: []runtime.Object{buildValidRouteAdvertisement()}, + })And drop buildDummyRouteAdvertisement():
-func buildDummyRouteAdvertisement() []runtime.Object { - return append([]runtime.Object{}, buildValidRouteAdvertisement()) -}Also applies to: 446-448
pkg/ovn/routeadvertisement.go (5)
125-131: Clarify Get() logging for non-NotFound errors.Current log claims “does not exist” for any error.
- if err != nil { - glog.V(100).Infof( - "RouteAdvertisement object %s does not exist in namespace %s", - builder.Definition.Name, builder.Definition.Namespace) - return nil, err - } + if err != nil { + if k8serrors.IsNotFound(err) { + glog.V(100).Infof("RouteAdvertisement %s not found in namespace %s", + builder.Definition.Name, builder.Definition.Namespace) + } else { + glog.V(100).Infof("Failed to get RouteAdvertisement %s/%s: %v", + builder.Definition.Namespace, builder.Definition.Name, err) + } + return nil, err + }
209-221: Populate Object from the API after create/exist.Ensures
Objectreflects server-defaulted fields (UID, RV).if !builder.Exists() { err := builder.apiClient.Create(context.TODO(), builder.Definition) if err != nil { glog.V(100).Infof("Failed to create RouteAdvertisement") return nil, err } } - - builder.Object = builder.Definition - - return builder, nil + obj, err := builder.Get() + if err != nil { + return nil, err + } + builder.Object = obj + return builder, nil
261-266: Set ResourceVersion before Update to avoid conflicts.Updating a fresh Definition without RV often fails.
- err := builder.apiClient.Update(context.TODO(), builder.Definition) + // Ensure ResourceVersion present for optimistic concurrency + if builder.Definition.ResourceVersion == "" { + current := &ovnv1.RouteAdvertisement{} + if err := builder.apiClient.Get( + context.TODO(), + runtimeClient.ObjectKey{ + Name: builder.Definition.Name, Namespace: builder.Definition.Namespace, + }, + current, + ); err != nil { + return nil, err + } + builder.Definition.ResourceVersion = current.ResourceVersion + } + err := builder.apiClient.Update(context.TODO(), builder.Definition)
36-41: Normalize NewRouteAdvertisementBuilder error handling (return a builder with errorMsg instead of nil).Keeps the fluent/builder pattern consistent with
validate().if apiClient == nil { glog.V(100).Infof("The apiClient cannot be nil") - - return nil + return &RouteAdvertisementBuilder{errorMsg: "apiClient cannot be nil"} } err := apiClient.AttachScheme(ovnv1.AddToScheme) if err != nil { glog.V(100).Infof("Failed to add ovn scheme to client schemes") - - return nil + return &RouteAdvertisementBuilder{ + apiClient: apiClient.Client, + errorMsg: fmt.Sprintf("failed to attach ovn scheme: %v", err), + } }Also applies to: 42-47
319-331: WithOptions: avoid shadowing receiver and log the error.Minor readability; preserves behavior while making the flow explicit.
- for _, option := range options { - if option != nil { - builder, err := option(builder) - - if err != nil { - glog.V(100).Infof("Error occurred in mutation function") - - builder.errorMsg = err.Error() - - return builder - } - } - } + for _, option := range options { + if option != nil { + b, err := option(builder) + if err != nil { + glog.V(100).Infof("Error occurred in mutation function: %v", err) + builder.errorMsg = err.Error() + return builder + } + if b != nil { + builder = b + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
go.mod(1 hunks)internal/sync/configs/ovn-k-config.yaml(1 hunks)pkg/ovn/routeadvertisement.go(1 hunks)pkg/ovn/routeadvertisement_test.go(1 hunks)pkg/ovn/routeadvertisementlist.go(1 hunks)pkg/ovn/routeadvertisementlist_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/ovn/routeadvertisementlist.go (1)
pkg/ovn/routeadvertisement.go (1)
RouteAdvertisementBuilder(19-24)
pkg/ovn/routeadvertisement_test.go (2)
pkg/clients/clients.go (2)
GetTestClients(212-217)TestClientParams(203-209)pkg/ovn/routeadvertisement.go (3)
NewRouteAdvertisementBuilder(30-108)RouteAdvertisementBuilder(19-24)Pull(153-198)
pkg/ovn/routeadvertisementlist_test.go (2)
pkg/clients/clients.go (2)
GetTestClients(212-217)TestClientParams(203-209)pkg/ovn/routeadvertisementlist.go (1)
ListRouteAdvertisements(14-67)
pkg/ovn/routeadvertisement.go (2)
vendor/k8s.io/klog/klog.go (2)
V(1127-1154)Infof(1200-1202)pkg/msg/msg.go (3)
FailToUpdateNotification(11-18)FailToUpdateError(21-28)UndefinedCrdObjectErrString(6-8)
🪛 GitHub Actions: eco-gotests integration
go.mod
[error] 42-42: go.mod:42: require github.com/ovn-org/ovn-kubernetes/go-controller: version "latest" invalid: must be of the form v1.2.3
🪛 GitHub Actions: Test Incoming Changes
go.mod
[error] 1-1: Inconsistent vendoring detected: a module is explicitly required in go.mod but not marked explicit in vendor/modules.txt (github.com/ovn-org/ovn-kubernetes/[email protected]).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-k8s (ubuntu-22.04)
- GitHub Check: integration-k8s (ubuntu-22.04-arm)
🔇 Additional comments (9)
internal/sync/configs/ovn-k-config.yaml (2)
1-12: LGTM: OVN types sync entry is well-scopedConfig looks correct; excludes are sensible.
13-27: LGTM: RouteAdvertisement sync wiring and import replacements look correctImport replacements match the new local scheme paths; excludes align with expectations.
pkg/ovn/routeadvertisementlist.go (1)
55-64: Good: avoids the address-of-loop-variable pitfallCopying the item before taking its address prevents the common range-variable pointer bug.
pkg/ovn/routeadvertisementlist_test.go (1)
14-69: LGTM: table-driven coverage is clear and hits key paths (nil client, multi-options error, etc.)pkg/ovn/routeadvertisement_test.go (1)
20-92: Nice: validation scenarios cover empty fields, too many and duplicate adsGood negative coverage for builder validation.
pkg/ovn/routeadvertisement.go (4)
94-105: LGTM: duplicate advertisement detection.Simple, correct uniqueness check before building the spec.
336-341: LGTM: GVR helper.Correct group/version/resource for cleanup utilities.
345-373: LGTM: validate().Covers all invariants: builder nil, Definition nil, apiClient nil, and prior error state.
86-92: Verify CRD schema enforces maxItems=2
I didn’t find anykubebuilder:validationormaxItemsconstraint on theadvertisementsfield in the CRD Go types or YAML—please confirm the spec enforces at most two entries. If not, remove or adjust this hard cap in the builder to avoid rejecting valid specs.
pkg/ovn/routeadvertisement.go
Outdated
| var err error | ||
| builder.Object, err = builder.Get() | ||
|
|
||
| return err == nil || !k8serrors.IsNotFound(err) | ||
| } |
There was a problem hiding this comment.
Fix Exists(): it returns true for non-NotFound errors.
This can incorrectly treat transient/auth errors as “exists”.
Apply:
var err error
builder.Object, err = builder.Get()
-
- return err == nil || !k8serrors.IsNotFound(err)
+ if err != nil {
+ builder.Object = nil
+ if k8serrors.IsNotFound(err) {
+ return false
+ }
+ glog.V(100).Infof("Error checking RouteAdvertisement existence: %v", err)
+ return false
+ }
+ return true🤖 Prompt for AI Agents
In pkg/ovn/routeadvertisement.go around lines 146 to 150, Exists() currently
returns true for any error that is not IsNotFound, which falsely treats other
errors as “exists”; change the final return to only return true when no error
occurred by replacing the current return with "return err == nil" (i.e., if
Get() returns an error, including transient/auth errors, treat the object as not
existing).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
go.mod (1)
42-42: Fixed: pinned go-controller to a concrete pseudo-version (resolves “latest” CI break).Good correction; this should unblock module resolution and vendor. Consider running go mod tidy && go mod vendor to refresh artifacts.
🧹 Nitpick comments (3)
go.mod (3)
215-215: Direct dependency on structured-merge-diff/v4 looks fine.No action; just ensure it’s directly imported somewhere; otherwise mark it indirect or let tidy move it.
3-3: go directive: use “go 1.24” and (optionally) add a toolchain directive for the patch.The go directive should typically be major.minor. If you need the patch, add a toolchain pin.
Apply:
-go 1.24.5 +go 1.24 +toolchain go1.24.5
119-119: Use the latest stable gorilla/websocket tag
Verified viago list -m -versions -mod=mod github.com/gorilla/websocketthat v1.5.3 is the newest stable release—replace the pseudo-version to avoid churn:- github.com/gorilla/websocket v1.5.4-0.20250319132907-e064f32e3674 // indirect + github.com/gorilla/websocket v1.5.3 // indirect
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
go.mod(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration-k8s (ubuntu-22.04-arm)
- GitHub Check: integration-k8s (ubuntu-22.04)
- GitHub Check: build
| @@ -66,7 +66,7 @@ | |||
| open-cluster-management.io/governance-policy-propagator v0.16.0 | |||
| open-cluster-management.io/multicloud-operators-subscription v0.15.0 | |||
| sigs.k8s.io/container-object-storage-interface-api v0.1.0 | |||
| sigs.k8s.io/controller-runtime v0.20.4 | |||
| sigs.k8s.io/controller-runtime v0.21.0 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace overrides controller-runtime back to v0.19.7; remove or bump to v0.21.0 to match require.
As-is, the build will pull 0.19.7 despite require v0.21.0. That defeats the upgrade and can break API expectations.
Apply one of:
- sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.19.7
+ # sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.21.0or simply delete the override line if no fork/pin is needed.
🤖 Prompt for AI Agents
In go.mod around line 69, the go.mod contains an override pinning
sigs.k8s.io/controller-runtime to v0.19.7 which conflicts with the require for
v0.21.0; update the override to sigs.k8s.io/controller-runtime v0.21.0 to match
the require or remove the replace/override line entirely if no pin is needed,
then run `go mod tidy` to ensure modules are consistent.
e076f41 to
5345226
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (3)
go.mod (2)
69-69: Override forces controller-runtime back to v0.19.7; it must match require v0.21.0.Your require is v0.21.0 but replace pins v0.19.7, defeating the upgrade and risking API mismatches. Remove or bump the override.
- sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.19.7 + # sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.21.0Prefer deleting the line entirely unless a pin is strictly required.
56-58: Align Kubernetes client modules to v0.33.3 and drop client-go overrideIn go.mod:
- Update require block:
- k8s.io/client-go v12.0.0+incompatible + k8s.io/client-go v0.33.3 - k8s.io/kubectl v0.32.6 - k8s.io/kubelet v0.32.6 + k8s.io/kubectl v0.33.3 + k8s.io/kubelet v0.33.3- Update indirect dependency:
- k8s.io/cli-runtime v0.32.6 // indirect + k8s.io/cli-runtime v0.33.3 // indirect- Remove replace directive:
- k8s.io/client-go => k8s.io/client-go v0.32.6pkg/ovn/routeadvertisement.go (1)
190-203: Fix Exists(): it returns true for non-NotFound errors.Currently treats transient/auth errors as “exists”.
Apply:
var err error builder.Object, err = builder.Get() - return err == nil || !errors.IsNotFound(err) + if err != nil { + builder.Object = nil + if errors.IsNotFound(err) { + return false + } + glog.V(100).Infof("Error checking RouteAdvertisement existence: %v", err) + return false + } + return true
🧹 Nitpick comments (19)
go.mod (1)
119-119: Verify pseudo-version for gorilla/websocket.You’re on a commit pseudo-version. If there’s no specific reason, prefer the latest tagged release to reduce churn. If this commit is required (bugfix), keep it—just confirm no unexpected transitive pulls.
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go (1)
201-225: Fix misleading example in comment block (non-blocking but confusing).The sample uses context.Background() as if it returns a cancel func and refers to WaitForStop(), which this factory does not expose.
Update the comment snippet:
-// ctx, cancel := context.Background() -// defer cancel() +// ctx, cancel := context.WithCancel(context.Background()) +// defer cancel() // factory := NewSharedInformerFactory(client, resyncPeriod) -// defer factory.WaitForStop() // Returns immediately if nothing was started. +// defer factory.Shutdown() // Blocks until started informer goroutines terminate.pkg/schemes/ovn/types/doc.go (1)
1-1: Fix typo in package comment.Use “across” and end with a period.
-// Package types contains shared types accrorss API Schema definitions +// Package types contains shared types across API schema definitions.pkg/schemes/ovn/routeadvertisement/v1/doc.go (1)
2-2: Remove stray placeholder comment.The “// group” line is a leftover and can be dropped.
-// groupinternal/sync/configs/ovn-k-config.yaml (3)
17-18: Optional: narrow the remote_api_directory to v1To reduce noise and future diffs, consider syncing only v1.
- remote_api_directory: go-controller/pkg/crd/routeadvertisements + remote_api_directory: go-controller/pkg/crd/routeadvertisements/v1 - local_api_directory: schemes/ovn/routeadvertisement + local_api_directory: schemes/ovn/routeadvertisement/v1Note: if you apply this, adjust replace_imports accordingly (old/new both end with /v1).
28-28: Add trailing newlineYAMLlint flagged missing newline at EOF.
4-5: Pin upstream to a commit or tag for reproducible syncsTracking a moving master can cause nondeterministic diffs.
Suggest using a specific commit SHA and periodically bumping it in a controlled PR.
pkg/ovn/routeadvertisement_test.go (1)
32-101: Minor: unify assertion style.Tests mix t.Errorf and testify/assert. Prefer assert/require uniformly for consistency.
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/clientset_generated.go (1)
21-25: Unify import paths to avoid upstream/local drift.Same concern as in the real clientset: these fake clients import upstream packages while local equivalents exist in this repo. Please align to either fully-upstream or fully-local to prevent package/type divergence.
Proposed change (if using local packages):
-import ( - applyconfiguration "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration" - clientset "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned" - k8sv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1" - fakek8sv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake" +import ( + applyconfiguration "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration" + clientset "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned" + k8sv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1" + fakek8sv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake"pkg/schemes/ovn/types/networkselector.go (2)
72-76: Typo: 'mot' ➜ 'not'.User-facing CRD docs surface these comments via kubebuilder; fix the typo.
-// selector will mot match anything, while an empty ({}) selector will match +// selector will not match anything, while an empty ({}) selector will match
81-86: Typos/grammar: make sentence clear.Use “selects” and fix “servind”.
-// namespaceSelector select the primary UserDefinedNetworks that are servind -// the selected namespaces. This field follows standard label selector +// namespaceSelector selects the primary UserDefinedNetworks that are serving +// the selected namespaces. This field follows standard label selectorpkg/ovn/routeadvertisement.go (8)
68-85: Add validation for required fields (nodeSelector, frrConfigurationSelector, networkSelectors).Spec marks these as required; catching empties early improves UX.
Apply:
if len(advertisements) == 0 { glog.V(100).Infof("RouteAdvertisement 'advertisements' cannot be empty") builder.errorMsg = "RouteAdvertisement 'advertisements' cannot be empty" return builder } + // Validate required selectors and network selectors + if isEmptyLabelSelector(nodeSelector) { + glog.V(100).Infof("RouteAdvertisement 'nodeSelector' cannot be empty") + builder.errorMsg = "RouteAdvertisement 'nodeSelector' cannot be empty" + return builder + } + if isEmptyLabelSelector(frrConfigurationSelector) { + glog.V(100).Infof("RouteAdvertisement 'frrConfigurationSelector' cannot be empty") + builder.errorMsg = "RouteAdvertisement 'frrConfigurationSelector' cannot be empty" + return builder + } + if len(networkSelectors) == 0 { + glog.V(100).Infof("RouteAdvertisement 'networkSelectors' cannot be empty") + builder.errorMsg = "RouteAdvertisement 'networkSelectors' cannot be empty" + return builder + }Add helper (outside the shown range):
// isEmptyLabelSelector returns true when selector has no match labels/expressions. func isEmptyLabelSelector(sel metav1.LabelSelector) bool { return len(sel.MatchLabels) == 0 && len(sel.MatchExpressions) == 0 }
129-141: Improve error logging in Get(): distinguish NotFound from other errors.Avoid logging “does not exist” for transient/auth errors.
Apply:
- if err != nil { - glog.V(100).Infof("RouteAdvertisement object %s does not exist", builder.Definition.Name) - - return nil, err - } + if err != nil { + if errors.IsNotFound(err) { + glog.V(100).Infof("RouteAdvertisement object %s does not exist", builder.Definition.Name) + } else { + glog.V(100).Infof("Failed to get RouteAdvertisement %s: %v", builder.Definition.Name, err) + } + return nil, err + }
151-160: Make Create() idempotent without pre-check to avoid TOCTOU.Create directly; on AlreadyExists, Get the object; after successful Create, optionally Get to hydrate server defaults.
Apply:
- var err error - if !builder.Exists() { - err = builder.apiClient.Create(context.TODO(), builder.Definition) - if err == nil { - builder.Object = builder.Definition - } - } - - return builder, err + // Try to create and handle AlreadyExists to avoid race between Exists() and Create(). + if err := builder.apiClient.Create(context.TODO(), builder.Definition); err != nil { + if errors.IsAlreadyExists(err) { + obj, getErr := builder.Get() + if getErr != nil { + return builder, getErr + } + builder.Object = obj + return builder, nil + } + return builder, err + } + // Optionally refresh to capture server-populated fields. + if obj, getErr := builder.Get(); getErr == nil { + builder.Object = obj + } else { + // Fall back to Definition if Get fails. + builder.Object = builder.Definition + } + return builder, nil
210-224: Ensure ResourceVersion before Update() to avoid conflicts.If Definition lacks ResourceVersion, fetch current first.
Apply:
- err := builder.apiClient.Update(context.TODO(), builder.Definition) + // Ensure ResourceVersion is set + if builder.Definition.ResourceVersion == "" { + current, getErr := builder.Get() + if getErr != nil { + glog.V(100).Infof(msg.FailToUpdateNotification("RouteAdvertisement", builder.Definition.Name)) + return nil, getErr + } + builder.Definition.ResourceVersion = current.ResourceVersion + } + err := builder.apiClient.Update(context.TODO(), builder.Definition)
239-258: Validate advertisements: enforce MaxItems=2 and uniqueness.Aligns with CRD schema and prevents API rejections.
Apply:
if len(advertisements) == 0 { glog.V(100).Infof("RouteAdvertisement 'advertisements' cannot be empty") builder.errorMsg = "RouteAdvertisement 'advertisements' cannot be empty" return builder } + if len(advertisements) > 2 { + glog.V(100).Infof("RouteAdvertisement 'advertisements' cannot have more than 2 items") + builder.errorMsg = "RouteAdvertisement 'advertisements' cannot have more than 2 items" + return builder + } + seen := map[ovnv1.AdvertisementType]struct{}{} + for _, a := range advertisements { + if _, ok := seen[a]; ok { + glog.V(100).Infof("RouteAdvertisement 'advertisements' must be unique") + builder.errorMsg = "RouteAdvertisement 'advertisements' must be unique" + return builder + } + seen[a] = struct{}{} + }
260-271: Validate nodeSelector is non-empty.Apply:
glog.V(100).Infof("Setting RouteAdvertisement %s nodeSelector to %v", builder.Definition.Name, nodeSelector) + if isEmptyLabelSelector(nodeSelector) { + glog.V(100).Infof("RouteAdvertisement 'nodeSelector' cannot be empty") + builder.errorMsg = "RouteAdvertisement 'nodeSelector' cannot be empty" + return builder + } builder.Definition.Spec.NodeSelector = nodeSelector
273-284: Validate frrConfigurationSelector is non-empty.Apply:
glog.V(100).Infof("Setting RouteAdvertisement %s frrConfigurationSelector to %v", builder.Definition.Name, frrConfigurationSelector) + if isEmptyLabelSelector(frrConfigurationSelector) { + glog.V(100).Infof("RouteAdvertisement 'frrConfigurationSelector' cannot be empty") + builder.errorMsg = "RouteAdvertisement 'frrConfigurationSelector' cannot be empty" + return builder + } builder.Definition.Spec.FRRConfigurationSelector = frrConfigurationSelector
286-297: Validate networkSelectors is non-empty.Apply:
glog.V(100).Infof("Setting RouteAdvertisement %s networkSelectors to %v", builder.Definition.Name, networkSelectors) + if len(networkSelectors) == 0 { + glog.V(100).Infof("RouteAdvertisement 'networkSelectors' cannot be empty") + builder.errorMsg = "RouteAdvertisement 'networkSelectors' cannot be empty" + return builder + } builder.Definition.Spec.NetworkSelectors = networkSelectors
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (39)
go.mod(6 hunks)internal/sync/configs/ovn-k-config.yaml(1 hunks)internal/sync/configs/ovn-only-config.yaml(1 hunks)pkg/ovn/routeadvertisement.go(1 hunks)pkg/ovn/routeadvertisement_test.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/internal/internal.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/advertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsspec.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsstatus.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/utils.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/clientset_generated.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/register.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements_client.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/generated_expansion.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements_client.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/generic.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/internalinterfaces/factory_interfaces.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/expansion_generated.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/register.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/types.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go(1 hunks)pkg/schemes/ovn/types/doc.go(1 hunks)pkg/schemes/ovn/types/networkselector.go(1 hunks)pkg/schemes/ovn/types/zz_generated.deepcopy.go(1 hunks)
✅ Files skipped from review due to trivial changes (13)
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/doc.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/doc.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/doc.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/generated_expansion.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements_client.go
- pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/doc.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements_client.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/advertisements.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/utils.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/routeadvertisements.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsspec.go
🧰 Additional context used
🧬 Code graph analysis (17)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/internalinterfaces/factory_interfaces.go (3)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go (1)
Interface(30-33)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go (1)
Interface(26-29)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go (1)
Interface(25-28)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go (3)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go (2)
Interface(25-28)New(37-39)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go (1)
SharedInformerFactory(225-257)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/internalinterfaces/factory_interfaces.go (2)
SharedInformerFactory(33-36)TweakListOptionsFunc(39-39)
pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/routeadvertisements.go (3)
pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
RouteAdvertisements(19-25)pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/expansion_generated.go (1)
RouteAdvertisementsListerExpansion(22-22)pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
Resource(22-24)
pkg/schemes/ovn/types/zz_generated.deepcopy.go (1)
pkg/schemes/ovn/types/networkselector.go (6)
ClusterUserDefinedNetworkSelector(71-77)NetworkSelector(21-47)NetworkAttachmentDefinitionSelector(104-116)PrimaryUserDefinedNetworkSelector(80-86)SecondaryUserDefinedNetworkSelector(89-101)NetworkSelectors(14-14)
pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go (4)
vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/objectmeta.go (1)
ObjectMetaApplyConfiguration(11-18)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsspec.go (1)
RouteAdvertisementsSpecApplyConfiguration(28-34)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsstatus.go (1)
RouteAdvertisementsStatusApplyConfiguration(26-29)pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
RouteAdvertisements(19-25)
pkg/schemes/ovn/routeadvertisement/v1/register.go (4)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/register.go (1)
AddToScheme(50-50)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go (2)
AddToScheme(50-50)Scheme(29-29)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go (1)
RouteAdvertisements(37-43)pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
RouteAdvertisements(19-25)RouteAdvertisementsList(93-97)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go (2)
pkg/schemes/ovn/routeadvertisement/v1/register.go (2)
SchemeBuilder(12-12)AddToScheme(13-13)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/register.go (1)
AddToScheme(50-50)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/register.go (2)
pkg/schemes/ovn/routeadvertisement/v1/register.go (2)
SchemeBuilder(12-12)AddToScheme(13-13)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go (1)
AddToScheme(50-50)
pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsstatus.go (1)
pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
RouteAdvertisementsStatus(74-88)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/generic.go (2)
pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
SchemeGroupVersion(11-11)pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
RouteAdvertisements(19-25)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/clientset_generated.go (4)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go (2)
Clientset(36-39)Interface(30-33)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/utils.go (1)
NewTypeConverter(45-47)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements_client.go (1)
K8sV1Interface(28-31)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements_client.go (1)
FakeK8sV1(26-28)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go (1)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements_client.go (6)
K8sV1Interface(28-31)K8sV1Client(34-36)NewForConfig(45-55)NewForConfigAndClient(59-69)NewForConfigOrDie(73-79)New(82-84)
pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements.go (5)
pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go (2)
RouteAdvertisements(37-43)RouteAdvertisementsApplyConfiguration(28-33)pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
RouteAdvertisements(19-25)RouteAdvertisementsList(93-97)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements_client.go (1)
FakeK8sV1(26-28)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go (1)
RouteAdvertisementsInterface(39-54)pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
SchemeGroupVersion(11-11)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go (7)
pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go (2)
RouteAdvertisements(37-43)RouteAdvertisementsApplyConfiguration(28-33)pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
RouteAdvertisements(19-25)RouteAdvertisementsList(93-97)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go (1)
Interface(30-33)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go (1)
Interface(25-28)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/generated_expansion.go (1)
RouteAdvertisementsExpansion(20-20)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements_client.go (1)
K8sV1Client(34-36)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go (1)
ParameterCodec(31-31)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go (5)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go (2)
Interface(30-33)New(113-119)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go (2)
Interface(26-29)New(38-40)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go (2)
Interface(25-28)New(37-39)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/internalinterfaces/factory_interfaces.go (3)
TweakListOptionsFunc(39-39)SharedInformerFactory(33-36)NewInformerFunc(30-30)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/generic.go (1)
GenericInformer(30-33)
pkg/ovn/routeadvertisement.go (4)
pkg/schemes/ovn/routeadvertisement/v1/types.go (3)
RouteAdvertisements(19-25)AdvertisementType(61-61)RouteAdvertisementsSpec(30-57)pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)pkg/schemes/ovn/routeadvertisement/v1/register.go (2)
GroupName(10-10)SchemeGroupVersion(11-11)pkg/msg/msg.go (2)
FailToUpdateNotification(11-18)UndefinedCrdObjectErrString(6-8)
🪛 YAMLlint (1.37.1)
internal/sync/configs/ovn-k-config.yaml
[error] 28-28: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration-k8s (ubuntu-22.04)
- GitHub Check: integration-k8s (ubuntu-22.04-arm)
- GitHub Check: build
🔇 Additional comments (27)
go.mod (1)
42-42: Pinned ovn-kubernetes/go-controller looks good.Thanks for replacing “latest” with a concrete pseudo-version; this unblocks CI.
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go (1)
25-45: LGTM: version gateway and constructor mirror informer-gen patterns.Interface/V1 accessor and New(..) wiring look correct and consistent with the v1 subpackage.
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/internalinterfaces/factory_interfaces.go (1)
29-36: LGTM: internal contracts match client-go expectations.NewInformerFunc signature and SharedInformerFactory methods align with generated informer usage.
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/generic.go (1)
53-61: LGTM: resource→informer mapping is correct.GVR mapping for k8s.ovn.org/v1 routeadvertisements to the typed informer looks good and follows informer-gen style.
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go (1)
123-145: LGTM: Start() lifecycle and goroutine handling are solid.Locking, startedInformers tracking, and wg usage follow client-go conventions.
pkg/schemes/ovn/routeadvertisement/v1/doc.go (1)
4-5: API group annotation is consistent with register.go and upstream CRDs.pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/expansion_generated.go (1)
20-22: LGTM — standard lister expansion hook.Generated stub looks correct and ready for custom extensions.
pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/internal/internal.go (1)
27-36: LGTM — lazy parser initialization is correct.sync.Once usage and failure panic mirror upstream generator patterns.
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go (1)
61-75: LGTM: generic client wiring and resource nameThe resource string "routeadvertisements" and generic list/apply wiring are correct given a cluster-scoped CRD.
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements.go (1)
33-51: LGTM: fake client wiringResource/kind, list meta copy, and pointer slice conversions look correct.
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go (1)
29-55: LGTM: scheme, codecs, and init registrationStandard client-gen boilerplate; fine once imports point to local v1.
pkg/schemes/ovn/routeadvertisement/v1/register.go (2)
9-14: CRD registration wiring looks correct.Group/version, SchemeBuilder, Kind/Resource helpers, and known types registration are consistent with the v1 RouteAdvertisements API.
Also applies to: 26-34
10-14: No upstream RouteAdvertisements types referenced; ignore this verification.A scan of the repository shows no imports of
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"
—all RouteAdvertisement listers and clients use the local
"pkg/schemes/ovn/routeadvertisement/v1" types. There’s no mixing of local vs. upstream types, so no GVK mismatch risk.Likely an incorrect or invalid review comment.
pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsstatus.go (1)
24-56: LGTM: generated apply-configuration matches Status shape.Status pointer and Conditions builder align with types.go; panic-on-nil for WithConditions is standard codegen.
pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/routeadvertisements.go (2)
20-47: Typed lister implementation looks good.Indexer wiring and resource name "routeadvertisements" match expectations.
21-26: Confirm client-go v0.32.6 supports typed listers
go.mod’s replace directive pins k8s.io/client-go to v0.32.6, which includes ResourceIndexer[T] and listers.New[T] generics (pkg.go.dev)pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
28-56: Validation rules read well; no duplicates and PodNetwork/nodeSelector mutex.CEL rules achieve the intended constraints; Required/Min/Max annotations are coherent.
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go (2)
54-86: LGTM: clientset wiring, rate limiter, discovery.Constructor flow, rate-limiter defaults, and discovery wiring match client-go patterns.
Also applies to: 95-101, 112-119
24-28: Align routeadvertisements client import source
Current code imports typed clients fromgithub.1485827954.workers.dev/ovn-org/ovn-kubernetes/.../routeadvertisements/v1while local copies exist underpkg/schemes/ovn/routeadvertisement/v1. Mixing upstream and local packages risks subtle type mismatches. Decide on one source: either update all imports to the local module path (github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/...) or remove the local copies and import upstream consistently.pkg/schemes/ovn/types/zz_generated.deepcopy.go (1)
23-114: LGTM: deepcopy coverage is complete and correct.Covers all selector structs and the slice type; deep-copies nested LabelSelectors and pointer fields properly.
Also applies to: 116-149
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/clientset_generated.go (1)
81-111: LGTM: fake clientset behavior and reactors.Object trackers, watch reactors, and interface assertions look standard.
Also applies to: 118-121
pkg/schemes/ovn/types/networkselector.go (1)
7-15: Sanity-check max items = 5.MaxItems=5 may be product-driven. If not intentional, consider documenting the rationale or relaxing it.
pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go (1)
35-43: LGTM: apply-configuration scaffolding is consistent.Kind/APIVersion initialization, meta helpers, Spec/Status setters, and GetName() follow applyconfig patterns.
Also applies to: 203-223
pkg/ovn/routeadvertisement.go (4)
49-66: TypeMeta/APIVersion and Kind look correct.Using ovnv1.GroupName and ovnv1.SchemeGroupVersion.Version to compose APIVersion and Kind=RouteAdvertisements is appropriate.
299-304: GVR looks correct.Plural “routeadvertisements” matches Kind RouteAdvertisements and group/version from ovnv1.
306-336: Validation helper is solid and consistent with repo style.Covers nil builder/definition, nil apiClient, and prior errorMsg.
87-120: No action needed: RouteAdvertisements is cluster-scoped.Confirmed via the
+kubebuilder:resource…scope=Clusterannotation in pkg/schemes/ovn/routeadvertisement/v1/types.go, so using onlyNameinPullRouteAdvertisementis correct.
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"' | ||
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"' | ||
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/types"' | ||
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types"' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix replace_imports: wrong path (singular vs plural) and missing /v1 suffix break import rewriting
The upstream package path is plural and your local import should include /v1. As-is, the generator won’t rewrite imports, leaving generated code pointing at upstream types/scheme.
Apply this diff and re-run the sync/generators:
- replace_imports:
- - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"'
- new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"'
+ replace_imports:
+ - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"'
+ new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"'Also mirror this fix anywhere else (e.g., ovn-only-config.yaml) to keep configs consistent.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Upstream imports still present:"
rg -nP 'github\.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements?/v1' pkg/schemes/ovn/routeadvertisement -g '!**/vendor/**' || true
echo "Local imports present:"
rg -nP 'github\.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement(?:/v1)?' pkg/schemes/ovn/routeadvertisement -g '!**/vendor/**' || true
echo "Check this config file uses plural + /v1 mapping:"
rg -n 'routeadvertisement' internal/sync/configs/ovn-*config.yaml || trueLength of output: 9397
Fix replace_imports entry to match upstream plural path and include /v1 suffix
Update both internal/sync/configs/ovn-k-config.yaml and ovn-only-config.yaml:
- replace_imports:
- - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"'
- new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"'
+ replace_imports:
+ - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"'
+ new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"'Then re-run the sync generators.
📝 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.
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"' | |
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"' | |
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/types"' | |
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types"' | |
| replace_imports: | |
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"' | |
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"' | |
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/types"' | |
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types"' |
🤖 Prompt for AI Agents
In internal/sync/configs/ovn-k-config.yaml around lines 20 to 23, the
replace_imports entries use singular paths and omit the /v1 suffix; update both
entries to use the upstream plural package path and append the /v1 suffix (e.g.,
change the routeadvertisement path to the plural package path with /v1 and
change the types path similarly), apply the same changes to
ovn-only-config.yaml, then re-run the sync generators to propagate the corrected
imports.
| - name: ovn-kubernetes-routeadvertisement | ||
| sync: true | ||
| repo_link: "https://github.com/ovn-org/ovn-kubernetes" | ||
| branch: main | ||
| remote_api_directory: go-controller/pkg/crd/routeadvertisement/v1 | ||
| local_api_directory: schemes/ovn/routeadvertisement |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify remote path and ensure all sync configs use the same mapping.
Cross-check this file (and ovn-k-config.yaml) for consistent use of routeadvertisement/v1.
Also applies to: 9-16
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Local replace_imports across all sync configs:"
rg -nC1 'replace_imports:|routeadvertisement' internal/sync/configs -g '!**/vendor/**'
echo -e "\nCheck for any mappings missing /v1 in eco-goinfra path:"
rg -nP 'new:\s*\"\S*eco-goinfra/pkg/schemes/ovn/routeadvertisement\"(?!/v1\")' internal/sync/configs || echo "OK: no missing /v1 mappings"
echo -e "\nValidate existence of local target directory:"
test -d pkg/schemes/ovn/routeadvertisement/v1 && echo "OK: pkg/schemes/ovn/routeadvertisement/v1 exists" || echo "MISSING: pkg/schemes/ovn/routeadvertisement/v1"
echo -e "\nValidate upstream directory exists (shallow clone):"
tmpdir="$(mktemp -d)"; trap 'rm -rf "$tmpdir"' EXIT
git clone --depth 1 https://github.com/ovn-org/ovn-kubernetes "$tmpdir/ovn-k"
ls -ld "$tmpdir/ovn-k/go-controller/pkg/crd/routeadvertisement/v1" || {
echo "NOTE: If upstream uses 'routeadvertisements' (plural), update remote_api_directory accordingly."
ls -ld "$tmpdir/ovn-k/go-controller/pkg/crd/routeadvertisements" || true
}Length of output: 12623
Update remote_api_directory to use the plural “routeadvertisements”
In internal/sync/configs/ovn-only-config.yaml (and mirror in ovn-k-config.yaml), change
remote_api_directory: go-controller/pkg/crd/routeadvertisement/v1to
remote_api_directory: go-controller/pkg/crd/routeadvertisements[/v1]and adjust the replace_imports entries accordingly to "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1", matching the upstream directory.
🤖 Prompt for AI Agents
In internal/sync/configs/ovn-only-config.yaml around lines 2-7 (and the mirror
file ovn-k-config.yaml), update the remote_api_directory value from the singular
"go-controller/pkg/crd/routeadvertisement/v1" to the plural path
"go-controller/pkg/crd/routeadvertisements/v1" (or
"go-controller/pkg/crd/routeadvertisements" if your tooling expects no version
suffix), and update any replace_imports entries to use the matching upstream
module path
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"
so imports and generated API paths match the upstream directory layout.
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"' | ||
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"' | ||
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/types"' | ||
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types"' |
There was a problem hiding this comment.
Fix replace_imports target path: missing /v1 breaks imports.
Upstream imports “.../routeadvertisement/v1”. Mapping to “.../routeadvertisement” will fail to resolve the v1 package. Add “/v1”.
- - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"'
- new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"'
+ - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"'
+ new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"'📝 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.
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"' | |
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"' | |
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/types"' | |
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types"' | |
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"' | |
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"' | |
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/types"' | |
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types"' |
🤖 Prompt for AI Agents
In internal/sync/configs/ovn-only-config.yaml around lines 9 to 12, the mapping
for the upstream import
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"
was changed to
"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"
which omits the required "/v1" segment and will break imports; update that new
path to include "/v1" (i.e., ".../routeadvertisement/v1") so it matches the
upstream package path while leaving the types mapping as-is.
| func TestRouteAdvertisementGet(t *testing.T) { | ||
| testCases := []struct { | ||
| routeAdvertisement *ovnv1.RouteAdvertisements | ||
| expectedError bool | ||
| }{ | ||
| { | ||
| routeAdvertisement: buildDummyRouteAdvertisement(defaultRouteAdvertisementName), | ||
| expectedError: false, | ||
| }, | ||
| { | ||
| routeAdvertisement: buildDummyRouteAdvertisement(""), | ||
| expectedError: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, testCase := range testCases { | ||
| var ( | ||
| runtimeObjects []runtime.Object | ||
| ) | ||
|
|
||
| if testCase.routeAdvertisement != nil { | ||
| runtimeObjects = append(runtimeObjects, testCase.routeAdvertisement) | ||
| } | ||
|
|
||
| testSettings := clients.GetTestClients(clients.TestClientParams{ | ||
| K8sMockObjects: runtimeObjects, | ||
| SchemeAttachers: []clients.SchemeAttacher{ | ||
| ovnv1.AddToScheme, | ||
| }, | ||
| }) | ||
|
|
||
| routeAdvertisementBuilder, err := PullRouteAdvertisement(testSettings.Client, testCase.routeAdvertisement.Name) | ||
|
|
||
| if testCase.expectedError { | ||
| assert.NotNil(t, err) | ||
| } else { | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, testCase.routeAdvertisement.Name, routeAdvertisementBuilder.Definition.Name) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid injecting an object with empty name into the fake client in Get test.
An empty metadata.name can cause tracker/indexer issues. Pass an empty requested name without adding such an object to the mock client.
Apply:
- testCases := []struct {
- routeAdvertisement *ovnv1.RouteAdvertisements
- expectedError bool
- }{
+ testCases := []struct {
+ requestedName string
+ routeAdvertisement *ovnv1.RouteAdvertisements
+ expectedError bool
+ }{
{
- routeAdvertisement: buildDummyRouteAdvertisement(defaultRouteAdvertisementName),
+ requestedName: defaultRouteAdvertisementName,
+ routeAdvertisement: buildDummyRouteAdvertisement(defaultRouteAdvertisementName),
expectedError: false,
},
{
- routeAdvertisement: buildDummyRouteAdvertisement(""),
+ requestedName: "",
+ routeAdvertisement: nil,
expectedError: true,
},
}
...
- routeAdvertisementBuilder, err := PullRouteAdvertisement(testSettings.Client, testCase.routeAdvertisement.Name)
+ routeAdvertisementBuilder, err := PullRouteAdvertisement(testSettings.Client, testCase.requestedName)📝 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.
| func TestRouteAdvertisementGet(t *testing.T) { | |
| testCases := []struct { | |
| routeAdvertisement *ovnv1.RouteAdvertisements | |
| expectedError bool | |
| }{ | |
| { | |
| routeAdvertisement: buildDummyRouteAdvertisement(defaultRouteAdvertisementName), | |
| expectedError: false, | |
| }, | |
| { | |
| routeAdvertisement: buildDummyRouteAdvertisement(""), | |
| expectedError: true, | |
| }, | |
| } | |
| for _, testCase := range testCases { | |
| var ( | |
| runtimeObjects []runtime.Object | |
| ) | |
| if testCase.routeAdvertisement != nil { | |
| runtimeObjects = append(runtimeObjects, testCase.routeAdvertisement) | |
| } | |
| testSettings := clients.GetTestClients(clients.TestClientParams{ | |
| K8sMockObjects: runtimeObjects, | |
| SchemeAttachers: []clients.SchemeAttacher{ | |
| ovnv1.AddToScheme, | |
| }, | |
| }) | |
| routeAdvertisementBuilder, err := PullRouteAdvertisement(testSettings.Client, testCase.routeAdvertisement.Name) | |
| if testCase.expectedError { | |
| assert.NotNil(t, err) | |
| } else { | |
| assert.Nil(t, err) | |
| assert.Equal(t, testCase.routeAdvertisement.Name, routeAdvertisementBuilder.Definition.Name) | |
| } | |
| } | |
| } | |
| func TestRouteAdvertisementGet(t *testing.T) { | |
| testCases := []struct { | |
| requestedName string | |
| routeAdvertisement *ovnv1.RouteAdvertisements | |
| expectedError bool | |
| }{ | |
| { | |
| requestedName: defaultRouteAdvertisementName, | |
| routeAdvertisement: buildDummyRouteAdvertisement(defaultRouteAdvertisementName), | |
| expectedError: false, | |
| }, | |
| { | |
| requestedName: "", | |
| routeAdvertisement: nil, | |
| expectedError: true, | |
| }, | |
| } | |
| for _, testCase := range testCases { | |
| var ( | |
| runtimeObjects []runtime.Object | |
| ) | |
| if testCase.routeAdvertisement != nil { | |
| runtimeObjects = append(runtimeObjects, testCase.routeAdvertisement) | |
| } | |
| testSettings := clients.GetTestClients(clients.TestClientParams{ | |
| K8sMockObjects: runtimeObjects, | |
| SchemeAttachers: []clients.SchemeAttacher{ | |
| ovnv1.AddToScheme, | |
| }, | |
| }) | |
| routeAdvertisementBuilder, err := PullRouteAdvertisement( | |
| testSettings.Client, | |
| testCase.requestedName, | |
| ) | |
| if testCase.expectedError { | |
| assert.NotNil(t, err) | |
| } else { | |
| assert.Nil(t, err) | |
| assert.Equal( | |
| t, | |
| testCase.routeAdvertisement.Name, | |
| routeAdvertisementBuilder.Definition.Name, | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/ovn/routeadvertisement_test.go around lines 103 to 143, the test
currently injects a RouteAdvertisements object with an empty metadata.name into
the fake client which can break the tracker/indexer; update the test setup so
you only append the object to runtimeObjects when the object is non-nil AND its
Name is non-empty (i.e., skip adding testCase.routeAdvertisement to
K8sMockObjects when testCase.routeAdvertisement.Name == ""), but still call
PullRouteAdvertisement with the requested name (which may be empty) to exercise
the error path.
| v1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1" | ||
| routeadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration/routeadvertisements/v1" | ||
| typedrouteadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1" | ||
| gentype "k8s.io/client-go/gentype" | ||
| ) |
There was a problem hiding this comment.
Point fake client to local packages (types, applyconfig, typed interface)
Current imports bind to upstream modules; switch to local to keep tests and fakes in-repo.
- v1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"
- routeadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration/routeadvertisements/v1"
- typedrouteadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1"
+ v1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"
+ routeadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1"
+ typedrouteadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1"🤖 Prompt for AI Agents
In
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements.go
around lines 21-25, the imports reference the upstream go-controller module;
update them to the repository-local packages for the CRD types,
applyconfiguration and typed client. Replace
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"
with the repo-local path to the types package (e.g.
"github.com/ovn-org/ovn-kubernetes/pkg/crd/routeadvertisements/v1"), replace the
applyconfiguration import to
"github.com/ovn-org/ovn-kubernetes/pkg/crd/routeadvertisements/v1/apis/applyconfiguration/routeadvertisements/v1",
and replace the typed client import to
"github.com/ovn-org/ovn-kubernetes/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1";
keep gentype as-is.
| routeadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1" | ||
| applyconfigurationrouteadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration/routeadvertisements/v1" | ||
| scheme "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/scheme" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| types "k8s.io/apimachinery/pkg/types" | ||
| watch "k8s.io/apimachinery/pkg/watch" | ||
| gentype "k8s.io/client-go/gentype" | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Generated client imports point to upstream; switch to local packages
Using upstream types/scheme here will type-mismatch your locally synced types and tie the client to the ovn-kubernetes module. After fixing sync config, regenerate; if you need a hotfix, update imports as below.
- routeadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"
- applyconfigurationrouteadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration/routeadvertisements/v1"
- scheme "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/scheme"
+ routeadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"
+ applyconfigurationrouteadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1"
+ scheme "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme"Verify scope assumptions (cluster-scoped) align with empty namespace in the generated client:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Applyconfig has no WithNamespace (cluster-scoped hint):"
rg -n 'func RouteAdvertisements\(' pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go -n -A4Length of output: 522
Switch generated client imports to local packages
Using upstream CRD imports causes type mismatches with your locally synced types and ties the client to the ovn-kubernetes module; after fixing your sync config and regenerating the client, update the imports in pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go as follows:
- routeadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"
- applyconfigurationrouteadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration/routeadvertisements/v1"
- scheme "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/scheme"
+ routeadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"
+ applyconfigurationrouteadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1"
+ scheme "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme"Resource is cluster-scoped (no namespace), as confirmed by the absence of a WithNamespace call in the apply-configuration code.
📝 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.
| routeadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1" | |
| applyconfigurationrouteadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration/routeadvertisements/v1" | |
| scheme "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/scheme" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| types "k8s.io/apimachinery/pkg/types" | |
| watch "k8s.io/apimachinery/pkg/watch" | |
| gentype "k8s.io/client-go/gentype" | |
| ) | |
| routeadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1" | |
| applyconfigurationrouteadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1" | |
| scheme "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| types "k8s.io/apimachinery/pkg/types" | |
| watch "k8s.io/apimachinery/pkg/watch" | |
| gentype "k8s.io/client-go/gentype" | |
| ) |
🤖 Prompt for AI Agents
In
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go
lines 23-30, replace the upstream ovn-kubernetes import paths with the
repository's local package paths for routeadvertisements,
applyconfiguration/routeadvertisements and scheme so the generated client uses
the locally-synced types; after updating the imports, regenerate the client code
so all references align, and confirm the resource is treated as cluster-scoped
(no namespace handling) consistent with the apply-configuration code.
| */ | ||
| // Code generated by informer-gen. DO NOT EDIT. | ||
|
|
||
| package routeadvertisement |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Package name likely incorrect (singular vs plural).
Directory is routeadvertisements but package is routeadvertisement. If any sibling file in this directory uses the conventional plural, the build will fail due to mixed packages. Recommend aligning to plural.
Apply:
-package routeadvertisement
+package routeadvertisements🏁 Script executed:
#!/bin/bash
# Verify there are no mixed package names in this directory.
rg -nP '^\s*package\s+routeadvertisements\b|^\s*package\s+routeadvertisement\b' \
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisementsLength of output: 304
Align package name with directory
The declared package routeadvertisement doesn’t match its containing directory routeadvertisements, which will break the build.
Apply:
-package routeadvertisement
+package routeadvertisements📝 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.
| package routeadvertisement | |
| package routeadvertisements |
🤖 Prompt for AI Agents
In
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go
around line 18, the file declares package `routeadvertisement` but lives in the
`routeadvertisements` directory; change the package declaration to `package
routeadvertisements` so it matches the directory name and update any imports or
references if needed, then run `go build` or `go vet` to verify the fix.
| // +kubebuilder:resource:path=routeadvertisements,scope=Cluster,shortName=ra,singular=routeadvertisements | ||
| // +kubebuilder::singular=routeadvertisements | ||
| // +kubebuilder:object:root=true | ||
| // +kubebuilder:subresource:status | ||
| // +kubebuilder:printcolumn:name="Status",type=string,JSONPath=".status.status" | ||
| // RouteAdvertisements is the Schema for the routeadvertisements API |
There was a problem hiding this comment.
Fix kubebuilder resource annotations: invalid/duplicated singular.
- The singular must be “routeadvertisement” (not plural).
- The extra line with “+kubebuilder::singular=…” is invalid (double colon) and redundant.
These will break or mis-generate CRDs if used.
Apply:
-// +kubebuilder:resource:path=routeadvertisements,scope=Cluster,shortName=ra,singular=routeadvertisements
-//
-// +kubebuilder::singular=routeadvertisements
+// +kubebuilder:resource:path=routeadvertisements,scope=Cluster,shortName=ra,singular=routeadvertisement📝 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.
| // +kubebuilder:resource:path=routeadvertisements,scope=Cluster,shortName=ra,singular=routeadvertisements | |
| // +kubebuilder::singular=routeadvertisements | |
| // +kubebuilder:object:root=true | |
| // +kubebuilder:subresource:status | |
| // +kubebuilder:printcolumn:name="Status",type=string,JSONPath=".status.status" | |
| // RouteAdvertisements is the Schema for the routeadvertisements API | |
| // +kubebuilder:resource:path=routeadvertisements,scope=Cluster,shortName=ra,singular=routeadvertisement | |
| // +kubebuilder:object:root=true | |
| // +kubebuilder:subresource:status | |
| // +kubebuilder:printcolumn:name="Status",type=string,JSONPath=".status.status" | |
| // RouteAdvertisements is the Schema for the routeadvertisements API |
🤖 Prompt for AI Agents
In pkg/schemes/ovn/routeadvertisement/v1/types.go around lines 13 to 18, the
kubebuilder annotations contain an invalid duplicated singular and the singular
value is plural; change the singular in the +kubebuilder:resource annotation
from singular=routeadvertisements to singular=routeadvertisement and remove the
extraneous invalid line "+kubebuilder::singular=routeadvertisements", leaving
only the valid kubebuilder annotations (including +kubebuilder:resource with the
corrected singular).
5345226 to
09a12f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go (1)
23-26: Switch generated client imports to local packagesUse the locally-synced types/scheme to avoid type mismatches and upstream coupling.
Apply:
- routeadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1" - applyconfigurationrouteadvertisementsv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration/routeadvertisements/v1" - scheme "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/scheme" + routeadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1" + applyconfigurationrouteadvertisementsv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1" + scheme "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme"pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/register.go (1)
21-21: Use local v1 AddToScheme, not upstreamWire fake scheme to local types.
- k8sv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1" + k8sv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"internal/sync/configs/ovn-k-config.yaml (1)
20-23: Fix replace_imports: plural upstream path and include /v1 in localCurrent mapping won’t trigger because upstream uses plural; local should include /v1.
- - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"' - new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"' + - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"' + new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"'pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
13-15: Fix kubebuilder annotations: correct singular and remove invalid lineEnsures CRD generation is valid.
-// +kubebuilder:resource:path=routeadvertisements,scope=Cluster,shortName=ra,singular=routeadvertisements -// -// +kubebuilder::singular=routeadvertisements +// +kubebuilder:resource:path=routeadvertisements,scope=Cluster,shortName=ra,singular=routeadvertisementpkg/schemes/ovn/types/networkselector.go (1)
42-46: Fix incorrect doc: wrong NetworkSelectionType mentioned.The field selects when NetworkSelectionType is NetworkAttachmentDefinitions, not SecondaryUserDefinedNetworks.
Apply:
-// selected NetworkAttachmentDefinitions when NetworkSelectionType is -// 'SecondaryUserDefinedNetworks'. +// selected NetworkAttachmentDefinitions when NetworkSelectionType is +// 'NetworkAttachmentDefinitions'.pkg/ovn/routeadvertisement.go (1)
198-202: Fix Exists(): it returns true on non-NotFound errors.This treats RBAC/transient errors as “exists”, which breaks Create/Delete logic. Return true only when Get() succeeds.
Apply:
- return err == nil || !errors.IsNotFound(err) + return err == nil(optional, more verbose):
- return err == nil || !errors.IsNotFound(err) + if err != nil { + if errors.IsNotFound(err) { + return false + } + glog.V(100).Infof("Error checking RouteAdvertisement existence: %v", err) + return false + } + return true
🧹 Nitpick comments (6)
internal/sync/configs/ovn-k-config.yaml (2)
19-23: Add mappings for generated subpackages (applyconfiguration, scheme)Prevents stray upstream imports in generated clients.
replace_imports: - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1"' new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1"' + - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration/routeadvertisements/v1"' + new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1"' + - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/scheme"' + new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme"'
28-28: Add trailing newlineSatisfy yamllint.
pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
55-57: Optional: drop omitempty on a Required fieldAvoids ambiguity when clients serialize zero values.
- Advertisements []AdvertisementType `json:"advertisements,omitempty"` + Advertisements []AdvertisementType `json:"advertisements"`pkg/ovn/routeadvertisement.go (2)
151-159: Create(): define behavior when object already exists.Currently returns (builder, nil) without setting Object when Exists()==true. Either:
- no-op and ensure Object is populated, or
- return an AlreadyExists error.
Idempotent create recommended:
Apply:
var err error - if !builder.Exists() { + if !builder.Exists() { err = builder.apiClient.Create(context.TODO(), builder.Definition) if err == nil { builder.Object = builder.Definition } - } + } else { + // Keep Object from Exists()->Get() + if builder.Object == nil { + builder.Object, _ = builder.Get() + } + }
286-297: Validate networkSelectors not empty at builder level (defensive).If controller-gen doesn’t propagate MinItems from the alias, server-side may accept empty lists. Add a len check mirroring advertisements.
Apply:
func (builder *RouteAdvertisementBuilder) WithNetworkSelectors(networkSelectors types.NetworkSelectors) *RouteAdvertisementBuilder { if valid, _ := builder.validate(); !valid { return builder } glog.V(100).Infof("Setting RouteAdvertisement %s networkSelectors to %v", builder.Definition.Name, networkSelectors) - builder.Definition.Spec.NetworkSelectors = networkSelectors + if len(networkSelectors) == 0 { + glog.V(100).Infof("RouteAdvertisement 'networkSelectors' cannot be empty") + builder.errorMsg = "RouteAdvertisement 'networkSelectors' cannot be empty" + return builder + } + builder.Definition.Spec.NetworkSelectors = networkSelectors return builder }pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go (1)
201-225: Doc example references non-existent WaitForStop; align with API and package usage.Replace WaitForStop with Shutdown and show the typed informer path used here (K8s().V1().RouteAdvertisements()).
-// ctx, cancel := context.Background() -// defer cancel() -// factory := NewSharedInformerFactory(client, resyncPeriod) -// defer factory.WaitForStop() // Returns immediately if nothing was started. +// ctx, cancel := context.WithCancel(context.Background()) +// defer cancel() +// factory := NewSharedInformerFactory(client, resyncPeriod) +// defer factory.Shutdown() // Blocks until informer goroutines terminate. // genericInformer := factory.ForResource(resource) -// typedInformer := factory.SomeAPIGroup().V1().SomeType() +// typedInformer := factory.K8s().V1().RouteAdvertisements() // factory.Start(ctx.Done()) // Start processing these informers. // synced := factory.WaitForCacheSync(ctx.Done())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (39)
go.mod(6 hunks)internal/sync/configs/ovn-k-config.yaml(1 hunks)internal/sync/configs/ovn-only-config.yaml(1 hunks)pkg/ovn/routeadvertisement.go(1 hunks)pkg/ovn/routeadvertisement_test.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/internal/internal.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/advertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsspec.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsstatus.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/utils.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/clientset_generated.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/register.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements_client.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/generated_expansion.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements_client.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/generic.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/internalinterfaces/factory_interfaces.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/expansion_generated.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/routeadvertisements.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/doc.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/register.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/types.go(1 hunks)pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go(1 hunks)pkg/schemes/ovn/types/doc.go(1 hunks)pkg/schemes/ovn/types/networkselector.go(1 hunks)pkg/schemes/ovn/types/zz_generated.deepcopy.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pkg/schemes/ovn/routeadvertisement/v1/doc.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/doc.go
🚧 Files skipped from review as they are similar to previous changes (26)
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/doc.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/internalinterfaces/factory_interfaces.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/utils.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements_client.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsspec.go
- pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/doc.go
- pkg/schemes/ovn/routeadvertisement/v1/register.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/internal/internal.go
- pkg/schemes/ovn/types/zz_generated.deepcopy.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/routeadvertisements.go
- pkg/ovn/routeadvertisement_test.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/expansion_generated.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/generated_expansion.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisementsstatus.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go
- internal/sync/configs/ovn-only-config.yaml
- pkg/schemes/ovn/routeadvertisement/v1/apis/listers/routeadvertisements/v1/routeadvertisements.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements_client.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go
- go.mod
- pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/doc.go
- pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/advertisements.go
🧰 Additional context used
🧬 Code graph analysis (8)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/generic.go (1)
pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
SchemeGroupVersion(11-11)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/register.go (2)
pkg/schemes/ovn/routeadvertisement/v1/register.go (2)
SchemeBuilder(12-12)AddToScheme(13-13)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go (1)
AddToScheme(50-50)
pkg/ovn/routeadvertisement.go (5)
pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go (1)
RouteAdvertisements(37-43)pkg/schemes/ovn/routeadvertisement/v1/types.go (3)
RouteAdvertisements(19-25)AdvertisementType(61-61)RouteAdvertisementsSpec(30-57)pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)pkg/schemes/ovn/routeadvertisement/v1/register.go (3)
Kind(17-19)GroupName(10-10)SchemeGroupVersion(11-11)pkg/msg/msg.go (2)
FailToUpdateNotification(11-18)UndefinedCrdObjectErrString(6-8)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/clientset_generated.go (4)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go (2)
Clientset(36-39)Interface(30-33)pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/utils.go (1)
NewTypeConverter(45-47)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements_client.go (1)
K8sV1Interface(28-31)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake/fake_routeadvertisements_client.go (1)
FakeK8sV1(26-28)
pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go (4)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/routeadvertisements.go (1)
RouteAdvertisementsInformer(36-39)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go (1)
SharedInformerFactory(225-257)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/internalinterfaces/factory_interfaces.go (2)
SharedInformerFactory(33-36)TweakListOptionsFunc(39-39)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go (2)
New(38-40)Interface(26-29)
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go (6)
pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration/routeadvertisements/v1/routeadvertisements.go (2)
RouteAdvertisements(37-43)RouteAdvertisementsApplyConfiguration(28-33)pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
RouteAdvertisements(19-25)RouteAdvertisementsList(93-97)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/clientset.go (1)
Interface(30-33)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/generated_expansion.go (1)
RouteAdvertisementsExpansion(20-20)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements_client.go (1)
K8sV1Client(34-36)pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/scheme/register.go (1)
ParameterCodec(31-31)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go (4)
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/interface.go (2)
Interface(26-29)New(38-40)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go (2)
Interface(25-28)New(37-39)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/internalinterfaces/factory_interfaces.go (3)
TweakListOptionsFunc(39-39)SharedInformerFactory(33-36)NewInformerFunc(30-30)pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/generic.go (1)
GenericInformer(30-33)
🪛 YAMLlint (1.37.1)
internal/sync/configs/ovn-k-config.yaml
[error] 28-28: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration-k8s (ubuntu-22.04-arm)
- GitHub Check: integration-k8s (ubuntu-22.04)
- GitHub Check: build
🔇 Additional comments (12)
pkg/schemes/ovn/types/doc.go (2)
3-3: LGTM: package declaration.
2-2: Deepcopy generator output verifiedzz_generated.deepcopy.go exists and contains the expected “Code generated by deepcopy-gen” header.
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/register.go (1)
52-55: LGTM after import fixScheme/codec setup is standard for generated fakes.
pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
28-30: Nice: uniqueness constraint on advertisementsThe XValidation prevents duplicates and pairs well with Enum+MaxItems=2.
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/clientset_generated.go (1)
81-111: LGTM: field-managed tracker wiringSSA testing support via NewFieldManagedObjectTracker and TypeConverter looks correct.
pkg/schemes/ovn/types/networkselector.go (3)
16-26: Union shape and discriminators look solid.Required discriminator plus XValidations correctly bind each selector to its type; “DefaultNetwork” case permits no selector. Good.
49-68: Double-check allowed types in RouteAdvertisements.Spec.RouteAdvertisementsSpec comment says only DefaultNetwork and ClusterUserDefinedNetworks are allowed, but the enum here includes five values. Ensure the Spec adds an XValidation constraining each element of networkSelectors to those two types; otherwise users can submit Primary/Secondary/NAD types successfully.
I can draft the exact XValidation for the Spec field if desired.
7-14: Verify CRD schema for networkSelectors list annotations
controller-gen should emitx-kubernetes-list-type: mapandx-kubernetes-list-map-keys: ["networkSelectionType"]onspec.versions[*].schema.openAPIV3Schema.properties.spec.properties.networkSelectors. Manually generate the CRD (e.g.,controller-gen crd:crdVersions=v1 paths=./... output:crd:dir=./_out/crds) and inspect that field to confirm the annotations are applied.pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/routeadvertisements/v1/interface.go (2)
24-39: LGTM: versioned interface and constructor are consistent with informer-gen patterns.
41-44: Confirm scope: namespace is not passed to the typed informer.RouteAdvertisements() does not include v.namespace in the struct literal. If RouteAdvertisement is namespaced, this will ignore the WithNamespace option and list cluster-wide. Please verify the CRD scope. If namespaced, adjust generation so the struct includes namespace.
Example (only if namespaced):
- return &routeAdvertisementsInformer{factory: v.factory, tweakListOptions: v.tweakListOptions} + return &routeAdvertisementsInformer{factory: v.factory, namespace: v.namespace, tweakListOptions: v.tweakListOptions}pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/generic.go (1)
53-57: GVR mapping looks correct.The k8s.ovn.org/v1 RouteAdvertisements mapping to the typed informer is correct and returns a GenericInformer wrapping the typed informer.
pkg/schemes/ovn/routeadvertisement/v1/apis/informers/externalversions/factory.go (1)
123-145: Startup lifecycle and synchronization are sound.Mutex guarding, startedInformers gate, and wg usage are correct for idempotent Start.
| - name: ovn-kubernetes-routeadvertisement | ||
| sync: true | ||
| repo_link: "https://github.com/ovn-org/ovn-kubernetes" | ||
| branch: master | ||
| remote_api_directory: go-controller/pkg/crd/routeadvertisements | ||
| local_api_directory: schemes/ovn/routeadvertisement | ||
| replace_imports: | ||
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisement/v1"' | ||
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement"' | ||
| - old: '"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/types"' | ||
| new: '"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types"' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Mirror the same fixes in ovn-only-config.yaml
Keep sync configs consistent.
🏁 Script executed:
#!/bin/bash
rg -n 'ovn-only-config\.yaml' internal/sync/configs || true
rg -nP 'replace_imports:|routeadvertisements?/v1' internal/sync/configs/ovn-only-config.yaml -n -C2 || trueLength of output: 654
Align ovn-only-config.yaml with ovn-k-config.yaml
- In
internal/sync/configs/ovn-only-config.yaml, change
remote_api_directory: go-controller/pkg/crd/routeadvertisement/v1
to
remote_api_directory: go-controller/pkg/crd/routeadvertisements - Add the missing
replace_importsentry for CRD types:- old: "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/types" new: "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types"
- Ensure the
branch:value (main) matches the upstream default (master) if consistency is required.
🤖 Prompt for AI Agents
In internal/sync/configs/ovn-only-config.yaml around lines 13 to 23, update
remote_api_directory from go-controller/pkg/crd/routeadvertisement/v1 to
go-controller/pkg/crd/routeadvertisements, add a replace_imports entry mapping
old: "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/types" to new:
"github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types", and optionally
change branch: main to branch: master to match the upstream default for
consistency.
| if apiClient == nil { | ||
| glog.V(100).Infof("RouteAdvertisement 'apiClient' cannot be nil") | ||
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid returning nil builder on nil apiClient; return a builder with errorMsg instead.
Returning nil makes fluent chaining crash. Align with other validations by setting builder.errorMsg and returning the builder.
Apply:
- if apiClient == nil {
- glog.V(100).Infof("RouteAdvertisement 'apiClient' cannot be nil")
-
- return nil
- }
-
- builder := &RouteAdvertisementBuilder{
+ builder := &RouteAdvertisementBuilder{
apiClient: apiClient,
Definition: &ovnv1.RouteAdvertisements{
TypeMeta: metav1.TypeMeta{
Kind: "RouteAdvertisements",
APIVersion: fmt.Sprintf("%s/%s", ovnv1.GroupName, ovnv1.SchemeGroupVersion.Version),
},
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
Spec: ovnv1.RouteAdvertisementsSpec{
Advertisements: advertisements,
NodeSelector: nodeSelector,
FRRConfigurationSelector: frrConfigurationSelector,
NetworkSelectors: networkSelectors,
},
},
}
+
+ if apiClient == nil {
+ glog.V(100).Infof("RouteAdvertisement 'apiClient' cannot be nil")
+ builder.errorMsg = "RouteAdvertisement 'apiClient' cannot be nil"
+ return builder
+ }Also applies to: 49-67
🤖 Prompt for AI Agents
In pkg/ovn/routeadvertisement.go around lines 43-48 (and similarly for lines
49-67), the function currently returns nil when apiClient is nil which breaks
fluent chaining; instead instantiate and return the builder with
builder.errorMsg set to a descriptive error. Change the code so that when
apiClient is nil you create the builder object (as other validations do), set
builder.errorMsg to something like "apiClient cannot be nil" (including context
if helpful), and return that builder rather than nil; apply the same pattern for
the other validation blocks in the 49-67 range.
| applyconfiguration "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration" | ||
| clientset "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned" | ||
| k8sv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1" | ||
| fakek8sv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake" | ||
| "k8s.io/apimachinery/pkg/runtime" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Point fake client imports to local packages
Align fake client with local applyconfig, clientset, and typed clients.
- applyconfiguration "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration"
- clientset "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned"
- k8sv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1"
- fakek8sv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake"
+ applyconfiguration "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration"
+ clientset "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned"
+ k8sv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1"
+ fakek8sv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake"📝 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.
| applyconfiguration "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/applyconfiguration" | |
| clientset "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned" | |
| k8sv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1" | |
| fakek8sv1 "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake" | |
| "k8s.io/apimachinery/pkg/runtime" | |
| applyconfiguration "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/applyconfiguration" | |
| clientset "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned" | |
| k8sv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1" | |
| fakek8sv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/fake" | |
| "k8s.io/apimachinery/pkg/runtime" |
🤖 Prompt for AI Agents
In
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/fake/clientset_generated.go
around lines 21-25, the fake client imports currently reference external
github.com/ovn-org/ovn-kubernetes paths; change those import paths to the
corresponding local package paths under
pkg/schemes/ovn/routeadvertisement/v1/apis (applyconfiguration, clientset, typed
k8sv1 and fakek8sv1). Update the import block to use the local module paths,
save, then run gofmt/goimports and go build to ensure the imports compile
correctly.
| // newRouteAdvertisements returns a RouteAdvertisements | ||
| func newRouteAdvertisements(c *K8sV1Client) *routeAdvertisements { | ||
| return &routeAdvertisements{ | ||
| gentype.NewClientWithListAndApply[*routeadvertisementsv1.RouteAdvertisements, *routeadvertisementsv1.RouteAdvertisementsList, *applyconfigurationrouteadvertisementsv1.RouteAdvertisementsApplyConfiguration]( | ||
| "routeadvertisements", | ||
| c.RESTClient(), | ||
| scheme.ParameterCodec, | ||
| "", | ||
| func() *routeadvertisementsv1.RouteAdvertisements { return &routeadvertisementsv1.RouteAdvertisements{} }, | ||
| func() *routeadvertisementsv1.RouteAdvertisementsList { | ||
| return &routeadvertisementsv1.RouteAdvertisementsList{} | ||
| }, | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Cluster-scoped client wiring looks correct
Empty namespace arg confirms non-namespaced resource; rest wiring via local scheme’s ParameterCodec is fine (post-import fix).
Run to ensure no upstream imports remain:
🏁 Script executed:
#!/bin/bash
rg -nP 'github\.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements?/v1' pkg/schemes/ovn/routeadvertisement -g '!**/vendor/**' || trueLength of output: 7738
Remove upstream CRD imports in pkg/schemes/ovn/routeadvertisement
The files under pkg/schemes/ovn/routeadvertisement still import from github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1; replace these with the local module’s generated client packages.
🤖 Prompt for AI Agents
In
pkg/schemes/ovn/routeadvertisement/v1/apis/clientset/versioned/typed/routeadvertisements/v1/routeadvertisements.go
around lines 61-75, the code is still using upstream CRD types/imports from
github.com/ovn-org/ovn-kubernetes/...; replace those imports and type references
with the local module's generated client packages (the locally generated
routeadvertisements v1 package and applyconfiguration package), update the
gentype.NewClientWithListAndApply generic type arguments and the two factory
functions to return the local RouteAdvertisements and RouteAdvertisementsList
types, and adjust the import aliases to point to the local module path so the
file compiles against the project’s generated clients rather than the upstream
CRD package.
| informer = newFunc(f.client, resyncPeriod) | ||
| informer.SetTransform(f.transform) | ||
| f.informers[informerType] = informer | ||
|
|
||
| return informer | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure client-go version supports SetTransform.
informer.SetTransform(f.transform) requires a client-go version that exposes SetTransform on SharedIndexInformer. Please confirm go.mod pins a compatible k8s.io/client-go.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "client-go version in go.mod:"
rg -nP '^\s*k8s\.io/client-go\s+v[0-9]+\.[0-9]+\.[0-9]+' go.mod || true
echo
echo "Occurrences of SetTransform in repo:"
rg -n 'SetTransform\(' -g '!**/vendor/**' || trueLength of output: 450
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Clone client-go v12.0.0 and search for SetTransform
rm -rf client-go-v12
git clone --branch v12.0.0 --depth 1 https://github.com/kubernetes/client-go.git client-go-v12
grep -R "SetTransform(" client-go-v12/tools/cache || echo "Not found"Length of output: 894
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for ver in v0.25.3 v0.26.3 v0.27.0; do
echo "=== Checking client-go $ver ==="
rm -rf client-go-$ver
git clone --branch $ver --depth 1 https://github.com/kubernetes/client-go.git client-go-$ver
if grep -R "SetTransform" client-go-$ver/tools/cache -n; then
echo "$ver: SetTransform found"
else
echo "$ver: SetTransform not found"
fi
echo
doneLength of output: 4219
Upgrade k8s.io/client-go to ≥ v0.25.3
go.mod currently pins k8s.io/client-go v12.0.0+incompatible, which predates the addition of SharedIndexInformer.SetTransform (introduced in v0.25.3).
| @@ -0,0 +1,3 @@ | |||
| // Package types contains shared types accrorss API Schema definitions | |||
There was a problem hiding this comment.
Fix typo and polish package comment.
"accrorss" → "across"; use lowercase "schema"; end with period.
Apply:
-// Package types contains shared types accrorss API Schema definitions
+// Package types contains shared types across API schema definitions.📝 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.
| // Package types contains shared types accrorss API Schema definitions | |
| // Package types contains shared types across API schema definitions. |
🤖 Prompt for AI Agents
In pkg/schemes/ovn/types/doc.go around line 1, fix the package comment by
correcting the typo and polishing wording: change "Package types contains shared
types accrorss API Schema definitions" to a single, well-formed sentence such as
"Package types contains shared types across API schema definitions." (use
lowercase "schema" and ensure the comment ends with a period).
| // ClusterUserDefinedNetworkSelector selects ClusterUserDefinedNetworks. | ||
| type ClusterUserDefinedNetworkSelector struct { | ||
| // networkSelector selects ClusterUserDefinedNetworks by label. A null | ||
| // selector will mot match anything, while an empty ({}) selector will match | ||
| // all. | ||
| // +kubebuilder:validation:Required | ||
| NetworkSelector metav1.LabelSelector `json:"networkSelector"` | ||
| } |
There was a problem hiding this comment.
Correct selector semantics text and typo (“mot” → “not”).
This field is required and non-pointer, so “null selector” isn’t applicable. Keep the empty-selector semantics only.
Apply:
-// networkSelector selects ClusterUserDefinedNetworks by label. A null
-// selector will mot match anything, while an empty ({}) selector will match
-// all.
+// networkSelector selects ClusterUserDefinedNetworks by label. An empty ({})
+// selector will match all.📝 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.
| // ClusterUserDefinedNetworkSelector selects ClusterUserDefinedNetworks. | |
| type ClusterUserDefinedNetworkSelector struct { | |
| // networkSelector selects ClusterUserDefinedNetworks by label. A null | |
| // selector will mot match anything, while an empty ({}) selector will match | |
| // all. | |
| // +kubebuilder:validation:Required | |
| NetworkSelector metav1.LabelSelector `json:"networkSelector"` | |
| } | |
| // ClusterUserDefinedNetworkSelector selects ClusterUserDefinedNetworks. | |
| type ClusterUserDefinedNetworkSelector struct { | |
| // networkSelector selects ClusterUserDefinedNetworks by label. An empty ({}) | |
| // selector will match all. | |
| // +kubebuilder:validation:Required | |
| NetworkSelector metav1.LabelSelector `json:"networkSelector"` | |
| } |
🤖 Prompt for AI Agents
In pkg/schemes/ovn/types/networkselector.go around lines 70 to 77, update the
comment on ClusterUserDefinedNetworkSelector to fix the typo ("mot" → "not") and
correct the selector semantics: since NetworkSelector is required and
non-pointer, remove any mention of a "null selector" (not applicable) and state
only that an empty selector ({}) matches all; keep the rest of the description
and validation tag as-is.
| // PrimaryUserDefinedNetworkSelector selects primary UserDefinedNetworks. | ||
| type PrimaryUserDefinedNetworkSelector struct { | ||
| // namespaceSelector select the primary UserDefinedNetworks that are servind | ||
| // the selected namespaces. This field follows standard label selector | ||
| // semantics. | ||
| // +kubebuilder:validation:Required | ||
| NamespaceSelector metav1.LabelSelector `json:"namespaceSelector"` | ||
| } |
There was a problem hiding this comment.
Fix grammar: “select” → “selects”; “servind” → “serving”.
Apply:
-// namespaceSelector select the primary UserDefinedNetworks that are servind
+// namespaceSelector selects the primary UserDefinedNetworks that are serving
// the selected namespaces. This field follows standard label selector
// semantics.📝 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.
| // PrimaryUserDefinedNetworkSelector selects primary UserDefinedNetworks. | |
| type PrimaryUserDefinedNetworkSelector struct { | |
| // namespaceSelector select the primary UserDefinedNetworks that are servind | |
| // the selected namespaces. This field follows standard label selector | |
| // semantics. | |
| // +kubebuilder:validation:Required | |
| NamespaceSelector metav1.LabelSelector `json:"namespaceSelector"` | |
| } | |
| // PrimaryUserDefinedNetworkSelector selects primary UserDefinedNetworks. | |
| type PrimaryUserDefinedNetworkSelector struct { | |
| // namespaceSelector selects the primary UserDefinedNetworks that are serving | |
| // the selected namespaces. This field follows standard label selector | |
| // semantics. | |
| // +kubebuilder:validation:Required | |
| NamespaceSelector metav1.LabelSelector `json:"namespaceSelector"` | |
| } |
🤖 Prompt for AI Agents
In pkg/schemes/ovn/types/networkselector.go around lines 79 to 86, the struct
field comments contain grammatical mistakes: change "select" to "selects" and
correct "servind" to "serving" in the NamespaceSelector comment so it reads
clearly (e.g., "namespaceSelector selects the primary UserDefinedNetworks that
are serving the selected namespaces. This field follows standard label selector
semantics."). Ensure punctuation and capitalization remain consistent with
surrounding comments.
Summary by CodeRabbit
New Features
Tests
Chores