-
Notifications
You must be signed in to change notification settings - Fork 61
OTA-2320: Add an option to aktualizr-info to print delegations metadata #1138
OTA-2320: Add an option to aktualizr-info to print delegations metadata #1138
Conversation
@@ -107,6 +107,10 @@ enum class InstalledVersionUpdateMode { kNone, kCurrent, kPending }; | |||
// Functions loading/storing multiple pieces of data are supposed to do so atomically as far as implementation makes it | |||
// possible | |||
class INvStorage { | |||
public: | |||
using DelegationRecord = std::tuple<std::string, std::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.
Any reason not to use std::pair
? And shouldn't role_name
be the first field?
Probably a matter of test but also not sure if it's worth it adding this type synonym in the header: in client code, we'd have one std::vector<std::pair<Uptane::Role, std::string>>
, and the rest can be deduced via auto
.
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.
And shouldn't role_name be the first field?
Why should it be the first ? I think, it makes sense to preserve the same field order as in the DB table.
Any reason not to use std::pair?
The reason, I decided to go with std::tuple over std::pair is the fact that the later can accommodate just two fields while the former can have arbitrary number of them what can be handy in case if the table schema changes (more than two fields or just one field).
Probably a matter of test but also not sure if it's worth it adding this type synonym in the header: > in client code, we'd have one std::vector<std::pair<Uptane::Role, std::string>>, and the rest can > be deduced via auto
I think, it makes sense to use Uptane::Role regardless of the type been declared or not, it will make the type more clear and explicit.
Regarding introduction of the new type. IMHO, there are several pros over not doing it:
- if the type is changed then you don't need to change it in every spot where it's used;
- if adding some methods is required then the type declaration can be replaced with corresponding struct/class;
- less typing (at the moment it's used in the storage interface, DB storage interface, DB storage implementation and at least once for each client (although
auto
cannot be used in function declarations/signature)
In general, it improves Extensibility and Maintainability.
Having said that, I don't mind to get rid of the type declaration since this is very unlikely that loadAllDelegations() will have more than two clients, i.e. aktualizr-info and its tests.
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.
Pushed code with the changes implied here.
8259aeb
to
f5cd7ec
Compare
Codecov Report
@@ Coverage Diff @@
## master #1138 +/- ##
==========================================
- Coverage 76.17% 76.17% -0.01%
==========================================
Files 164 164
Lines 9869 9904 +35
==========================================
+ Hits 7518 7544 +26
- Misses 2351 2360 +9
Continue to review full report at Codecov.
|
Signed-off-by: Mike Sul <[email protected]>
f5cd7ec
to
20945b7
Compare
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.
LGTM. @mike-sul do you want to merge now despite the TODO in SpawnProcess? This is higher-priority anyway and if you come back to fix that up later than I don't see a problem.
Yes, anyway the TODO depends on the other PR that hasn't been merged yet. |
Signed-off-by: Mike Sul [email protected]
--delegate
to aktualizr-info aimed to print delegations metadata;