cincinnati/plugins: add plugins catalog and configuration #88
Conversation
openshift-ci-robot
added
approved
|
This is extracted from #74, in order to make that PR easier to rebase and review. |
|
There is an ongoing quay outage. Tests will need to be re-triggered once that is solved. |
lucab
force-pushed
the
ups/plugins-catalog
branch
from
844003f to
b7633f6
Compare
There was a problem hiding this comment.
Looks like a practical and simple enough approach to the issue and I think we can move forward with it! A part of me wants to disperse the centralized list of known plugins in the match statement into the plugin files as some form of macro, however it's probably better to keep it simple for now.
This adds a plugin catalog in order to be able to match plugins in configuration entries. It also adds config deserialization for plugins, from TOML values.
|
Replied to comments and pushed an update with fixes, PTAL. |
| /// Settings for a plugin. | ||
| pub trait PluginSettings: Debug + Send { | ||
| /// Build the corresponding plugin for this configuration. | ||
| fn build_plugin(&self) -> Fallible<Box<Plugin<PluginIO>>>; |
There was a problem hiding this comment.
What do you think of taking ownership of self here and drop the .clone() which is used in all the implementations? That leaves it to the caller if they want to clone or not.
There was a problem hiding this comment.
That would be nice, however I'm not sure that's possible. In particular, taking the ownership and using it results in:
error[E0161]: cannot move a value of type dyn plugins::catalog::PluginSettings:
the size of dyn plugins::catalog::PluginSettings cannot be statically determined
And adding a Sized bound turns into:
error[E0038]: the trait `plugins::catalog::PluginSettings` cannot be made into an object
= note: the trait cannot require that `Self : Sized`
There was a problem hiding this comment.
It does seem to me like we're not making full use of the type system here since two of the methods which need to be implemented by every plugin aren't in any trait, so it's mere convention. However I'm failing to come up with something that seems better 🤷♂️
|
I think we can ignore my idealistic remark about the implementation details and go with this approach. It definitely does what we want and has elegant settings merging with minimal repetitiveness. @lucab otherwise: |
openshift-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lucab, steveeJ 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 |
|
From out-of-band discussion: let's keep it as is. It still needs to be wired into application config, further tweaks may happen while doing that. /hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
This adds a plugins catalog in order to be able to lookup plugins in
configuration entries. It also adds config deserialization for plugins,
from TOML values.
/cc @steveej