[snapshots] record snapshot creation time #3125
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## snapshots #3125 +/- ##
=============================================
- Coverage 84.21% 84.13% -0.08%
=============================================
Files 246 246
Lines 13152 13164 +12
=============================================
Hits 11076 11076
- Misses 2076 2088 +12
|
ricab
requested changes
Jun 22, 2023
| : BaseSnapshot{json["name"].toString().toStdString(), // name | ||
| json["comment"].toString().toStdString(), // comment | ||
| QDateTime::fromString(json["creation_timestamp"].toString(), | ||
| "yyyy-MM-ddTHH:mm:ss.zzzZ"), // creation_timestamp |
There was a problem hiding this comment.
Isn't this pretty much the same as Qt::ISODate? In any case, can you extract a constant please?
ricab
force-pushed
the
record-snapshot-creation-time
branch
from
ad8652b
to
cf24856
Compare
sharder996
force-pushed
the
record-snapshot-creation-time
branch
from
cf24856
to
d4809df
Compare
ricab
force-pushed
the
record-snapshot-creation-time
branch
from
d4809df
to
cf24856
Compare
sharder996
force-pushed
the
record-snapshot-creation-time
branch
from
cf24856
to
d234eb1
Compare
ricab
reviewed
Jun 27, 2023
|
|
||
| private: | ||
| std::string name; | ||
| std::string comment; | ||
| const QDateTime creation_timestamp; |
There was a problem hiding this comment.
All other const fields are below, after a comment explaining them. Is there a reason why you need this order? Otherwise, could you move the timestamp there?
sharder996
force-pushed
the
record-snapshot-creation-time
branch
from
e65404d
to
d5037b4
Compare
ricab
reviewed
Jun 28, 2023
There was a problem hiding this comment.
Hey @sharder996, one final detail that I missed before. And this needs a rebase now.
| @@ -134,6 +145,7 @@ QJsonObject multipass::BaseSnapshot::serialize() const | |||
|
|
|||
| snapshot.insert("name", QString::fromStdString(name)); | |||
| snapshot.insert("comment", QString::fromStdString(comment)); | |||
| snapshot.insert("creation_timestamp", creation_timestamp.toString(Qt::ISODate)); | |||
There was a problem hiding this comment.
I just realized this was dropping milliseconds. There is a Qt::ISODateWithMs which is closer (same?) to what you had initially.
There was a problem hiding this comment.
BTW, I believe this explains the wrong order after loading snapshots that were taken in quick succession:
$ mp snapshot b -n foo && mp snapshot a -n bar && mp snapshot b -n bar && mp snapshot b -n baz && mp snapshot a -n baz && mp snapshot a -n foo
Snapshot taken: b.foo
Snapshot taken: a.bar
Snapshot taken: b.bar
Snapshot taken: b.baz
Snapshot taken: a.baz
Snapshot taken: a.foo
$ multipass info --snapshot-overview --all Instance Snapshot Parent Comment
a bar -- --
a baz bar --
a foo baz --
b foo -- --
b bar foo --
b baz bar --
$ snap stop multipass.multipassd
2023-06-28T12:43:29+01:00 INFO Waiting for "snap.multipass.multipassd.service" to stop.
Stopped.
$ snap start multipass.multipassd
Started.
$ multipass info --snapshot-overview --all
Instance Snapshot Parent Comment
a bar -- --
a foo baz --
a baz bar --
b foo -- --
b baz bar --
b bar foo --
sharder996
force-pushed
the
record-snapshot-creation-time
branch
from
d5037b4
to
976eb07
Compare
ricab
added a commit
that referenced
this pull request
Aug 29, 2023
[snapshots] record snapshot creation time
Merged
ricab
added a commit
that referenced
this pull request
Oct 25, 2023
[snapshots] record snapshot creation time
ricab
added a commit
that referenced
this pull request
Oct 30, 2023
[snapshots] record snapshot creation time
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Debated the use of
QDateTimevsstd::chrono::time_point, but some functions that would be useful for serializing the object to a string and vice versa are only available in C++20. You can still make it work with the C++17 STL, but you will be forced to usestd::time_tin places. Depending on the platform,std::time_tcan be only 32 bits, meaning that its maximmum date is sometime in 2038.