refactor(libsinsp): remove cri extern variables #1541
Conversation
|
@therealbobo starting some better unit tests for the container engine, will tag you. We should add some more tests as we change things around in the container engine going forward. |
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
62197f8 to
79ed5c5
Compare
a974ae0 to
d4ca0d3
Compare
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
d4ca0d3 to
787a777
Compare
userspace/libsinsp/cri_settings.cpp
Outdated
| settings::~settings() | ||
| { } | ||
|
|
||
| settings* settings::s_instance = nullptr; |
There was a problem hiding this comment.
Same feedback as other PR, do we need raw pointers? If possible smart pointers going forward in libs, we have bee burnt a few times.
There was a problem hiding this comment.
The fundamental problem is that the adoption of smart pointers requires having at least the dtor public which totally defeats the settings singleton 😅
There was a problem hiding this comment.
First time I hear that the destructor would need to be private. For example, checkout sinsp_dns_manager.
There was a problem hiding this comment.
The problem here is that I didn't take the static local variable approach (like Meyer's Singleton) but, instead, I went for a more straightforward singleton with a static variable in the global scope. Do you know if the static local variable approach guarantees the uniqueness of the object in all the cases? 🤔
There was a problem hiding this comment.
Thanks for the additional comment @therealbobo let's collectively check what the best approach would be here. Maybe the other maintainers have preferences as well.
There was a problem hiding this comment.
Thought more about it and I would prefer a static get() function that returns a reference to a std::unique_ptr similar to the existing sinsp_dns_manager. Reasons being that we adopt a more consistent style throughout the code base and we discussed in previous core maintainers meetings that we won't accept raw pointers anymore going forward unless absolutely necessary, which I believe we don't have a strong case here for an exception. WDYT?
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
There was a problem hiding this comment.
Thanks @therealbobo LGTM, now just need to overcome the CI.
|
|
||
| */ | ||
|
|
||
| #include <gtest/gtest.h> |
There was a problem hiding this comment.
Not sure if the CI failures are related to that, but I recall there were hiccups including the cri.hpp, e.g. see
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
|
LGTM label has been added. Git tree hash: 338039d9c9a6900150c21149f4e7da247035f26b
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: incertum, jasondellaluce, therealbobo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
/kind design
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
This removes the extern cri variables in attempt to make libsinsp more maintainable.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: