-
Notifications
You must be signed in to change notification settings - Fork 48
feat(plugins): add plugin groups #1393
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
base: main
Are you sure you want to change the base?
Conversation
This adds plugin groups as a way to set a collection of plugins all at once. Setting a plugin group will remove any manually registered plugins so it is crucial that this happen before registering other plugins.
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.
Pull Request Overview
This PR introduces plugin groups to allow setting collections of plugins at once. The implementation adds three plugin groups: MINIMAL (dump and nil plugins), DEFAULT (all existing plugins), and QUESTING (DEFAULT plus v2 versions of dotnet and python plugins for Ubuntu 25.10+).
Key changes:
- Plugin groups are defined as an enum with plugin dictionaries as values
- New
set_plugin_group()function replaces registered plugins with the specified group - The registry now initializes with the DEFAULT group instead of copying from a static dictionary
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| craft_parts/plugins/plugins.py | Implements PluginGroup enum and set_plugin_group() function, refactors plugin registration |
| craft_parts/plugins/python_v2/python_plugin.py | Updates imports to use specific base and properties modules |
| tests/unit/plugins/test_plugins.py | Adds test fixture for plugin cleanup and parametrized test for plugin groups |
| tests/integration/plugins/test_plugin_registry.py | Adds integration test verifying MINIMAL group excludes autotools plugin |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
craft_parts/plugins/plugins.py
Outdated
| The plugins in this group are generally considered functional on most legacy bases. | ||
| """ | ||
|
|
||
| QUESTING = DEFAULT | { |
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.
What about naming it "EXPERIMENTAL" or "DEVEL"? I don't like the idea of having another place with an Ubuntu codename to update someday
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.
Well we'd definitely need to change EXPERIMENTAL or DEVEL, but this name is a placeholder for "the plugin set we should use starting with Ubuntu questing."
I'm open to better names, but I don't think EXPERIMENTAL or DEVEL is it.
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.
cc @tigarmo for name bikeshedding :-)
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.
For this initial implementation I'm actually tending towards not having this per-base definition in craft-parts at all. My thinking is that ultimately it's the applications that dictate which plugins are even valid/possible for a given base, ideally after the plugins have been tested and validated. So it feels to me like any decision me make here regarding the 'default' plugins for a given base might be incorrect.
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.
Any name suggestions? Otherwise I'll probably just call them ALICE and BOB
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.
Well, at the moment it just contains usrmerge-fixed plugins, how about something like USRMERGE_READY?
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.
Any name suggestions? Otherwise I'll probably just call them
ALICEandBOB
My suggestion is only having DEFAULT
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.
We are no longer questing for anything.
| """Unregister all user-registered plugins.""" | ||
| global _plugins # noqa: PLW0603 | ||
| _plugins = copy.deepcopy(_BUILTIN_PLUGINS) | ||
| set_plugin_group(PluginGroup.DEFAULT) |
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.
I think unregister_all should reset to _plugins = {}, or _plugins should be PluginGroup.DEFAULT as its initial value. Having the two different possibilities is confusing.
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.
The latter is the case already (see line 124).
I would prefer unregister_all to set it to empty, but that would be a breaking change.
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.
oooh, sneaky...
| """Unregister all user-registered plugins.""" | ||
| global _plugins # noqa: PLW0603 | ||
| _plugins = copy.deepcopy(_BUILTIN_PLUGINS) | ||
| set_plugin_group(PluginGroup.DEFAULT) |
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.
oooh, sneaky...
| } | ||
|
|
||
| _plugins = copy.deepcopy(_BUILTIN_PLUGINS) | ||
| class PluginGroup(enum.Enum): |
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.
is client code (the applications) meant to subclass this?
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.
No, the application simply passes a similar looking dictionary to register to override the plugins. Something like...
if base < JAMMY:
set_plugin_group(PluginGroup.MINIMAL)
register(APP_SUPER_OLD_PLUGINS)
elif base <= NOBLE:
set_plugin_group(PluginGroup.DEFAULT)
register(APP_PLUGINS_FOR_JAMMY_AND_NOBLE)
elif base <= RESOLUTE:
set_plugin_group(PluginGroup.THE_ONE_WITH_THE_NEW_PYTHON_AND_DOTNET)
register(APP_PLUGINS_FOR_RESOLUTE)
else:
raise WeHaventThoughtThatFarInTheFutureError()If the app wants to store those in their own enum it can, but then the likely steps would be to do:
set_plugin_group(PluginGroup.MINIMAL)
register(AppPluginGroup.MY_GROUP)I could generalise set_plugin_group though to take that dictionary?
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.
I could generalise
set_plugin_groupthough to take that dictionary?
I think this might be a good idea, otherwise it kind of looks like one needs to subclass the enum (and the linters might complain)
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.
🫡
tigarmo
left a comment
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.
Thanks! Now I believe we should add a task in craft-app to provide a 'blessed' way for an application to fully define the set of plugins, right?
This adds plugin groups as a way to set a collection of plugins all at once. Setting a plugin group will remove any manually registered plugins so it is crucial that this happen before registering other plugins.
CRAFT-4915
make lint && make test?docs/reference/changelog.rst)?