Adding output of impacted isvcs when --verbose flag is set. #33
Adding output of impacted isvcs when --verbose flag is set. #33lburgazzoli merged 4 commits intoopendatahub-io:mainfrom
Conversation
…espace, and Deployment mode is printed out. Signed-off-by: Andres Llausas <allausas@redhat.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a deployment-mode filter and verbose formatting for KServe impacted workloads: new Changes
Sequence DiagramsequenceDiagram
participant Client
participant ImpactedWorkloadsCheck
participant DiagnosticResult
participant TableRenderer
participant IOWriter
Client->>ImpactedWorkloadsCheck: FormatVerboseOutput(out, dr)
ImpactedWorkloadsCheck->>DiagnosticResult: iterate impacted objects
loop for each object
DiagnosticResult-->>ImpactedWorkloadsCheck: object (kind, metadata, annotations)
alt object.kind == InferenceService
ImpactedWorkloadsCheck->>ImpactedWorkloadsCheck: determine DeploymentMode (annotations → rawDeployment → Unknown)
ImpactedWorkloadsCheck->>ImpactedWorkloadsCheck: apply deploymentModeFilter
ImpactedWorkloadsCheck->>ImpactedWorkloadsCheck: append inferenceServiceRow
end
end
ImpactedWorkloadsCheck->>ImpactedWorkloadsCheck: sort rows by Namespace, Name
ImpactedWorkloadsCheck->>TableRenderer: render table rows
TableRenderer->>IOWriter: write formatted table
IOWriter-->>Client: output complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/lint/checks/workloads/kserve/impacted_test.go (1)
1258-1315: Consider adding an end-to-end test and verifying sort orderTwo gaps in the current test coverage:
End-to-end path not exercised: The test manually constructs
drwith explicitTypeMetarather than callingValidatefirst. If anyappend*helper omitsTypeMeta.Kindon the objects it puts intoImpactedObjects,FormatVerboseOutputwould silently emit no rows, and this test would not catch it. A complementary test that callsValidate→FormatVerboseOutputwould close that gap.Sort order not asserted: The test only checks substring presence, not that
ns-a/isvc-serverlessappears beforens-b/isvc-modelmesh. A simple index comparison would verify the sorting contract:♻️ Proposed addition for sort-order assertion
+ // Verify sort order: ns-a must appear before ns-b + g.Expect(strings.Index(output, "ns-a")).To(BeNumerically("<", strings.Index(output, "ns-b")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/workloads/kserve/impacted_test.go` around lines 1258 - 1315, Add an end-to-end test that builds the same scenario but runs the check's Validate method before calling FormatVerboseOutput (call kserve.NewImpactedWorkloadsCheck(), chk.Validate(dr) or the equivalent public validate entry, then chk.FormatVerboseOutput) so missing TypeMeta.Kind from append helpers will be caught; also assert the sort order by locating substrings (e.g., strings.Index(output, "ns-a") vs strings.Index(output, "ns-b") or the full "ns-a/isvc-serverless" vs "ns-b/isvc-modelmesh") to ensure ns-a/isvc-serverless appears before ns-b/isvc-modelmesh rather than only checking presence.pkg/lint/checks/workloads/kserve/impacted.go (1)
182-182: Pre-allocateisvcsto satisfy the linterThe slice should be allocated with a capacity to avoid repeated reallocations and silence the
preallocgolangci-lint warning.♻️ Proposed fix
- var isvcs []inferenceServiceRow + isvcs := make([]inferenceServiceRow, 0, len(dr.ImpactedObjects))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/workloads/kserve/impacted.go` at line 182, Pre-allocate the isvcs slice to avoid reallocations and satisfy the linter: replace the declaration of isvcs ([]inferenceServiceRow) with a make(...) that creates a zero-length slice with a capacity equal to the expected number of entries (for example use the length of the source collection you iterate over, e.g. len(inferenceServices) or len(rows) as appropriate). Ensure you update the declaration for the symbol isvcs (type inferenceServiceRow) so subsequent appends use the reserved capacity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/lint/checks/workloads/kserve/impacted_test.go`:
- Around line 1258-1315: Add an end-to-end test that builds the same scenario
but runs the check's Validate method before calling FormatVerboseOutput (call
kserve.NewImpactedWorkloadsCheck(), chk.Validate(dr) or the equivalent public
validate entry, then chk.FormatVerboseOutput) so missing TypeMeta.Kind from
append helpers will be caught; also assert the sort order by locating substrings
(e.g., strings.Index(output, "ns-a") vs strings.Index(output, "ns-b") or the
full "ns-a/isvc-serverless" vs "ns-b/isvc-modelmesh") to ensure
ns-a/isvc-serverless appears before ns-b/isvc-modelmesh rather than only
checking presence.
In `@pkg/lint/checks/workloads/kserve/impacted.go`:
- Line 182: Pre-allocate the isvcs slice to avoid reallocations and satisfy the
linter: replace the declaration of isvcs ([]inferenceServiceRow) with a
make(...) that creates a zero-length slice with a capacity equal to the expected
number of entries (for example use the length of the source collection you
iterate over, e.g. len(inferenceServices) or len(rows) as appropriate). Ensure
you update the declaration for the symbol isvcs (type inferenceServiceRow) so
subsequent appends use the reserved capacity.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
pkg/lint/checks/workloads/kserve/impacted.gopkg/lint/checks/workloads/kserve/impacted_test.go
… verbose output. Default to all. Signed-off-by: Andres Llausas <allausas@redhat.com>
Signed-off-by: Andres Llausas <allausas@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/lint/command.go (1)
122-125:⚠️ Potential issue | 🟡 Minor
ISVCDeploymentModeis not defaulted inNewCommand—Validate()will fail for programmatic callers
ISVCDeploymentModeis zero-valued ("") whenCommandis constructed directly without subsequently callingAddFlags. BecauseValidate()rejects any value outside["all","serverless","modelmesh"], any test or library consumer that constructsCommanddirectly (e.g., table-driven tests ofRun/Validate) will get an unexpected validation error unless they remember to set this field. The default should be co-located with the constructor.🛠️ Proposed fix
c := &Command{ SharedOptions: shared, registry: registry, + ISVCDeploymentMode: "all", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/command.go` around lines 122 - 125, The Command constructor currently leaves ISVCDeploymentMode zero-valued causing Validate() to fail for callers that instantiate Command directly; update NewCommand (the function that creates &Command{...}) to initialize ISVCDeploymentMode to a sensible default (e.g., "all") so programmatic consumers and tests don't need to call AddFlags before Validate; adjust the initialization in NewCommand where Command is created and ensure the default value aligns with the accepted values used by Validate() (["all","serverless","modelmesh"]).
🧹 Nitpick comments (2)
pkg/lint/checks/workloads/kserve/impacted.go (2)
194-194: Pre-allocateisvcsto avoid repeated re-allocations (flagged by static analysis)Using
len(dr.ImpactedObjects)as the capacity is a safe upper bound even though some objects are filtered; it avoids the repeated backing-array reallocations that happen with an uninitialized nil slice.♻️ Proposed fix
- var isvcs []inferenceServiceRow + isvcs := make([]inferenceServiceRow, 0, len(dr.ImpactedObjects))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/workloads/kserve/impacted.go` at line 194, Pre-allocate the isvcs slice to avoid repeated reallocations by initializing it with capacity equal to len(dr.ImpactedObjects) instead of leaving it nil; update the declaration of isvcs (the slice of inferenceServiceRow) to use make with length 0 and capacity len(dr.ImpactedObjects) so subsequent appends reuse the allocated backing array.
7-7: Optional: replacesort.Slicewithslices.SortFuncThe
sortpackage itself notes that "in many situations, the newerslices.SortFuncfunction is more ergonomic and runs faster." The codebase already importsslicesincommand.go; aligning this file keeps the approach consistent and avoids the reflection overhead ofsort.Slice.♻️ Proposed refactor
- "sort" + "cmp" + "slices"- // Sort by namespace, then by name - sort.Slice(isvcs, func(i, j int) bool { - if isvcs[i].Namespace != isvcs[j].Namespace { - return isvcs[i].Namespace < isvcs[j].Namespace - } - - return isvcs[i].Name < isvcs[j].Name - }) + // Sort by namespace, then by name + slices.SortFunc(isvcs, func(a, b inferenceServiceRow) int { + if n := cmp.Compare(a.Namespace, b.Namespace); n != 0 { + return n + } + return cmp.Compare(a.Name, b.Name) + })Also applies to: 238-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/workloads/kserve/impacted.go` at line 7, Replace uses of sort.Slice in pkg/lint/checks/workloads/kserve/impacted.go with slices.SortFunc: remove the "sort" import, add/ensure the "slices" import, and change each comparator from index-based (func(i, j int) bool { ... slice[i] ... slice[j] ... }) to element-based (func(a, b T) bool { ... compare a and b ... }). Update the two occurrences (including the block around the previous lines 238-244) to call slices.SortFunc(yourSlice, func(a, b <elementType>) bool { ... }) so the logic compares elements directly and no longer relies on indices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/lint/command.go`:
- Around line 122-125: The Command constructor currently leaves
ISVCDeploymentMode zero-valued causing Validate() to fail for callers that
instantiate Command directly; update NewCommand (the function that creates
&Command{...}) to initialize ISVCDeploymentMode to a sensible default (e.g.,
"all") so programmatic consumers and tests don't need to call AddFlags before
Validate; adjust the initialization in NewCommand where Command is created and
ensure the default value aligns with the accepted values used by Validate()
(["all","serverless","modelmesh"]).
---
Nitpick comments:
In `@pkg/lint/checks/workloads/kserve/impacted.go`:
- Line 194: Pre-allocate the isvcs slice to avoid repeated reallocations by
initializing it with capacity equal to len(dr.ImpactedObjects) instead of
leaving it nil; update the declaration of isvcs (the slice of
inferenceServiceRow) to use make with length 0 and capacity
len(dr.ImpactedObjects) so subsequent appends reuse the allocated backing array.
- Line 7: Replace uses of sort.Slice in
pkg/lint/checks/workloads/kserve/impacted.go with slices.SortFunc: remove the
"sort" import, add/ensure the "slices" import, and change each comparator from
index-based (func(i, j int) bool { ... slice[i] ... slice[j] ... }) to
element-based (func(a, b T) bool { ... compare a and b ... }). Update the two
occurrences (including the block around the previous lines 238-244) to call
slices.SortFunc(yourSlice, func(a, b <elementType>) bool { ... }) so the logic
compares elements directly and no longer relies on indices.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
pkg/lint/checks/workloads/kserve/impacted.gopkg/lint/command.gopkg/lint/constants.go
Signed-off-by: Andres Llausas <allausas@redhat.com>
|
/lgtm |
|
(output looks good to me) |
4 participants
Name, Namespace, and Deployment mode is printed out. Also added --isvc-deployment-mode flag. New flag allows users to filter for only serverless or modelmesh ISVCs when using the --verbose flag.
New output:
Summary by CodeRabbit
New Features
Tests