-
Notifications
You must be signed in to change notification settings - Fork 61
aktualizr-info add parameter '--images-snapshot' and '--images-timest… #1207
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1207 +/- ##
==========================================
+ Coverage 76.75% 76.78% +0.02%
==========================================
Files 168 168
Lines 9993 10005 +12
==========================================
+ Hits 7670 7682 +12
Misses 2323 2323
Continue to review full report at Codecov.
|
std::string images_snapshot = Utils::jsonToStr(meta_snapshot); | ||
db_storage_->storeNonRoot(images_snapshot, Uptane::RepositoryType::Image(), Uptane::Role::Snapshot()); | ||
|
||
aktualizr_info_process_.run(std::vector<std::string>{"--snapshot"}); |
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.
Is it really necessary to do an explicit casting here (std::vectorstd::string) ? If not, then it can be removed.
std::string images_timestamp = Utils::jsonToStr(meta_timestamp); | ||
db_storage_->storeNonRoot(images_timestamp, Uptane::RepositoryType::Image(), Uptane::Role::Timestamp()); | ||
|
||
aktualizr_info_process_.run(std::vector<std::string>{"--timestamp"}); |
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 same as above, explicit casting can be avoided (std::vectorstd::string)
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.
Thank you Mike, I believe you are right, they should be removed.
Two minor issues that I see:
Thanks for the work! I've tested it and it works 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.
I am fine with this specific change/code. I do have some concerns/suggestion around aktualizr-info in general but it's out of scope of this PR.
src/aktualizr_info/main.cc
Outdated
("allow-migrate", "Opens database in read/write mode to make possible to migrate database if needed"); | ||
("allow-migrate", "Opens database in read/write mode to make possible to migrate database if needed") | ||
("images-snapshot", "Outputs snapshot.json from images repo") | ||
("images-timestamp", "Outputs timestamp.json from images repo"); |
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.
Very small complaint: could you move these two items between "images-root" and "images-target"? The ideal order would be: root, timestamp, snapshot, targets. That more or less matches the order they are downloaded by libaktualizr and the order in which they sign each other.
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.
Of cause. I moved them.
…amp' Signed-off-by: cheng.xiang <[email protected]>
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.
Thank you! Travis is failing due to a codecov timeout; I'm merging despite that since it isn't relevant to your changes.
aktualizr-info add parameter '--images-snapshot' and '--images-timestamp'