Use DI for the CommandExecutor

[rtorrero]

Switch gatherers that use the CommandExecutor to do it using dependency injection

[CDimonaco]

Good job, just a note, in the tests maybe we can use the constructor function to build the gatherers and pass the mock executor, instead of creating the struct directly and set the field, this way we test the exposed api and we can also give a hint to the other developers about how to use the gatherers properly

What do you thing?

[fabriziosestito]

@rtorrero nice! all good , green after addressing @CDimonaco comment

[nelsonkopliku]

LGTM as long as we address what the guys are saying.

Just a note: somewhere else I personally have used two constructors

func NewDefaultPasswordGatherer() *VerifyPasswordGatherer {
	return NewVerifyPasswordGatherer(Executor{})
}

func NewVerifyPasswordGatherer(executor CommandExecutor) *VerifyPasswordGatherer {
	return &VerifyPasswordGatherer{
		executor,
	}
}

Not sure if you guys like that (I hope so because the PR was approved and merged 😅 )
Anyway, I don't mind, whichever way.

[nelsonkopliku]

I believe that reading the cibadmin.xml could go in a SetupSuite() because reading that file once would be enough, while we need to reset the executor on each test because of the .On(...) prophecies (telling how the method should behave).

Yet, this is just fine as well.

[rtorrero]

Indeed, nice catch, this should make the tests execute a bit faster

[rtorrero]

(I don't really get the benefit of the two constructors though :/)

[CDimonaco]

@rtorrero In this case the benefit is to have a much more ergonomic external api, because we don't care about having one instance of the executor among all the gatherers (for now, because really the executor is just a function that runs command), in much more advanced scenarios, where the dependency we want to inject needs to be atomic or at least controlled in all the difference instances, the defaul constructor should be omitted

For example a database client with connection parameters and connection pool, we need to inject the dependency explicitly in order to have a coordination and control among the dependency injected to the dependents, so a default constructor can be counter intuitive and lead to confusion

At the end @rtorrero is up to you, the important thing is that we distinguish this two scenarios when we decide now and in future how to inject the deps

[rtorrero]

Ok, now I see. Indeed in this case, sharing the CommandExecutor instance for all the gatherers work, but I'd also go for what @nelsonkopliku proposes as I think it's just more generic.

[CDimonaco]

@nelsonkopliku For me is fine, the important thing is that we have the injection in place, if we want to give the developers an option to build a gatherer with default dependency, I'm totally with it and can be more ergonomic and simple to use