Refactor/ota 2633/aktualizr info output

[kbushgit]

Refactoring an output of aktualizr-info app.
If the user does not provide any arguments the help will be printed.
To print general information with human readable formatting, use --info.
--ecu-keys and --tls-keys was left, this commands print result more in h2m format, which requires additional parsing.
All other commands print pure information into stdout without garbage, which can be used directly in the pipe.

[lbonn]

There are a couple of clang-tidy warnings.

Also, I'm a bit wary of the change that requires the --info flag to display the human readable info summary. At the moment it would break a lot of existing uses + docs. Have you found it was absolutely necessary?

[codecov-io]

Codecov Report

Merging #1215 into master will increase coverage by 0.15%.
The diff coverage is 97.16%.

@@            Coverage Diff             @@
##           master    #1215      +/-   ##
==========================================
+ Coverage   79.18%   79.33%   +0.15%     
==========================================
  Files         170      170              
  Lines       10044    10086      +42     
==========================================
+ Hits         7953     8002      +49     
+ Misses       2091     2084       -7

Impacted Files	Coverage Δ
src/libaktualizr/logging/logging.cc	100% <100%> (ø)	⬆️
src/aktualizr_info/main.cc	93.75% <100%> (+1.69%)	⬆️
src/libaktualizr/utilities/utils.cc	83.03% <50%> (ø)	⬆️
src/aktualizr_info/aktualizr_info_config.cc	80.48% <0%> (-9.76%)	⬇️
src/libaktualizr/storage/sqlstorage.cc	75.28% <0%> (+0.38%)	⬆️
src/libaktualizr/uptane/imagesrepository.cc	92.59% <0%> (+0.61%)	⬆️
src/libaktualizr/primary/sotauptaneclient.cc	92.98% <0%> (+1.06%)	⬆️

---

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 d90f244...f773f4c. Read the comment docs.

[lbonn]

Are you using another version of clang-tidy than 6.0 (or with other settings)? It looks like you're trying to fix warnings that are not flagged on CI.

[mike-sul]

Once, this PR is merged corresponding changes must be done in meta-updater as it relies on aktualizr-info in its tests (https://github.com/advancedtelematic/meta-updater/search?q=aktualizr-info&unscoped_q=aktualizr-info)

[mike-sul]

While developing "ptest" for posix secondaries I noticed that aktualizr-info can't fetch the requested info from DB quite often (most likely because aktualizr locks access to the DB during its work). I overcame this issue in the test by running aktualizr-info in a loop until it actually succeeds (or number of iteration exceed the maximum allowed).
The question is, if a customer application that utilizes aktualizr/libaktualizr uses aktualizr-info and relies on its output then there are two question appears:

1. What should the customer application do if aktualizr-info fails to fetch requested info ? Do polling/launch it multiple times ? Or maybe, aktualizr-info should guarantee somehow that the requested info is fetched ?

2. It's more convenient and reliable to use some structured format for passing info between apps, e.g. json. Perhaps, makes sense to add such functionality into aktualizr-info ? For example, if there is such use case as an app using aktualizr would like to get info about installed versions on each ECU then having output in json format would be something useful.

[mike-sul]

Regarding these multiple "if () { do_foo(); return EXIT_SUCCESS;}", does it mean that it's not possible to get output of a few types of info/metadata within a single call to aktualizr-info ?

Also, these blocks of "if ... return EXIT_SUCCESS" statements look like ideal candidates for defining a map <std::string, std::function> and having a single block of code that works for all options, e.g. "for op in options { if is_specified(op) { foo_mapop; return EXIT_SUCCESS;}. This is rather matter of style and is minor comment that can be ignored.

[pattivacek]

The early exit is probably undesirable. Whether the suggested refactoring is worth it is debatable; I have no strong feelings on that.

[kbushgit]

Regarding these multiple "if () { do_foo(); return EXIT_SUCCESS;}", does it mean that it's not possible to get output of a few types of info/metadata within a single call to aktualizr-info ?

Also, these blocks of "if ... return EXIT_SUCCESS" statements look like ideal candidates for defining a map <std::string, std::function> and having a single block of code that works for all options, e.g. "for op in options { if is_specified(op) { foo_mapop; return EXIT_SUCCESS;}. This is rather matter of style and is minor comment that can be ignored.

We do not need multiple outputs since we going to use the actualizr-info output in PIPE.

The block with "if .. return" looks ugly, but it does the same job you propose but generates less of CPU instructions.

Also, can we consider our main function as a single block of code? We can use exit() vs return but this particular case it's the same.

By the way, I using this tool https://godbolt.org/ to check the compiler output, time to time

[pattivacek]

We do not need multiple outputs since we going to use the actualizr-info output in PIPE.

I disagree. There is no reason not to support multiple outputs if the user asks for it. We've supported that until now, and I see no reason to remove it.

[kbushgit]

Since we enable output for multiple arguments, should we rename command --name-only?

[pattivacek]

No, that is the one specific exception. I had forgotten about that; I made that as a special option for one of the backend devs. That one should indeed only print the name.

[pattivacek]

1. What should the customer application do if aktualizr-info fails to fetch requested info ? Do polling/launch it multiple times ? Or maybe, aktualizr-info should guarantee somehow that the requested info is fetched ?

aktualizr-info is used primarily internally, so I'm not too worried about this. It would be better to do it right, but no one should be depending on this for production use cases.

2\. It's more convenient and reliable to use some structured format for passing info between apps, e.g. json. Perhaps, makes sense to add such functionality into aktualizr-info ?

Yes, we've considered that and I'm open to it. I believe some e2e tests rely on aktualizr-info output, though, so it's worth coordinating with them. (@rdanitz or @stevana can you confirm?)

[stevana]

Yes, structured aktualizr-info output would be appreciated! (Or a way to get a specific field, e.g. aktualizr-info --device-name.)

[pattivacek]

Yes, structured aktualizr-info output would be appreciated!

@stevana This should be a big step in that direction. Uptane metadata is now printed so that it can be piped directly to jq. Keys are now printed without any other output, so it may be possible to pipe them to other tools as well.

Or a way to get a specific field, e.g. aktualizr-info --device-name.

That actually already exists. --name-only was added by request from @jochenschneider for scripting purposes. Now it's even better; you previously had to add --loglevel 4 to prevent any other output from being generated, but now it does that by default. The name of the flag could perhaps be better, but whatever, the functionality is there.

[stevana]

@patrickvacek: Nice!

/cc @rdanitz @balajirrao

[pattivacek]

Tested manually and works exactly as expected. Thanks!