Clean all targets #1290
Clean all targets #1290
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1290 +/- ##
==========================================
- Coverage 79.2% 79.19% -0.01%
==========================================
Files 175 175
Lines 10359 10384 +25
==========================================
+ Hits 8205 8224 +19
- Misses 2154 2160 +6
Continue to review full report at Codecov.
|
CHANGELOG.md
Outdated
|
|
||
| ### Changed | ||
|
|
||
| - Renamed `GetStoredTarget` to `OpenStoredTarget` in aktualizr api |
There was a problem hiding this comment.
Minor, but I'd capitalize "API", and now that you have a PR, you can already fill in the links.
| @@ -494,6 +494,47 @@ TEST(storage, store_target) { | |||
| } | |||
| } | |||
|
|
|||
| TEST(storage, list_remove_targets) { | |||
There was a problem hiding this comment.
I'd like a brief comment explaining the precise features you're trying to test.
(I've given up on maintaining the actions list these days, since no one seems to care about it, but I still think the comments for each test that clarify the important points of the test are very useful.)
There was a problem hiding this comment.
Yes that's too bad actually. Part of the problem might be that it's a bit hard to keep track as it needs to be changed in two places.
If we could just define them where they are tested and have an automated tool to collect them, it would be a bit more helpful. But then, it would maybe take some non-trivial effort.
There was a problem hiding this comment.
If we could just define them where they are tested and have an automated tool to collect them, it would be a bit more helpful. But then, it would maybe take some non-trivial effort.
100% agreed, and it was my hope that we would eventually do that. The one problem I foresaw was how to structure the information, though. That was the nice thing about the list. In any case, it probably isn't hard to read the comments around the tests; we started doing something similar with zephyr integration, but that also got stalled.
| return true; | ||
| } | ||
|
|
||
| std::unique_ptr<StorageTargetRHandle> Aktualizr::OpenStoredTarget(const Uptane::Target &target) { |
There was a problem hiding this comment.
While we're at it, can we change the return type to something like std::ofstream? This is an external API, and IMO we should try to expose less internal/implementation-specific details here.
There was a problem hiding this comment.
Yes, probably a good idea since we don't store the files in database, so the abstraction is not needed now.
I would leave that for another task/PR though.
| try { | ||
| auto handle = storage_->openTargetFile(target); | ||
| if (handle->isPartial()) { | ||
| return std::unique_ptr<StorageTargetRHandle>(); | ||
| } | ||
| return storage_->openTargetFile(target); | ||
| return handle; |
There was a problem hiding this comment.
Maybe it would be better to throw an exception if somebody tries to open a not or partially downloaded target?
| bool Aktualizr::DeleteStoredTarget(const Uptane::Target &target) { | ||
| try { | ||
| storage_->removeTargetFile(target.filename()); | ||
| } catch (...) { |
There was a problem hiding this comment.
Probably we should discuss that again, but haven't we decided to let exceptions propagate through the C++ API level? That's also what other API calls do.
| try { | ||
| boost::filesystem::remove(images_path_ / filename); | ||
| } catch (std::exception& e) { | ||
| LOG_ERROR << "Could not remove target file"; |
There was a problem hiding this comment.
Only printing an error seems insufficient to me. Maybe we should rethrow?
lbonn
force-pushed
the
feat/OTA-2151/clean-old-targets
branch
from
dd6cd68
to
45c2f69
Compare
Makes it clearer as we will add methods to list targets currently in storage Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
It has nothing to do with SqlStorageBase Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
Was a bit buggy Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
Use lambdas with closures instead Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
lbonn
force-pushed
the
feat/OTA-2151/clean-old-targets
branch
2 times, most recently
from
90db743
to
2e24889
Compare
lbonn
requested review from
pattivacek,
eu-siemann,
kbushgit,
mike-sul,
xcheng-here and
Zee314159
Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
lbonn
force-pushed
the
feat/OTA-2151/clean-old-targets
branch
from
2e24889
to
9d2ee15
Compare
|
@eu-smirnov I think I've addressed your comments. |
Add api calls to list and delete targets in storage.
Tests in test_aktualizr forthcoming.