Add a separate API call to send the device manifest

[eu-siemann]

No description provided.

[pattivacek]

This looks fine, but can you explain the context a bit more? And we should probably add an item to the actions.md list, although it's probably about time to revisit if that list is actually useful anymore considering that the requirements management process seems to be ignoring it.

[codecov-io]

Codecov Report

Merging #1176 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1176      +/-   ##
==========================================
- Coverage   77.97%   77.91%   -0.06%     
==========================================
  Files         170      170              
  Lines        9998    10003       +5     
==========================================
- Hits         7796     7794       -2     
- Misses       2202     2209       +7

Impacted Files	Coverage Δ
src/libaktualizr/primary/aktualizr.h	100% <ø> (ø)	⬆️
src/libaktualizr/primary/sotauptaneclient.h	100% <ø> (ø)	⬆️
src/libaktualizr/primary/sotauptaneclient.cc	91.21% <100%> (+0.02%)	⬆️
src/libaktualizr/primary/aktualizr.cc	96.73% <100%> (+0.1%)	⬆️
src/libaktualizr/package_manager/ostreemanager.cc	68.22% <0%> (-2.97%)	⬇️

---

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 f4b5c98...2a8efdc. Read the comment docs.

[eu-siemann]

I've added a sub-item to the actions.md, as there was already a section for sending device manifest.
UptaneCycle doesn't call CheckUpdates for sending the manifest but uses uptane_client->sendManifest() instead. Were you mentioning that place, @patrickvacek?

[pattivacek]

UptaneCycle doesn't call CheckUpdates for sending the manifest but uses uptane_client->sendManifest() instead. Were you mentioning that place, @patrickvacek?

Precisely!

[eu-siemann]

Funny that no code analyzer is complaining about an unused variable.

[lbonn]

It's probably more difficult to flag that realiably in C++, because it could just rely on side effects of constructors and destructors. For example, a mutex guard is typically not referenced outside its declaration.

[pattivacek]

Might not be strictly necessary, but can you change the UptaneCycle fuction to call the new SendManifest function instead of uptane_client_->putManifest()?

[lbonn]

How would it look like right now, with all the other calls putManifest that we have around our code? To use this method, an user would have to make sure that these will never be called even if SendManifest(xxx) fails once, for example.

[eu-siemann]

@lbonn Could you explain why putManifest shouldn't be called if SendManifest fails?
Right now we call putManifest from Aktualizr::CheckUpdates(), Aktualizr::SendDeviceData() and SotaUptaneClient::finalizeAfterReboot().

[lbonn]

The three calls you mentioned won't include the custom data supplied to SendManifest. In the end, it could lead to the custom data to never reach the server because it was superseded by another manifest send.

[eu-siemann]

@lbonn Right, but that doesn't prevent a user from sending the manifest with custom data again, does it?

[lbonn]

Sure but I'm worried that it would be ignored by backend. Would be good (but maybe impractical?) if the custom data would be sent along with all attempts after the call to sendManifest.

[eu-siemann]

I don't think we should try to be extra smart here and send potentially outdated custom data each time to the backend. In my view, it is better to be explicit and don't interact with the custom data in any way.
But I should probably ask Vladimir when he's back if he has the same view on this.
Still, even if we decide to add such logic later, that won't make this PR incorrect, what do you think, @lbonn?

[lbonn]

Sure we can merge it now.

Just that it doesn't sound easy to use reliably in this state. Or should we have a way to inhibit other manifest sends, so that the user has really full control on that?

[eu-siemann]

@lbonn That would be a more consistent approach IMO. But, to be honest, I'm still not sure if this is what we want. I've added a description of this (inconsistent?) behavior to the API call description, so we can later return to this problem.
Any other concerns regarding this PR? @patrickvacek?
BTW, UptaneCycle now uses API calls instead of sotauptaneclient methods where it is possible.

[pattivacek]

Looks good to me. I agree that there is some inconsistency with the custom metadata, but if we don't know what will be most useful for potential users of this feature, we shouldn't guess and add more complexity that might not be useful. I'd vote leaving it as is; the comment in aktualizr.h is good.

[eu-siemann]

Cool, I'm merging it then, thank you @lbonn and @patrickvacek!