-
Notifications
You must be signed in to change notification settings - Fork 45
Produce map of items that need backup details entries #1892
Conversation
4551fcb
to
682b7af
Compare
if d.info == nil { | ||
// TODO(ashmrtn): We should probably be returning an error here? | ||
if d.prevPath == nil { | ||
return |
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.
Add an error log here?
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.
we don't have access to a context here so unless I stash a logger in the struct instance there's not a whole lot we can do :/
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.
The logger will fall back to a singleton instance if given the background context. You might miss out on some logging within tests, but for the CLI it'll still write out as expected.
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.
thinking about this more broadly, I think we should be storing errors in the struct and returning them at the end. Otherwise we could miss the fact that we failed to add some details. I'll make a follow up ticket for this
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.
tracking with #1915
This is mostly for streaming base directory entries since the path is not immediately available for them. Will be used to produce metadata required to merge backup details from base snapshots.
Add more metadata like the previous path to progress entries and track entries for items in the base snapshot. This will allow adding details entries for base snapshot items eventually.
src/internal/kopia/upload.go
Outdated
@@ -183,6 +179,30 @@ func (cp *corsoProgress) FinishedFile(relativePath string, err error) { | |||
} | |||
|
|||
cp.deets.AddFolders(folders) |
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.
Following up on the slack discussion - where we've changed this to AddFoldersForItem
in main
. I think we should just defer the AddFoldersForItem
for an item that is being sourced from the base snapshot to when it is being "merged into details"
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.
yea, that makes the most sense. A bit of a bummer cause I wanted to keep the code for building the set of directories limited to as few places as possible. Guess I'll move it to a helper function somewhere
Use a map[old shortref]new path.Path to track entries sourced from the base snapshot. kopia.Wrapper can't directly add the ItemInfo for these to the Details, but it can return the map to something that can. Also always add entries for the folders of an item.
Return the map[old shortref]new path.Path fo items sourced from a base snapshot. This will give BackupOp the information it needs to source those ItemInfo entries from base snapshots.
Aviator status
This PR was merged using Aviator. |
PR failed to merge with reason: some CI status(es) failed. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Description
Generate a
map[old ShortRef]new path.Path
for every item sourced from a base snapshot during backup. Return this information at the end of BackupCollections so that callers can use it to merge backup details if desired.This PR does a few auxiliary things as well:
Does this PR need a docs update or release note?
Type of change
Issue(s)
Test Plan