Issue 1056

[hosie]

[hosie]

This is the first wave of changes for issue #1056 . This PR

• refactors the bootstrap code to make it easier to extend with a new executable
• add dynamic registration to the event plugin factory

future PRs will add dynamic registration to the other plugin factories

[hosie]

Thank you @peterbroadhurst this is great.

I really like the idea of the factory interface. That might also be a good opportunity to align the code with the config spelling wrt name vs type. e.g. when registering an instance of a factory, you would provide its type. When someone wants to use your factory to create an instance of the plugin, then they provide a name.

e.g.

type Factory interface {
	Type() string
	NewInstance(name string) Plugin
}

which would be registered via

func RegisterPluginFactory(factory Factory)

Maybe the need for this becomes more obvious when we look at the other plugins.

e.g. for the given config

plugins:
  database:
  - name: database0
    type: sqlite3
    sqlite3:
      url: /etc/firefly/db?_busy_timeout=5000
      migrations:
        auto: true
  - name: database1
    type: postgres
    postgres:
      url: postgresql://mypostgres/mydb
      migrations:
        auto: true        
  - name: database2
    type: postgres
    postgres:
      url: postgresql://myotherpostgres/mydb
      migrations:
        auto: false

There needs to have been 2 calls to RegisterPluginFactory. One with a factory whose Type is sqlite3 and the other with Type postgres. The namespace manager would then call NewInstance on the sqlite factory (with name database0 and then call NewInstance twice on the postgres factory with names database1 and database2.

However, given your comment

which I assume would run to all other plugins.

it is worth mentioning that event plugins are special. They don't have a name, only a type. e.g.

 transports:
   enabled: ["websockets","webhooks","system"]

And also, the rest of the code base assumes there is only ever one instance of each type.

i.e

when we call eifactory.GetPlugin with type websockets here https://github.com/hyperledger/firefly/blob/main/internal/apiserver/route_get_websockets.go#L41 we must get back exactly the same golang object that was returned here https://github.com/hyperledger/firefly/blob/main/internal/namespace/manager.go#L1095 (and initialised a few lines later).

So there are a couple of options a) have a single pattern that applies to all plugins b) treat event plugins as special.

Given that, for all plugins, we need to define the behaviour in the case where GetPlugin is called twice with the same name and type, and _it returns the existing instance with that name or else creates a new instance _ seems like a sensible behaviour then it seems like a is a good option.

I would propose that a module in firefly core provides that behaviour rather than asserting that the plug in providers do. e.g. maybe rename difactory and eifactory etc.. to diregistry eiregistry and they would hold a list of factories ( each with a unique type) and a list of Plugin objects ( with a unique type/name tuple) . They registry would implement a GetPlugin function that takes name and type and only calls NewInstance on the factory for that type if it does not already have an instance with that name.

For event plugins, then we would just always use the same name ( e.g. nil or "" or default).

[peterbroadhurst]

The thing that seems to be stopping the Events interface being identical, is the fact that we have name passed into NewInstance(name string) Plugin.

e.g. somehow the instance of the plugin itself seems to need to be aware of its name, or even to be responsible for storing it.

Would it be easier to make the storing of name the responsibility of the Manager that instantiates the plugin instead?

[hosie]

Yes good point. The namespace manager currently does that for all other plugins ~but not for events~. Actually, as I look closer, I see that the namespace manger does pass the instantiated instances of the event plugins down to the orchestrator for each namespace. So the 3 cases that were causing the problem by assuming eifactory.GetPlugin does not create new instances are the exception

• https://github.com/hyperledger/firefly/blob/main/internal/apiserver/route_get_websockets.go#L41
• https://github.com/hyperledger/firefly/blob/main/internal/apiserver/server.go#L355
• https://github.com/hyperledger/firefly/blob/main/internal/events/event_manager.go#L162

So, if we can figure out a contract between apiserver and either namespace manager or orchestrator to get hold of the existing instance of the websocket event plugin , then everything stays nice and clean. Factory just worries about creating new instances and namespace manager worries about managing those instances.

... or we accept that events are different. That they are singletons and the plugins do not register a factory, they each just register a single instance of the Plugin object. That certainly seems easiest at this stage. I guess the question is whether we want to allow multiple instances of event plugins in the future (e.g. maybe connecting to different brokers for different namespaces).

[awrichar]

Since namespace manager is responsible for parsing the config and instantiating the events plugins, I think it makes sense to also expose a "getter" for the websockets plugin. Since the API server always has access to namespace manager, it can ask for the websockets plugin at the two points it's required.

[codecov-commenter]

Codecov Report

Merging #1075 (cea9588) into main (8cced12) will decrease coverage by 0.04%. The diff coverage is 96.90%.

:exclamation: Current head cea9588 differs from pull request most recent head 33487dc. Consider uploading reports for the commit 33487dc to get more accurate results

@@            Coverage Diff             @@
##             main    #1075      +/-   ##
==========================================
- Coverage   99.99%   99.94%   -0.05%     
==========================================
  Files         307      308       +1     
  Lines       20329    20336       +7     
==========================================
- Hits        20327    20325       -2     
- Misses          2       11       +9     

Impacted Files	Coverage Δ
internal/apiserver/route_get_websockets.go	100.00% <ø> (ø)
internal/apiserver/server.go	100.00% <ø> (ø)
internal/events/event_manager.go	100.00% <ø> (ø)
internal/events/system/events.go	98.03% <0.00%> (-1.97%)	:arrow_down:
internal/namespace/manager.go	100.00% <ø> (ø)
cmd/firefly.go	27.27% <60.00%> (-72.73%)	:arrow_down:
internal/events/webhooks/webhooks.go	100.00% <100.00%> (ø)
internal/events/websockets/websockets.go	100.00% <100.00%> (ø)
pkg/firefly/firefly.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[hosie]

need to spend some time sorting out the merge