WRKLDS-1191: oc rsync: Add --last flag #2078
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tchap: This pull request references WRKLDS-1191 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@tchap: This pull request references WRKLDS-1191 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
I guess this can also wait for 4.21 as nobody is going to use it in 4.20 anyway... |
tchap
force-pushed
the
rsync-last-n-include
branch
2 times, most recently
from
9f81cd5 to
b3c84cc
Compare
|
/test okd-scos-e2e-aws-ovn |
|
/retest |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
WalkthroughAdds a --last option to copy only the N most-recently modified files. Introduces a file discovery abstraction with local and remote implementations, wires discovery into rsync and tar strategies, updates CLI validation/help, and adds unit tests and test helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
Comment |
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
tchap
force-pushed
the
rsync-last-n-include
branch
from
b3c84cc to
a587e18
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @pkg/cli/rsync/copy_rsync.go:
- Around line 95-112: When r.Last > 0 make a nil-check for r.fileDiscovery
before calling r.fileDiscovery.DiscoverFiles to avoid a panic in cases where
rsyncStrategy was instantiated directly (e.g., in tests); if r.fileDiscovery is
nil, either initialize a sensible default file discovery implementation or log a
warning and skip the --last branch (leaving in and dst unchanged), otherwise
call r.fileDiscovery.DiscoverFiles as before and proceed; reference r.Last,
r.fileDiscovery.DiscoverFiles, and rsyncStrategy (and Complete() which normally
sets fileDiscovery) to locate the code to change.
In @pkg/cli/rsync/copy_rsyncd.go:
- Around line 238-255: The block that applies --last in copy_rsyncd.go calls
s.fileDiscovery.DiscoverFiles when s.Last > 0 but lacks a nil check and can
panic if s.fileDiscovery is nil; update the conditional to first verify
s.fileDiscovery != nil before calling DiscoverFiles, e.g., if s.Last > 0 &&
s.fileDiscovery != nil { ... } and if fileDiscovery is nil log a warning (using
klog.Infof or klog.Warningf) that --last was ignored; keep the existing behavior
of populating in, adjusting dst, and logging when DiscoverFiles returns an error
or succeeds.
In @pkg/cli/rsync/discovery_remote.go:
- Around line 44-47: The code extracts filename using filepath.Base on a remote
container path (variables fullPath and filename in discovery_remote.go) which
breaks on Windows; replace filepath.Base with path.Base (from the "path"
package) to operate on POSIX-style forward-slash remote paths and update imports
accordingly so the file uses the "path" package instead of "path/filepath".
🧹 Nitpick comments (3)
pkg/cli/rsync/discovery_remote.go (1)
51-52: Checkscanner.Err()after the scan loop.The scanner may encounter errors during iteration. Add a check after the loop to ensure no errors occurred.
🔎 Proposed fix
} + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("error reading remote command output: %w", err) + } return filenames, nilpkg/cli/rsync/copy_tar_test.go (1)
66-74: Type assertion could panic; consider a safer approach.The type assertion
NewTarStrategy(options).(*tarStrategy)will panic ifNewTarStrategyreturns a different concrete type. While this is unlikely in a test, using the two-value form provides clearer test failure output.🔎 Suggested improvement
- strategy := NewTarStrategy(options).(*tarStrategy) + strategy, ok := NewTarStrategy(options).(*tarStrategy) + if !ok { + t.Fatal("NewTarStrategy did not return *tarStrategy") + }pkg/cli/rsync/copy_rsyncd.go (1)
227-274: Consider extracting duplicated file discovery logic.The file discovery block (building stdin buffer, adjusting destination path, logging) is nearly identical to
copy_rsync.go. Extracting this to a helper function would improve maintainability and ensure consistent behavior across strategies.🔎 Example helper signature
// discoverFilesForRsync returns the stdin reader and adjusted destination path // when --last filtering is enabled. Returns nil reader if discovery fails or is disabled. func discoverFilesForRsync(fd fileDiscoverer, last uint, sourcePath, destPath string) (io.Reader, string) { // ... shared logic ... }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
pkg/cli/rsync/copy_rsync.gopkg/cli/rsync/copy_rsyncd.gopkg/cli/rsync/copy_tar.gopkg/cli/rsync/copy_tar_test.gopkg/cli/rsync/discovery.gopkg/cli/rsync/discovery_local.gopkg/cli/rsync/discovery_local_test.gopkg/cli/rsync/discovery_remote.gopkg/cli/rsync/discovery_remote_test.gopkg/cli/rsync/discovery_test.gopkg/cli/rsync/exec_test.gopkg/cli/rsync/rsync.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/rsync/discovery_remote_test.gopkg/cli/rsync/discovery_remote.gopkg/cli/rsync/rsync.gopkg/cli/rsync/discovery_test.gopkg/cli/rsync/copy_rsyncd.gopkg/cli/rsync/copy_tar.gopkg/cli/rsync/discovery_local_test.gopkg/cli/rsync/discovery.gopkg/cli/rsync/discovery_local.gopkg/cli/rsync/copy_tar_test.gopkg/cli/rsync/exec_test.gopkg/cli/rsync/copy_rsync.go
🧬 Code graph analysis (2)
pkg/cli/rsync/copy_tar.go (1)
pkg/helpers/source-to-image/tar/tar.go (2)
Tar(32-73)Writer(82-86)
pkg/cli/rsync/copy_rsync.go (1)
pkg/cli/rsync/pathspec.go (1)
PathSpec(17-20)
🔇 Additional comments (14)
pkg/cli/rsync/exec_test.go (1)
1-27: LGTM!Clean mock implementation for testing command execution. The structure properly validates expected commands and returns configurable output/errors.
pkg/cli/rsync/copy_tar.go (2)
48-65: LGTM on the discovery integration.The error handling correctly logs warnings and proceeds without filtering when discovery fails, which aligns with the graceful degradation behavior described in the PR.
222-253: Verify--no-recursionbehavior with trailing-slash sources.The
--no-recursionflag is only applied when the source path does not end with/. When the source ends with/, tar includes specific file patterns without--no-recursion. Please confirm this asymmetry is intentional and that file-only filtering works correctly for both path styles.pkg/cli/rsync/discovery.go (1)
1-7: LGTM!Clean interface definition with appropriate abstraction for file discovery.
pkg/cli/rsync/discovery_local_test.go (1)
12-97: LGTM!Good test coverage with clear setup and multiple scenarios. The test properly validates ordering by modification time and ensures subdirectories are ignored.
pkg/cli/rsync/discovery_local.go (1)
19-65: LGTM!Clean implementation with proper error handling and efficient pre-allocation. The sorting and limiting logic is correct.
pkg/cli/rsync/discovery_test.go (1)
1-14: LGTM!Simple and effective mock for testing discovery behavior.
pkg/cli/rsync/copy_tar_test.go (1)
12-77: Well-structured test with good coverage of edge cases.The test cases comprehensively cover the file discovery logic: successful discovery replacing originals, fallback when discovery returns empty or errors, and handling of nil/empty states. The use of table-driven tests and
cmp.Difffor clear failure output follows Go testing best practices.pkg/cli/rsync/rsync.go (2)
237-245: File discovery initialization is correctly ordered.The file discovery setup happens before strategy initialization, ensuring strategies can use the discovered files during construction. The conditional logic properly distinguishes between local and remote sources.
276-287: Validation for mutually exclusive flags is comprehensive.The switch-case structure cleanly validates that
--lastcannot be combined with--watch,--include,--exclude, or--delete. Error messages clearly identify which flags conflict.pkg/cli/rsync/discovery_remote_test.go (2)
12-87: Good test coverage for remote file discovery.The test suite covers the expected scenarios: successful discovery with exact and partial results, empty directories, and error propagation. The mock executor approach cleanly isolates the discovery logic from actual command execution.
64-72: Fix shell injection vulnerability in command construction.Line 28 directly embeds
basePathin single quotes without escaping:fmt.Sprintf("find '%s' ...", basePath). IfbasePathcontains a single quote, it breaks the shell command and enables injection. Use proper shell escaping or an escaping library before embedding user-controlled paths into shell strings.pkg/cli/rsync/copy_rsync.go (1)
87-133: Clean integration of--lastfiltering with rsync strategy.The implementation properly builds the file list for
--files-from, adjusts the destination path to maintain rsync semantics, and gracefully falls back on discovery failure. The warning log ensures users are informed when filtering cannot be applied.pkg/cli/rsync/copy_rsyncd.go (1)
324-341: Strategy initialization correctly propagates--lastoptions.The
NewRsyncDaemonStrategyfunction properly initializesLastandfileDiscoveryfrom the options, maintaining consistency with other strategy constructors.
tchap
force-pushed
the
rsync-last-n-include
branch
from
a587e18 to
b8bcada
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @pkg/cli/rsync/copy_rsync.go:
- Around line 107-108: The destination construction unconditionally appends
filepath.Base(source.Path) which yields "." when source.Path has a trailing
slash; update the dst adjustment in copy_rsync.go (the block that currently does
dst = filepath.Join(dst, filepath.Base(source.Path))) to only append
filepath.Base(source.Path) when source.Path does not end with "/" (use
strings.HasSuffix(source.Path, "/") check, matching the logic in copy_tar.go);
apply the same fix to the analogous code in copy_rsyncd.go so both rsync paths
behave like copy_tar.go when handling --files-from inputs with trailing slashes.
In @pkg/cli/rsync/copy_tar.go:
- Around line 48-65: The code calls o.fileDiscovery.DiscoverFiles without
checking o.fileDiscovery for nil and unconditionally sets NoRecursion based on
o.Last; fix by first verifying o.fileDiscovery != nil before calling
DiscoverFiles (log a warning and skip discovery if nil), and track discovery
success (e.g., discoverySucceeded bool) so that includes is replaced and
klog.V(3) logged only when discoverySucceeded && len(filenames) > 0; ensure the
NoRecursion flag is only set when discoverySucceeded (and files were applied),
avoiding --no-recursion when discovery failed or was skipped.
🧹 Nitpick comments (1)
pkg/cli/rsync/exec_test.go (1)
25-25: Check the error return fromWrite.The
Writecall can fail, and ignoring the error could mask test failures. Consider checking and returning the error.♻️ Proposed fix
- out.Write([]byte(m.output)) - return nil + if _, err := out.Write([]byte(m.output)); err != nil { + return err + } + return nil
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
pkg/cli/rsync/copy_rsync.gopkg/cli/rsync/copy_rsyncd.gopkg/cli/rsync/copy_tar.gopkg/cli/rsync/copy_tar_test.gopkg/cli/rsync/discovery.gopkg/cli/rsync/discovery_local.gopkg/cli/rsync/discovery_local_test.gopkg/cli/rsync/discovery_remote.gopkg/cli/rsync/discovery_remote_test.gopkg/cli/rsync/discovery_test.gopkg/cli/rsync/exec_test.gopkg/cli/rsync/rsync.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/rsync/discovery_local_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/cli/rsync/discovery.go
- pkg/cli/rsync/copy_rsyncd.go
- pkg/cli/rsync/discovery_local.go
- pkg/cli/rsync/copy_tar_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/rsync/rsync.gopkg/cli/rsync/copy_tar.gopkg/cli/rsync/discovery_remote_test.gopkg/cli/rsync/discovery_test.gopkg/cli/rsync/copy_rsync.gopkg/cli/rsync/discovery_remote.gopkg/cli/rsync/exec_test.go
🧬 Code graph analysis (2)
pkg/cli/rsync/copy_tar.go (1)
pkg/helpers/source-to-image/tar/tar.go (2)
Tar(32-73)Writer(82-86)
pkg/cli/rsync/copy_rsync.go (1)
pkg/cli/rsync/pathspec.go (1)
PathSpec(17-20)
🔇 Additional comments (17)
pkg/cli/rsync/discovery_test.go (1)
1-14: LGTM!The mock implementation is clean and appropriate for testing the file discovery interface.
pkg/cli/rsync/rsync.go (6)
44-60: LGTM!The documentation and examples clearly explain the new
--lastflag functionality.
95-95: LGTM!The
Lastfield is appropriately typed asuintfor storing the count of files to sync.
112-114: LGTM!The
fileDiscoveryfield is well-documented, explaining its purpose for testing and--lastfunctionality.
142-142: LGTM!The flag definition correctly uses
UintVarto prevent negative values.
237-245: LGTM!The file discovery initialization correctly selects the appropriate discoverer based on whether the source is local or remote, and is properly placed before strategy initialization.
276-287: LGTM!The validation logic correctly prevents incompatible flag combinations with
--last:
--watch(continuous sync conflicts with one-time filtered sync)--include/--exclude(alternative filtering mechanisms)--delete(dangerous with partial file sets)All restrictions are well-justified.
pkg/cli/rsync/discovery_remote.go (4)
7-7: LGTM!Correctly uses the
pathpackage instead ofpath/filepathfor handling remote POSIX paths, which addresses the previous review concern about Windows compatibility.
29-30: LGTM!The single-quote escaping correctly prevents command injection by using the standard shell escaping pattern
'\'', which addresses the previous review concern.
32-35: Verify GNU find availability in target containers.The command uses
find -printf, which is a GNU find extension not available in BSD find (macOS, FreeBSD). While this is likely fine since most containers run Linux with GNU find, it could fail in containers using busybox or other minimal distributions with BSD-style find.Consider whether this is a concern for your target environment. If busybox or Alpine-based containers are expected targets, you may need a more portable approach using
statorls -lt.
37-60: LGTM!The output parsing logic is robust:
- Handles empty lines
- Validates expected format
- Correctly uses
path.Basefor remote paths- Comprehensive error handling
The fix to use
path.Baseinstead offilepath.Baseproperly addresses the previous review concern about Windows compatibility.pkg/cli/rsync/discovery_remote_test.go (1)
12-104: LGTM!Excellent test coverage with table-driven tests covering:
- Happy path with multiple files in correct order
- Partial results (fewer files than limit)
- Empty directory edge case
- Single-quote escaping validation (security fix verification)
- Error propagation
The tests effectively validate both the functionality and the security fixes implemented in the remote discovery.
pkg/cli/rsync/copy_rsync.go (2)
31-34: LGTM - Struct fields added for --last support.The new
LastandfileDiscoveryfields are appropriately added to support the --last filtering feature.
114-120: LGTM - Command construction with --files-from.The conditional addition of
--files-from -whenin != nilis correct. The stdin reader is properly passed toLocalExecutor.Execute(), and the command structure follows rsync conventions.pkg/cli/rsync/copy_tar.go (3)
169-169: LGTM - tarRemote invocation updated correctly.The
tarRemotecall properly passes the newr.NoRecursionparameter.
222-252: LGTM - tarRemote function signature and implementation.The function signature extension and the conditional
--no-recursionflag insertion are implemented correctly. The flag is properly placed in the command construction for both path suffix cases.
244-249: Remove this comment; the concern is invalid.
DiscoverFilesreturns only basenames (seediscovery_local.goline 62:entry.Name()anddiscovery_remote.goline 50:path.Base(fullPath)), never nested paths. The local discoverer skips directories entirely (line 35), and the remote discoverer usesfind -maxdepth 1(line 32) to list only direct files. Therefore, nested paths likesubdir/file.txtwill never appear in the include list, making the concern about--no-recursionmoot. The--no-recursionflag placement is correct.Likely an incorrect or invalid review comment.
tchap
force-pushed
the
rsync-last-n-include
branch
from
b8bcada to
2d96602
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @pkg/cli/rsync/copy_rsync.go:
- Around line 95-112: Add a nil check before calling
r.fileDiscovery.DiscoverFiles to avoid panics when rsyncStrategy is directly
instantiated with Last > 0 but fileDiscovery is nil: inside the if r.Last > 0
block, verify r.fileDiscovery != nil and log or handle the condition (e.g.,
klog.Infof warning and skip filtering) before attempting DiscoverFiles;
reference r.fileDiscovery, DiscoverFiles, r.Last and rsyncStrategy to locate the
call site (or adjust Complete to ensure initialization) and ensure behavior
matches existing error handling for DiscoverFiles.
In @pkg/cli/rsync/copy_tar.go:
- Around line 48-67: o.fileDiscovery may be nil and noRecursion is set too
early; before calling o.fileDiscovery.DiscoverFiles check that o.fileDiscovery
!= nil and handle the nil case like a discovery failure, and when
o.Source.Local() is true set noRecursion = false (since --last is ignored for
local sources). Only set noRecursion = true when discovery actually succeeded
and you applied a filtered list (track a discoverySuccess boolean or similar),
and only replace includes and log the Applied --last message when filenames were
returned; on any discovery error or nil fileDiscovery leave noRecursion false
and log the warning including the error when present.
🧹 Nitpick comments (1)
pkg/cli/rsync/exec_test.go (1)
18-27: Consider checking the Write error for robustness.While ignoring the
Writeerror on Line 25 is acceptable in test code where writers are typically test buffers, checking and handling it would make the mock more robust.♻️ Optional improvement
- out.Write([]byte(m.output)) - return nil + if _, err := out.Write([]byte(m.output)); err != nil { + return err + } + return nil
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
pkg/cli/rsync/copy_rsync.gopkg/cli/rsync/copy_rsyncd.gopkg/cli/rsync/copy_tar.gopkg/cli/rsync/copy_tar_test.gopkg/cli/rsync/discovery.gopkg/cli/rsync/discovery_local.gopkg/cli/rsync/discovery_local_test.gopkg/cli/rsync/discovery_remote.gopkg/cli/rsync/discovery_remote_test.gopkg/cli/rsync/discovery_test.gopkg/cli/rsync/exec_test.gopkg/cli/rsync/rsync.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/cli/rsync/discovery.go
- pkg/cli/rsync/discovery_remote_test.go
- pkg/cli/rsync/discovery_remote.go
- pkg/cli/rsync/copy_tar_test.go
- pkg/cli/rsync/discovery_local.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/rsync/discovery_test.gopkg/cli/rsync/copy_rsyncd.gopkg/cli/rsync/copy_rsync.gopkg/cli/rsync/rsync.gopkg/cli/rsync/discovery_local_test.gopkg/cli/rsync/exec_test.gopkg/cli/rsync/copy_tar.go
🧬 Code graph analysis (1)
pkg/cli/rsync/copy_rsync.go (1)
pkg/cli/rsync/pathspec.go (1)
PathSpec(17-20)
🔇 Additional comments (19)
pkg/cli/rsync/exec_test.go (1)
10-16: LGTM!The mock type is well-designed with clear field names that make the test intentions obvious.
pkg/cli/rsync/discovery_local_test.go (2)
12-53: LGTM!The test setup is thorough, with proper cleanup, explicit modification times for deterministic ordering, and appropriate error handling.
54-97: LGTM!The test cases are comprehensive and well-structured, covering boundary conditions and validating the correct ordering of results. The table-driven approach enhances maintainability.
pkg/cli/rsync/discovery_test.go (1)
3-14: LGTM!The mock implementation is appropriately simple and correctly supports both success and error scenarios for testing.
pkg/cli/rsync/rsync.go (6)
44-59: LGTM!The documentation and example clearly explain the new
--lastflag functionality with a practical use case.
95-95: LGTM!The
Lastfield is appropriately typed asuintfor representing a count of files.
112-114: LGTM!The
fileDiscoveryfield is appropriately unexported with clear documentation explaining its purpose for testability.
142-142: LGTM!The flag registration is correct with a clear description and appropriate default value.
237-245: LGTM!The file discovery initialization correctly selects the appropriate discoverer based on whether the source is local or remote, and is properly positioned before strategy initialization.
276-287: LGTM!The validation correctly enforces logical incompatibilities between
--lastand other flags, with clear error messages.pkg/cli/rsync/copy_rsync.go (4)
8-8: LGTM!The
path/filepathimport is necessary for the destination path adjustments when using--files-from.
32-33: LGTM!The new fields are appropriately unexported and will be populated from
RsyncOptions.
82-83: LGTM!The fields are correctly propagated from
RsyncOptionsto the strategy.
115-118: LGTM!The command construction correctly adds
--files-from -when a file list is provided, passing it via stdin.pkg/cli/rsync/copy_tar.go (2)
33-33: LGTM!The
NoRecursionfield addition totarStrategyis appropriate for controlling tar's recursion behavior when--lastis specified.
224-255: LGTM!The
tarRemotesignature extension and implementation correctly handle thenoRecursionparameter. The--no-recursionflag is appropriately placed in the tar command construction when needed.pkg/cli/rsync/copy_rsyncd.go (3)
71-72: LGTM!The
LastandfileDiscoveryfield additions torsyncDaemonStrategyappropriately enable--lastsupport in the rsync-daemon strategy.
338-339: LGTM!The initialization of
LastandfileDiscoveryfields correctly wires in the values fromRsyncOptions.
238-255: Nil pointer dereference risk remains unaddressed.Line 239 calls
s.fileDiscovery.DiscoverFiles()without verifying thats.fileDiscoveryis non-nil, which can cause a panic.As suggested in the previous review, add a defensive nil check:
if s.Last > 0 && s.fileDiscovery != nil { filenames, err := s.fileDiscovery.DiscoverFiles(source.Path, s.Last) // ... rest of the logic } else if s.Last > 0 { klog.Infof("Warning: --last specified but file discovery not available") }Likely an incorrect or invalid review comment.
The flag can be used to only sync N most recently modified files. The flag is mutually exclusive to: * --include * --exclude * --delete * --watch It is also ignored when the source is a local directory when using tar, because the implementation doesn't allow to select particular files. Generally when there is any problem when using --last, the error is ignored and sync happens as if the flag was not specified. Regarding implementation details, oc rsync performs an extras step when --last is specified, and that is discovering relevant files to select. This is done using manual directory walking when local, for remote the remote executor is used to invoke a shell using find+sort+head. The resulting filenames are then passed to --files-from for rsync, for tar they are simply passed to the command as arguments. Tests were added for testing the discovery mechanism, the rest has been tested manually. oc rsync is poorly unit-tested in general. Assisted-by: Claude Code
tchap
force-pushed
the
rsync-last-n-include
branch
from
2d96602 to
0062ecf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/cli/rsync/copy_tar_test.go (1)
66-66: Consider adding type assertion safety check.The unchecked type assertion could panic if
NewTarStrategyunexpectedly returns a different implementation. While test panics clearly signal issues, a two-value assertion with an error message would provide clearer diagnostics.♻️ Safer type assertion
- strategy := NewTarStrategy(options).(*tarStrategy) + strategy, ok := NewTarStrategy(options).(*tarStrategy) + if !ok { + t.Fatal("NewTarStrategy did not return a *tarStrategy") + }pkg/cli/rsync/copy_tar.go (1)
48-67: Add defensive nil check for fileDiscovery.While the normal execution flow ensures
o.fileDiscoveryis initialized inComplete()whenLast > 0, adding a nil check before line 55 would make the code more robust against direct instantiation or initialization order issues.♻️ Recommended defensive check
if o.Last > 0 { if o.Source.Local() { klog.Info("Warning: --last flag is ignored when creating a local tar file") - } else { + } else if o.fileDiscovery != nil { filenames, err := o.fileDiscovery.DiscoverFiles(o.Source.Path, o.Last) if err != nil { klog.Infof("Warning: failed to apply --last filtering: %v", err) } else { // Replace any existing includes with our filtered list. if len(filenames) > 0 { includes = filenames noRecursion = true klog.V(3).Infof("Applied --last=%d to tar strategy: using %d files", o.Last, len(filenames)) } } + } else { + klog.Info("Warning: --last flag ignored due to missing file discovery") } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
pkg/cli/rsync/copy_rsync.gopkg/cli/rsync/copy_rsyncd.gopkg/cli/rsync/copy_tar.gopkg/cli/rsync/copy_tar_test.gopkg/cli/rsync/discovery.gopkg/cli/rsync/discovery_local.gopkg/cli/rsync/discovery_local_test.gopkg/cli/rsync/discovery_remote.gopkg/cli/rsync/discovery_remote_test.gopkg/cli/rsync/discovery_test.gopkg/cli/rsync/exec_test.gopkg/cli/rsync/rsync.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cli/rsync/discovery_test.go
- pkg/cli/rsync/discovery.go
- pkg/cli/rsync/discovery_remote_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/rsync/discovery_local.gopkg/cli/rsync/rsync.gopkg/cli/rsync/discovery_remote.gopkg/cli/rsync/copy_tar.gopkg/cli/rsync/copy_rsyncd.gopkg/cli/rsync/exec_test.gopkg/cli/rsync/discovery_local_test.gopkg/cli/rsync/copy_rsync.gopkg/cli/rsync/copy_tar_test.go
🧬 Code graph analysis (3)
pkg/cli/rsync/copy_tar.go (1)
pkg/helpers/source-to-image/tar/tar.go (2)
Tar(32-73)Writer(82-86)
pkg/cli/rsync/copy_rsync.go (1)
pkg/cli/rsync/pathspec.go (1)
PathSpec(17-20)
pkg/cli/rsync/copy_tar_test.go (3)
pkg/cli/rsync/rsync.go (1)
RsyncOptions(88-115)pkg/cli/rsync/pathspec.go (1)
PathSpec(17-20)pkg/cli/rsync/copy_tar.go (1)
NewTarStrategy(39-80)
🔇 Additional comments (11)
pkg/cli/rsync/exec_test.go (1)
10-27: LGTM!The
mockExecutortest helper is well-designed for validating command construction in rsync tests. The use ofgo-cmpfor command comparison provides clear diffs when expectations are not met.pkg/cli/rsync/copy_tar_test.go (1)
11-77: Well-structured test coverage for file discovery integration.The table-driven test comprehensively validates how
NewTarStrategyhandles file discovery across multiple scenarios, including error cases and empty results.pkg/cli/rsync/discovery_local_test.go (1)
12-98: Excellent test coverage with deterministic setup.The test thoroughly validates the local file discovery mechanism:
- Explicit modification time setup ensures deterministic ordering
- Subdirectory creation verifies non-file entries are correctly filtered
- Multiple scenarios confirm proper limiting and ordering behavior
pkg/cli/rsync/discovery_local.go (1)
19-65: Solid implementation of local file discovery.The non-recursive design is appropriate: it discovers files in the immediate directory, letting rsync handle the tree traversal. Key strengths:
- Proper filtering of directories
- Comprehensive error handling for both directory reads and file info retrieval
- Efficient sorting newest-first
- Memory-bounded by directory size
pkg/cli/rsync/rsync.go (4)
95-95: Clean field additions toRsyncOptions.The
Lastfield uses an appropriate type (uint) for a count, and thefileDiscoveryfield includes a helpful comment explaining its presence for testability.Also applies to: 112-114
44-45: Clear documentation for the new--lastflag.The help text concisely explains the feature, and the example demonstrates practical usage.
Also applies to: 58-59
237-245: Proper initialization sequence for file discovery.The
fileDiscoveryfield is correctly initialized before strategy creation, with the appropriate discoverer type selected based on whether the source is local or remote.
276-287: Comprehensive validation of--lastflag compatibility.The mutual exclusivity checks correctly prevent combining
--lastwith--watch,--include,--exclude, and--delete, all of which would conflict with the discovery-based filtering approach. Error messages are clear.pkg/cli/rsync/discovery_remote.go (1)
22-60: LGTM! Past review concerns have been addressed.The implementation correctly:
- Escapes single quotes in
basePath(line 30) to prevent command injection- Uses
path.Base(line 50) instead offilepath.Basefor remote POSIX pathsThe shell pipeline and parsing logic are sound for discovering the N most recently modified files from a remote directory.
pkg/cli/rsync/copy_rsync.go (1)
90-112: LGTM! The --last filtering implementation is correct.The logic properly:
- Discovers files when
Last > 0- Handles discovery errors gracefully (logs warning and proceeds without filtering)
- Builds the file list for rsync's
--files-fromoption- Adjusts the destination path to match rsync's expected behavior
The past review's suggestion about adding a defensive nil check for
fileDiscoveryis valid for robustness, though normal initialization throughComplete()ensures it's non-nil whenLast > 0.pkg/cli/rsync/copy_rsyncd.go (1)
228-255: LGTM! The --last filtering implementation mirrors the rsync strategy correctly.The logic properly:
- Computes the appropriate destination path based on whether it's local or remote
- Discovers files when
Last > 0- Handles discovery errors gracefully with warning logs
- Builds the file list for
--files-fromand adjusts the destination accordinglyAs with
copy_rsync.go, the normal initialization flow throughComplete()ensuresfileDiscoveryis non-nil when needed, though a defensive nil check would add robustness.
|
/retest |
|
@tchap: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
The flag can be used to only sync N most recently modified files.
The flag is mutually exclusive to:
--include--exclude--delete--watchIt is also ignored when the source is a local directory when using tar, because the implementation doesn't allow to select particular files. We would need to exclude all other files, which can explode easily. Simply use a different strategy in that case.
Generally when there is any problem when using
--last, the error is ignored and sync happens as if the flag was not specified.Regarding implementation details,
oc rsyncperforms an extras step when--lastis specified, and that is discovering relevant files to select. This is done using manual directory walking when local, for remote the remote executor is used to invoke a shell using find+sort+head.The resulting filenames are then passed to
--files-fromfor rsync, for tar they are simply passed to the command as arguments.Tests were added for testing the discovery mechanism, the rest has been tested manually.
oc rsyncis rather poorly unit-tested in general.