Dependency factory #337
Dependency factory #337
Conversation
abhay-krishna
added
approved
size/XL
There was a problem hiding this comment.
so....to make sure I understand.
you're maintaining a stack of idempotent dependency creation functions; each ensures that its own dependencies are included in the stack before it, if necessary.
When we .Build() the dependency graph , each dependency is built in first-in-first-out order, ensuring that each dependency is built in the correct order, without repeating (the 'idempotent' nil checks).
Is that right?
That's correct :) |
| WithExecutableBuilder(ctx, clusterSpec.UseImageMirror(eksaToolsImage.VersionedImage())). | ||
| WithWriterFolder(clusterSpec.Name). | ||
| WithDiagnosticCollectorImage(clusterSpec.VersionsBundle.Eksa.DiagnosticCollector.VersionedImage()) |
There was a problem hiding this comment.
What's the motivation of keeping these here vs with all the other With* methods? Is it because these are the ones we think are common regardless of command and provider? I notice that we include the diagnostic collector image which we don't use in all the commands.
If we don't need these methods, then we would only need a New method. I was thinking about whether something like below made more sense.
deps := dependencies.New().
WithSpec(spec).
WithExecutableBuilder(...)
...
There was a problem hiding this comment.
Yeah, this method is just a helper when creating dependencies based on a particular clusterSpec. In that case, there are certain configurations that are always the same. The collector image is not always used but is always the same for the same clusterSpec, that's why is here.
To be precise, these With* methods in the helper are slightly different than the other ones, since they don't necessarily build dependencies, but they configure the factory to be able to build the real dependencies.
Regarding New(), I believe that'sNewFactory(), isn't it? (it' called NewFactory instead of just New because it doesn't build the dependencies but a factory to build dependencies)
There was a problem hiding this comment.
I get the distinction now, this looks good to me
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danbudris, g-gaston, mrajashree, vivek-koppuru 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 |
* Move skip ip check option to method argument in provider factory * Fixed package name for kubectl client * Add constructor to build local executable builder * Add dependency factory * Updated commands in cmd to use the dependency factory
Resolves #310
In order to facilitate adding/removing/changing the dependencies for our types and at the same time reduce the duplicated code/work at the
cmdlevel (that's where we usually do DI), this OR add a factory to build a dependency graph.It's a pretty simple implementation but it achieves:
Some drawbacks:
There is definitely room for improvement but I believe the current state is good enough to start using it.
I also considered using https://github.com/google/wire instead, but with the amount of dependencies we have, it was going to take the same amount of time to learn how to use it than the time that saves by generating the boilerplate code. It's something that we could reconsider in the future if we start spending too much time adding entities to the factory.
There might be other places where we can use the factory (maybe E2E tests). I just started with the commands since they were the most obvious place.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.