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

Add ability to provide custom hardware information which will be visible in UI #1644

Merged
merged 3 commits into from
Apr 17, 2020

Conversation

eu-siemann
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Apr 15, 2020

Codecov Report

Merging #1644 into master will decrease coverage by 0.06%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1644      +/-   ##
==========================================
- Coverage   82.62%   82.55%   -0.07%     
==========================================
  Files         191      191              
  Lines       12062    12073      +11     
==========================================
+ Hits         9966     9967       +1     
- Misses       2096     2106      +10     
Impacted Files Coverage Δ
src/libaktualizr/primary/aktualizr.h 100.00% <ø> (ø)
src/libaktualizr/primary/sotauptaneclient.h 100.00% <ø> (ø)
src/aktualizr_primary/main.cc 80.00% <50.00%> (-3.04%) ⬇️
src/libaktualizr/primary/sotauptaneclient.cc 90.47% <84.61%> (-0.21%) ⬇️
src/libaktualizr/primary/aktualizr.cc 97.52% <100.00%> (ø)
src/libaktualizr/storage/sqlstorage_base.h 60.00% <0.00%> (-40.00%) ⬇️
src/libaktualizr/storage/sqlstorage_base.cc 74.49% <0.00%> (-4.70%) ⬇️
src/libaktualizr/storage/sql_utils.h 85.89% <0.00%> (-0.18%) ⬇️
src/sota_tools/ostree_http_repo.h 100.00% <0.00%> (ø)
... and 3 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 5894a06...3b5ff52. Read the comment docs.

Copy link
Contributor

@kbushgit kbushgit left a comment

Choose a reason for hiding this comment

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

I would prefer to leave the method sendDeviceData() unchanged. In the future, we may also want to put some custom data for the other reports, for instance, putManifestSimple().

If this information is not going to be changed during the client's lifetime, we can consider moving such functionality into aktualizr, without changing the external interface but rather extend Config i.e.
to add Json::Value hwinfo as a private field in the aktualizr client and fill it during the aktualizr configuration.

@eu-siemann
Copy link
Contributor Author

@kbushgit That's not a bad idea. But I've added this hwinfo file parameter mostly for testing purposes. Current users of this API will gather the needed information in the libaktualizr wrapper, so it should be easier for them to just pass the JSON object via this API.
I'm also not sure why do we even have putManifest call in the SendDeviceData, since we already do it in CheckUpdates and even have a separate API call for that.

@kbushgit
Copy link
Contributor

I mean that we also have an invocation of putManifestSimple, but you are right it can be done using sendManifest call, so, it is ok for now, I am just not sure that in the future, another custom information won't be needed during invocation of this method.

pattivacek
pattivacek previously approved these changes Apr 16, 2020
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.

I'm always reluctant to change or expand the API but I don't see a better way of doing this at present. I also wish it could be done via config somehow, but I don't see a way to do that that wouldn't be substantially more complex. This is pretty straightforward, so I'm okay with it for now.

I'm also not sure why do we even have putManifest call in the SendDeviceData, since we already do it in CheckUpdates and even have a separate API call for that.

I think there was a good reason for that once upon a time. I no longer remember it. It shouldn't really hurt, though, right? It's just extra bandwidth. To me the bigger problem is that if you call aktualizr check on the commandline, it calls SendDeviceData before CheckUpdates. It's the simplest/safest thing to do but does waste bandwidth if you call that repeatedly.

I am just not sure that in the future, another custom information won't be needed during invocation of this method.

That is a real concern, but I think we can reevaluate when a need for something like that arises.

Eugene Smirnov added 3 commits April 17, 2020 14:47
@eu-siemann eu-siemann merged commit a93c030 into master Apr 17, 2020
@eu-siemann eu-siemann deleted the feat/custom-hwinfo branch April 17, 2020 15:05
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.

4 participants