-
Notifications
You must be signed in to change notification settings - Fork 341
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
feat(cli): add more info to "snapshot create" json #3294
base: master
Are you sure you want to change the base?
Conversation
@@ -286,6 +286,19 @@ func (c *commandSnapshotCreate) snapshotSingleSource(ctx context.Context, fsEntr | |||
manifest.Tags = tags | |||
manifest.UpdatePins(c.pins, nil) | |||
|
|||
progress := c.svc.getProgress() | |||
|
|||
manifest.Progress = map[string]int64{ |
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.
I've listed all integer fields of cliProgress
here, except inProgressHashing
because it is "in progress" and thus not interesting to expose to the json
@@ -336,7 +349,7 @@ func (c *commandSnapshotCreate) snapshotSingleSource(ctx context.Context, fsEntr | |||
} | |||
} | |||
|
|||
c.svc.getProgress().Finish() | |||
progress.Finish() |
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.
not totally sure if I should first Finish()
before I access the values in progress
. to me it seems like that the Finish
function only changes what is displayed in the cli but doesn't affect the integer values, thus I think it doesn't matter when Finish
is called
Description string `json:"description"` | ||
StartTime fs.UTCTimestamp `json:"startTime"` | ||
EndTime fs.UTCTimestamp `json:"endTime"` | ||
Progress map[string]int64 `json:"progress"` |
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.
I wasn't sure how exactly to expose progress
here. Would it be better to introduce a new struct? Can I somehow directly use the cliProgress
struct? Is using a map acceptable 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.
doesn't Stats
below have what you need?
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.
Stats
doesn't contain an uploadedBytes field. The only byte fields it has are TotalFileSize
and ExcludedTotalFileSize
. What would be the best option to continue? For example, I could try to add the new fields UploadedTotalFileSize
and CachedTotalFileSize
to Stats
. Or I could keep the map that I introduced. Or I could create a new Stats struct that is only used to report the data in the json file. Thoughts :)?
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.
Let's do Stats. I would prefer to be intentional and only add 1 or 2 field as needed, I know some folks manage millions of manifests and bloat can already be a problem.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3294 +/- ##
==========================================
+ Coverage 75.74% 75.75% +0.01%
==========================================
Files 461 461
Lines 36960 36973 +13
==========================================
+ Hits 27994 28010 +16
- Misses 7036 7037 +1
+ Partials 1930 1926 -4
☔ View full report in Codecov by Sentry. |
I'd like to expose how many bytes were uploaded to the json of
kopia snapshot create --json
. I tested my diff and it works as expected.I think this diff is rather hacky. I am motivated to improve this diff, if you let me know what you would like me to change. I wasn't sure how to cleanly implement this and I am also new to Go.