feat(cli): helper functions to get preceding snapshots for diff
#4559
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4559 +/- ##
==========================================
+ Coverage 75.86% 76.39% +0.53%
==========================================
Files 470 527 +57
Lines 37301 40091 +2790
==========================================
+ Hits 28299 30629 +2330
- Misses 7071 7447 +376
- Partials 1931 2015 +84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
diff
There was a problem hiding this comment.
Pull Request Overview
This PR introduces helper functions to fetch preceding snapshots for the diff command, enhancing the CLI functionality of kopia diff.
- Added helper functions (GetPreceedingSnapshot and GetTwoLatestSnapshotsForASource) in diff.go to retrieve snapshots based on timestamps.
- Updated usage in snapshotfs/objref.go to export the FindSnapshotByRootObjectIDOrManifestID function and ensure consistency.
- Added comprehensive unit tests in diff_test.go to validate error conditions and proper snapshot ordering.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| snapshot/snapshotfs/objref.go | Updated exported function name and comment capitalization for clarity. |
| internal/diff/diff.go | Introduced two new helper functions for fetching snapshots and sorting them. |
| internal/diff/diff_test.go | Added unit tests covering error handling and snapshot order retrieval. |
Comments suppressed due to low confidence (1)
internal/diff/diff.go:413
- The function name 'GetPreceedingSnapshot' appears to be misspelled; consider renaming it to 'GetPrecedingSnapshot' to match the PR title and standard spelling.
func GetPreceedingSnapshot(ctx context.Context, rep repo.Repository, snapshotID string) (*snapshot.Manifest, error) {
| // - Add a snapshot, expect an error from GetPreceedingSnapshot since there is | ||
| // only a single snapshot in the repo; | ||
| // - Subsequently add more snapshots and GetPreceedingSnapshot theimmediately | ||
| // preceding with no error. |
There was a problem hiding this comment.
@Rohit-BM18 Added comments to this test and the one below. PTAL.
| latestSnapshotManifestID := mustSaveSnapshot(t, env.RepositoryWriter, manifests["latest_snapshot"]) | ||
| gotManID2, err := diff.GetPrecedingSnapshot(ctx, env.RepositoryWriter, string(latestSnapshotManifestID)) | ||
| require.NoError(t, err) | ||
| require.Equal(t, intermediateSnapshotManifestID, gotManID2.ID) |
There was a problem hiding this comment.
What is the expected behavior when a single snapshot is added for a different snapshot source ({host,user,path} tuple)? when:
- getting the 2 most recent snapshots for the source that has a single snapshot
- getting the 2 most recent snapshots for the source that has more than one snapshot
Can you create another PR with these test cases please?
Thanks.
There was a problem hiding this comment.
When either of the methods are invoked on a source with insufficient snapshots, it should result in an error.
PR incoming with these tests.
Added helper functions to find preceding snapshots.
Part of the planned enhancements for the
diffcommand.Added the helper methods to fetch preceding snapshots. These helpers will be consumed to enhance the CLI for the existing
diffcommand implementation.Also added relevant unit tests for the above methods:
GetPrecedingSnapshot, first the method is invoked with a wrongsnapshotIDand an error is expected. Next a snapshot is saved and the method is invoked again, since there is only a single snapshot in the repo, an error is expected. Subsequently more snapshots are saved and the method is invoked and the immediately preceding snapshot based on timestamp is expectedGetTwoLatestSnapshotsForASource, a similar strategy is used. The method is invoked with insufficient snapshots stored in the repo and errors are expected. Eventually more snapshots are saved and when the method is invoked the two most recent manifests for snapshot based on start time are expected