Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Make path to downloaded binaries separately configurable #1679

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

eu-siemann
Copy link
Contributor

Main changes:

  • Move binary target handling logic from Storage class to package manager
  • Add a configuration option for the path to downloaded files
  • Use std::ifstream in public API instead of StorageRHandle

Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks quite good to me!

@@ -17,7 +17,7 @@ void targets_autoclean_cb(Aktualizr &aktualizr, const std::shared_ptr<event::Bas
auto start = entry.installs.size() >= 2 ? entry.installs.end() - 2 : entry.installs.begin();
for (auto it = start; it != entry.installs.end(); it++) {
auto fit = std::find_if(installed_targets.begin(), installed_targets.end(),
[&it](const Uptane::Target &t2) { return it->sha256Hash() == t2.sha256Hash(); });
[&it](const Uptane::Target &t2) { return it->filename() == t2.filename(); });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we guarantee that filename will be unique? I'm wondering if we should match more than just that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the target name and I think there are a lot of places where we rely on it being unique.

std::vector<Uptane::Target> getStoredTargets() const { return package_manager_->getTargetFiles(); }
void deleteStoredTarget(const Uptane::Target &target) { package_manager_->removeTargetFile(target); }
std::ifstream openStoredTarget(const Uptane::Target &target) {
auto status = package_manager_->verifyTarget(target);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

src/libaktualizr/storage/sqlstorage.cc Outdated Show resolved Hide resolved
src/libaktualizr/storage/storage_common_test.cc Outdated Show resolved Hide resolved
src/libaktualizr/uptane/uptane_test.cc Show resolved Hide resolved
Eugene Smirnov added 2 commits June 9, 2020 15:06
Signed-off-by: Eugene Smirnov <evgenii.smirnov@here.com>
Signed-off-by: Eugene Smirnov <evgenii.smirnov@here.com>
@eu-siemann
Copy link
Contributor Author

I've added a few unit tests to cover refactored methods and hopefully solved the remaining CI issues.

Signed-off-by: Eugene Smirnov <evgenii.smirnov@here.com>
Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me!


std::vector<Uptane::Target> PackageManagerInterface::getTargetFiles() {
std::vector<Uptane::Target> v;
auto names = storage_->getAllTargetNames();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return just the names as strings from getAllTargetNames if we just construct fake Targets here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean why not construct fake targets in the storage method, or why construct them at all?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both, I guess. It looks like previously we were constructing Targets in SqlStorage::getTargetFiles() based on the hashes that were stored in the database. What's the benefit of losing that information?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no benefit in storing this information. Those Targets were still fake and used only for bookkeeping, as the comment in API header states:

Get list of targets currently in storage. This is intended to be used with
DeleteStoredTarget and targets are not guaranteed to be verified and
up-to-date with current metadata.

For bookkeeping, it is sufficient to have only a filename.
I also think it is cleaner to keep in Storage only that logic that is directly related to reading/storing necessary information from db.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, thanks for clarifying!

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2020

Codecov Report

Merging #1679 into master will increase coverage by 15.95%.
The diff coverage is 92.24%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1679       +/-   ##
===========================================
+ Coverage   66.79%   82.75%   +15.95%     
===========================================
  Files         190      190               
  Lines       15066    12072     -2994     
===========================================
- Hits        10064     9990       -74     
+ Misses       5002     2082     -2920     
Impacted Files Coverage Δ
...ibaktualizr/package_manager/packagemanagerconfig.h 100.00% <ø> (ø)
...ktualizr/package_manager/packagemanagerinterface.h 80.00% <ø> (ø)
src/libaktualizr/primary/aktualizr.h 100.00% <ø> (ø)
src/libaktualizr/storage/invstorage.h 100.00% <ø> (+7.69%) ⬆️
src/libaktualizr/storage/sqlstorage.h 50.00% <ø> (+16.66%) ⬆️
src/libaktualizr/storage/sqlstorage.cc 79.27% <80.95%> (+33.75%) ⬆️
...tualizr/package_manager/packagemanagerinterface.cc 92.81% <92.50%> (+11.75%) ⬆️
src/libaktualizr-c/libaktualizr-c.cc 56.58% <100.00%> (ø)
...baktualizr-c/test/api-test-utils/api-test-utils.cc 95.83% <100.00%> (+22.50%) ⬆️
src/libaktualizr/package_manager/debianmanager.cc 100.00% <100.00%> (ø)
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c73f2c2...0d43dc9. Read the comment docs.

@eu-siemann eu-siemann merged commit 6dfc797 into master Jun 10, 2020
@eu-siemann eu-siemann deleted the refact/bin-files branch June 10, 2020 08:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants