Skip to content

Closes: #18535 - Skip incompatible plugins during startup #18537

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

Merged
merged 17 commits into from
Mar 10, 2025

Conversation

bctiemann
Copy link
Contributor

Closes: #18535

Changes the behavior of the PluginConfig.validate method so that if a plugin with incompatible min or max version specified, it raises an IncompatiblePluginError which is caught in settings.py and registering the plugin is skipped, allowing NetBox to launch cleanly with the plugin disabled.

@a084ed22
Copy link

Just to understand, this would print a warning at netbox upgrade time? The experience of figuring out which plugin has a strict max_version specified and breaks the upgrade process even on patch upgrades during the database migration step could really use to be more explanatory.

@bctiemann
Copy link
Contributor Author

Yes, it would look like this:

Unable to load plugin netbox_branching: Plugin netbox_branching requires NetBox maximum version 4.2.1 (current: 4.2.2).
Watching for file changes with StatReloader
Performing system checks...

@bctiemann bctiemann changed the base branch from feature to main February 5, 2025 15:31
@bctiemann bctiemann changed the base branch from main to feature February 5, 2025 15:45
@bctiemann bctiemann changed the title Closes: #18535 - Skip incompatible plugins during startup and remove from PLUGINS Closes: #18535 - Skip incompatible plugins during startup Feb 5, 2025
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to rename the table columns. Currently, a plugin which fails to load shows false under "Installed" but confusingly still reports an installed version:

screenshot

We should also highlight any plugins which failed to load to draw attention to them.

@bctiemann
Copy link
Contributor Author

I renamed "Installed" to "Active", and I added a custom table column that allows an alert icon with a tooltip that explains the incompatibility issue:

Screenshot 2025-02-23 at 6 51 43 PM Screenshot 2025-02-23 at 6 50 32 PM

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This renders an alert for every non-installed plugin. The value for is_installed should probably be None if the plugin has not been added to PLUGINS, and False if it has been added but failed to load.

screenshot

@@ -707,3 +707,22 @@ class DistanceColumn(TemplateColumn):

def __init__(self, template_code=template_code, order_by='_abs_distance', **kwargs):
super().__init__(template_code=template_code, order_by=order_by, **kwargs)


class PluginActiveColumn(BooleanColumn):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure a custom column is needed here. Could we just use BooleanColumn and pass a custom HTML string for false_mark?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like it, if I want the false_mark text to be dynamically generated (i.e. to reflect the compatible version range). All the usage guidance I've been able to find suggests that the only way to add custom functionality referencing other columns is to subclass and override.

Also, in investigating this I found that BooleanColumn (and really all columns) ignore None values altogether and just render an — by default; if I comment out EMPTY_MARK it causes no ill effects. So setting is_installed to None means it doesn't trigger any custom column logic at all. I think for clarity I'll have to set another boolean field failed_to_load and reference that in the custom column logic, and set is_installed to True for all local plugins as it was before.

We might want to clean up BooleanColumn to remove the references to it, to avoid giving future devs the mistaken impression that we can incorporate None values into custom column logic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you just use a TemplateColumn then? I'd like to avoid introducing a new column class that only has one use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I think this will do the trick.

I fussed around a bunch trying to centralize the TRUE_MARK etc strings to avoid redundancy, but then realized we're already re-hard-coding those <i class="mdi mdi-check-bold"></i> in a million places so this only adds a little incremental chaos.

@bctiemann bctiemann force-pushed the 18535-skip-incompatible-plugins branch from 2d7e332 to b5e9ae0 Compare February 25, 2025 16:55
@bctiemann bctiemann force-pushed the 18535-skip-incompatible-plugins branch from b5e9ae0 to d1ea60f Compare February 25, 2025 17:05
@bctiemann bctiemann added this to the v4.3 milestone Feb 25, 2025
@jeremystretch jeremystretch self-requested a review February 27, 2025 14:36
Comment on lines 67 to 69
is_local: bool = False # extra field for locally installed plugins
is_installed: bool = False
failed_to_load: bool = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time discerning the difference between is_local and is_installed. It seems that any installed plugin has is_local set to True. Additionally, failed_to_load feels backward to me.

Could we simplify these to two attributes: is_enabled and is_installed (or is_loaded)? These would indicate whether the plugin is listed in settings.PLUGINS and whether is has been successfully loaded into the registry, respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this -- is_local (which mirrors the local_plugins etc verbage, as opposed to catalog_plugins); and is_loaded, which indicates whether it's loaded into the registry. This removes is_installed as redundant, and the is_installed column remains on the table class but is purely rendered via the template logic.

@@ -14,3 +14,13 @@
OBJECTCHANGE_REQUEST_ID = """
<a href="{% url 'core:objectchange_list' %}?request_id={{ value }}">{{ value }}</a>
"""

PLUGIN_IS_INSTALLED = """
{% if record.failed_to_load %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A plugin could fail to load for other reasons though, presumably. I'd avoid assuming it was due to a version incompatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrased the tooltip accordingly. It's possible to do an on-the-fly validation of the version range to see whether it should be suggested at all, but that got a bit too hairy.

@jeremystretch jeremystretch self-requested a review March 6, 2025 15:38
@jeremystretch jeremystretch merged commit b5d970f into feature Mar 10, 2025
6 checks passed
@jeremystretch jeremystretch deleted the 18535-skip-incompatible-plugins branch March 10, 2025 14:51
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants