-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(buckets): fix the filtering condition when retrieving the number of manifests #6509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughUpdates Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant LB as list_buckets (lib/buckets.ps1)
participant FS as Filesystem
U->>LB: Request bucket listing
LB->>FS: Get-ChildItem -Path "$path\\bucket" -Recurse -Force -Filter "*.json" -File
note right of FS `#d4f1d4`: Returns only JSON manifest files
FS-->>LB: JSON file list
LB->>LB: Count/aggregate manifests per bucket
LB-->>U: Return buckets with corrected Manifests counts
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-31T01:48:00.222ZApplied to files:
🔇 Additional comments (1)
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 |
Co-authored-by: z-Fng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/buckets.ps1 (1)
116-117: Good fix! Consider using-Filterfor better performance.The addition of
-Include "*.json"and-Filecorrectly addresses the bug where subdirectories were being counted as manifests. This implementation matches the past review suggestion.However, for better performance and consistency with the existing code at line 53, consider using
-Filterinstead of-Include:- $bucket.Manifests = Get-ChildItem -Path "$path\bucket" -Include "*.json" -File -Force -Recurse -ErrorAction SilentlyContinue | + $bucket.Manifests = Get-ChildItem -Path "$path\bucket" -Filter "*.json" -File -Force -Recurse -ErrorAction SilentlyContinue | Measure-Object | Select-Object -ExpandProperty CountThe
-Filterparameter is applied by the provider before results are returned, making it more efficient than-Include, which filters after retrieval.
Small tweak: use Please update the PR title and resolve merge conflicts as well. |
|
cc: @niheaven Could we merge this? Please take a look when you have time. |
Description
Fix the filtering condition when retrieving the number of manifests
Motivation and Context
Closes #6508
How Has This Been Tested?
Before
After
Checklist:
developbranch.Summary by CodeRabbit
Bug Fixes
Documentation