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

Generate for each role own key #1028

Merged
merged 5 commits into from
Dec 14, 2018
Merged

Generate for each role own key #1028

merged 5 commits into from
Dec 14, 2018

Conversation

patriotyk
Copy link
Contributor

No description provided.

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.

So far, so good, I think.

src/aktualizr_repo/repo.h Outdated Show resolved Hide resolved
class Repo {
public:
Repo(boost::filesystem::path path, const std::string &expires, std::string correlation_id);
Repo(std::string repo_type, boost::filesystem::path path, const std::string &expires, std::string correlation_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be nice if repo_type was an enum class, although since you use the actual text for various reasons, perhaps it makes sense to have conversion functions to strings.

boost::filesystem::path path_;
std::string correlation_id_;
std::string expiration_time_;
std::map<std::string, KeyPair> keys_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another thought: instead of using a string as the key of this map, would it be better to use a KeyType?

const boost::filesystem::path current = path_ / "repo/director/targets.json";
const boost::filesystem::path staging = path_ / "repo/director/staging/targets.json";

//
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing comment?

src/aktualizr_repo/director_repo.cc Show resolved Hide resolved
@patriotyk patriotyk force-pushed the feat/aktualizr-repo-keys branch from b8a989e to bdd29fa Compare December 11, 2018 13:36
@codecov-io
Copy link

codecov-io commented Dec 11, 2018

Codecov Report

Merging #1028 into master will increase coverage by 0.11%.
The diff coverage is 88.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1028      +/-   ##
==========================================
+ Coverage   82.31%   82.43%   +0.11%     
==========================================
  Files         189      195       +6     
  Lines       13663    13790     +127     
==========================================
+ Hits        11247    11368     +121     
- Misses       2416     2422       +6
Impacted Files Coverage Δ
src/aktualizr_secondary/update_test.cc 15.38% <0%> (ø) ⬆️
src/aktualizr_secondary/aktualizr_secondary.cc 22.82% <0%> (-0.26%) ⬇️
src/aktualizr_info/main.cc 63.54% <100%> (ø) ⬆️
src/libaktualizr/uptane/imagesrepository.h 100% <100%> (ø) ⬆️
src/libaktualizr/storage/sqlstorage.cc 74.96% <100%> (ø) ⬆️
src/aktualizr_repo/image_repo.cc 100% <100%> (ø)
src/aktualizr_repo/image_repo.h 100% <100%> (ø)
src/libaktualizr/storage/storage_common_test.cc 98.18% <100%> (ø) ⬆️
src/libaktualizr/uptane/fetcher.cc 97.16% <100%> (ø) ⬆️
src/libaktualizr/uptane/directorrepository.h 100% <100%> (ø) ⬆️
... and 31 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 b5d105c...c10c7ed. Read the comment docs.

@patriotyk patriotyk force-pushed the feat/aktualizr-repo-keys branch 4 times, most recently from e20c212 to 8a6e695 Compare December 11, 2018 17:19
Signed-off-by: Serhiy Stetskovych <patriotyk@gmail.com>
@patriotyk patriotyk force-pushed the feat/aktualizr-repo-keys branch from 8a6e695 to e90f1e7 Compare December 11, 2018 22:10
Signed-off-by: Serhiy Stetskovych <patriotyk@gmail.com>
@pattivacek
Copy link
Collaborator

Looks promising. I like the enum classes! I just realized that RepoType is quite similar to the RepositoryType in src/libaktualizr/uptane/tuf.h, but this version is actually probably better. We should consider merging them.

What's missing is testing:

  1. We should unit test both of the new features, and
  2. We should find an existing unit test using hardcoded uptane metadata and upgrade it to instead use the new functionality of aktualizr-repo to dynamically generate the metadata.

@OYTIS I'm also curious if this matches your expectations so far as well!

Signed-off-by: Serhiy Stetskovych <patriotyk@gmail.com>
@patriotyk patriotyk force-pushed the feat/aktualizr-repo-keys branch from c1e65d6 to ef59fce Compare December 12, 2018 15:57
Signed-off-by: Serhiy Stetskovych <patriotyk@gmail.com>
Signed-off-by: Serhiy Stetskovych <patriotyk@gmail.com>
@patriotyk patriotyk force-pushed the feat/aktualizr-repo-keys branch from 7f9cbd4 to c10c7ed Compare December 14, 2018 00:17
@patriotyk
Copy link
Contributor Author

Looks like it should be ok now.

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 good, thanks!

@pattivacek pattivacek merged commit 6956afc into master Dec 14, 2018
@pattivacek pattivacek deleted the feat/aktualizr-repo-keys branch December 14, 2018 10:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants