Gatherer/Gatherers plugin management #114
Conversation
CDimonaco
force-pushed
the
available_gatherers_manager
branch
from
b0c08e9 to
ccdaba8
Compare
CDimonaco
requested review from
arbulu89,
dottorblaster,
fabriziosestito,
rtorrero and
nelsonkopliku
There was a problem hiding this comment.
Hey @CDimonaco ,
It looks really good.
I just added some comments on things that I think we could discuss about!
| func (s *ErrorGatherer) Gather(_ []entities.FactRequest) ([]entities.Fact, error) { | ||
| return nil, &entities.FactGatheringError{Type: "dummy-type", Message: "some error"} | ||
| } | ||
|
|
||
| func (suite *GatheringTestSuite) TestGatheringGatherFacts() { |
There was a problem hiding this comment.
I like this naming for this test, but I think other tests are still using the previous prefix, e.g. TestFactsEngineGatherFacts which is a bit confusing IMO
Am I missing something?
| suite.Equal(expectedGatherers, result) | ||
| } | ||
|
|
||
| func (suite *TestManager) TestFactsEngineGetGathererNotFound() { |
There was a problem hiding this comment.
This test name still need to be changed hehe
FactsEngine to Manager
cmd/facts.go
Outdated
| @@ -81,17 +81,29 @@ func gather(*cobra.Command, []string) { | |||
| var argument = viper.GetString("argument") | |||
| var pluginsFolder = viper.GetString("plugins-folder") | |||
|
|
|||
| engine := factsengine.NewFactsEngine("", "") | |||
| gathererManager := gatherers.NewManager(gatherers.StandardGatherers()) | |||
There was a problem hiding this comment.
Maybe we could have a gatherers.NewManagerWithPlugins(gatherers.StandardGatherers(), pluginsFolder to remove some duplicated code
There was a problem hiding this comment.
Good job!
Only reasoning point I might raise is the Manager terminology.
I find it sometimes a bit too generic: what aspects is it managing? What does it actually mean manage, it can be really anything, and it can imply a lot of things.
This reminds me of some past not-so-nice experiences, where some sort of Manager in the code became sort of a god class/module 😅
What do you think about a more explicit variant Registry, or similar?
A place where you track all the available gatherers. That is where you can add and retrieve (get) them.
Just an idea, naming is hard 😄
Absolutely, great idea, manager was really a temp name 🤣 Registry is more appropriate, I will change that |
The manager will contain the business logic to access/modify the fact gatherers avoiding splitting responsabilities through the factsengine package
CDimonaco
force-pushed
the
available_gatherers_manager
branch
from
91ddfb8 to
ad0d852
Compare
|
Renamed to Registry. Ready to merge! 🚀 |
This PR contains an implementation of a "gatherer" manager, a component made in order to extract the responsability of gatherers management into a separate component, contained into gatherer package.
This component will be injected as dependency and initialized when the agents starts with the appropriate configuration.
The plugin management, is moved to the gatherers package in order to segregate responsabilities, infact the plugin system offer the capability of adding new fact gatherers.
Some cleanup and initialization is made to support this refactoring.
Various tests refactoring with auto generated mocks.