-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
hive: Reimplement on top of dig #21562
Conversation
42e742f
to
9c04599
Compare
9c04599
to
93146b4
Compare
/test |
93146b4
to
0d97dbd
Compare
0d97dbd
to
75fee56
Compare
c7674b1
to
fba199e
Compare
Revamped
Could still improve the config flag printing, but I think this is enough for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome!! Especially the printing of the objects that you've set up by implementing a different vision on top of dig (that's the reason the two were kept separate)
pkg/hive/cell/provide.go
Outdated
|
||
func (p *provider) Apply(c container) error { | ||
for _, ctor := range p.ctors { | ||
if err := c.Provide(ctor, dig.Export(true)); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to double check that dig.Export
here is explicit and to stay forever. On first glance this seems like it eliminates scoping from hive entirely, is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep all that scope right now does is give a name to a group of cells. Afaik exactly same as fx? I was thinking of adding something like ProvidePrivate or possible change to Provide(ctor, opts…) form, so you’d do Provide(newPrivateThing, cell.Private).
I was a bit puzzled that fx doesn’t have this. Wasn’t needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ProvidePrivate
. It's nice that Provide
takes ctor...
so didn't go with optional.
pkg/hive/cell/decorators.go
Outdated
} | ||
|
||
func (d *decorator) Apply(c container) error { | ||
scope := c.Scope("(decorate)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are decorators scoped out purely for logging and bookkeeping purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it seemed to make more sense to wrap things with a decorator rather than having the decorator be associated with a module and it’s scope. That’s why I ended up with this. Name of the scope could be better.
Hmm need to check what this ends up looking like in dot graph. Might not look quite right.
EDIT: dot-graph looks fine. Fixed up cilium-agent objects
a bit for Decorate:
🔀 daemon/cmd.init.0.func1 (root.go:92): func(cmd.DaemonCellConfig) cmd.DaemonCellConfig:
🛠️ daemon/cmd.init.0.func2 (root.go:94): func(cmd.DaemonCellConfig)
You read the above as: there's a decorator (emoji conveying "swapping") that takes DaemonCellConfig and returns an augmented DaemonCellConfig, and it's wrapping an Invoke function.
This reimplements hive directly on top of the uber/dig library. This simplifies dealing with lifecycles and logging, and most importantly allows implementing the configuration flag registration in a straightforward way with an invoke. The usage is also simplified as now there's only one library to learn. The API is now very similar to uber/fx, but with the extra Config() option. To simplify use of hive.New the flags are now registered afterwards with RegisterFlags(), and hive creates its own viper instance that can be retrieved with Viper(). See pkg/hive/example/main.go for summary of the API changes. Signed-off-by: Jussi Maki <jussi@isovalent.com>
We need to make sure that LocalNodeStore gets started before registerDaemonHooks as otherwise Get/Update blocks. Fix this by making registerDaemonHooks depend on LocalNodeStore and move the setting of node.localNode to it. Since the list of parameters for registerDaemonHooks is getting quite long, add daemonParams struct. Fixes: d62bd53 ("node: Add LocalNodeStore and convert getters/setters to it") Signed-off-by: Jussi Maki <jussi@isovalent.com>
fba199e
to
183d95d
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This reimplements hive directly on top of the uber/dig library. This simplifies
dealing with lifecycles and logging, and most importantly allows implementing
the configuration flag registration in a straightforward way with an invoke.
The usage is also simplified as now there's only one library to learn. The
API is very similar to uber/fx, but has support for configs. At "hive" creation
time the config command-line flags are registered, but object graph instantiation
is deferred to start time. This allows supporting command-line applications with
many commands (with each their own object graph) and only instantiating when
we know which command to run.
Usage looks like this now: