Code review comment for ~jfguedez/charm-telegraf:feature/intel-cmt-cat

@ guoqiao

Thanks, please see comments inline. Addressed in the latest push.

> for function `check_valid_intel_rdt_configuration`, a few issues:
>
> 1) to report issues, it used both exception and string message, maybe just use
> one way. I will prefer exception.
>
> 2) it returns empty str (false) as ok, which maybe misleading or be misused.
>

I had originally wanted to use the exception as a separate mechanism, but I can see how it would be confusing. Definitely agree with the empty string, so I switched it to use exceptions.

> 3) for kernel version compare, I noticed[0] there is version like `5.13`?
>
> Can we also use the `fetch.apt_pkg.version_compare` to do this ?
>
> [0]: https://en.wikipedia.org/wiki/Linux_kernel_version_history

According to [0], there is always a 3rd number. However, it seems that at least in Ubuntu it's always zero so is has no meaning, as it doesn't match the third digit from upstream. You can see the full table of ubuntu/upstream here [1], they all seem to have the 3 number. The first two numbers (major, minor) always match the kernel version so I changed the validation to use only those (e.g. 5.4), which should be enough for our purposes here.

As to using the apt_pkg version for this, you could have multiple kernels installed, some good, some bad (for example in bionic you need the HWE kernel) so it is more reliable to use the version of the running kernel.

I believe the comments/changes address the issues you brought up. Please take a look again, thanks.

[0] https://ubuntu.com/kernel
[1] https://people.canonical.com/~kernel/info/kernel-version-map.html