Skip to content

Conversation

@joaquimrocha
Copy link
Contributor

@joaquimrocha joaquimrocha commented Oct 28, 2025

Summary

Define plugins as being of different types (shipped/user-installed/dev) and load them according to the description set in #3628 .

Related Issue

Fixes #3628 .

Steps to Test

(use the app)

  1. Run the development version of a plugin that's shipped (like the Prometheus one) and verify that it takes precedence over the shipped one: make sure you change something in the plugin and see it showing in the UI
  2. Do the same for a plugin you have also installed with the catalog.
  3. Play with the plugin settings to see if it's reflecting reality and desire
  • tested on windows cmd
  • tested on windows WSL
  • tested on linux bash
  • tested on macOS zsh
  • does npm start run inside a plugin print the correct folder where the plugin is copied to?
  • compiled app loads plugins as expected
  • development mode loads plugins as expected
  • plugins load in-cluster as expected
  • test with an existing plugin installed, does that still get loaded?

Screenshots (if applicable)

Screenshot of the plugins' settings

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 28, 2025
@joaquimrocha joaquimrocha requested review from illume and skoeva and removed request for sniok and vyncent-t October 28, 2025 12:12
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 28, 2025
@illume illume added this to the v0.37.0 milestone Oct 28, 2025
@illume illume added backend Issues related to the backend frontend Issues related to the frontend plugins labels Oct 28, 2025
@illume illume requested a review from Copilot October 28, 2025 12:23
Copy link
Contributor

Copilot AI left a 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 implements a plugin loading system that differentiates plugins by type (shipped/user-installed/development) and loads them according to a priority-based system. Development plugins have the highest priority, followed by user-installed, then shipped plugins. When multiple versions of the same plugin exist, only the highest priority enabled version is loaded.

Key Changes:

  • Backend now returns plugin metadata including type (development/user/shipped) alongside path
  • Frontend applies priority-based loading logic, marking which plugin versions are loaded vs overridden
  • New user-plugins directory introduced to separate user-installed plugins from development plugins
  • UI updated to display plugin type, status (Loaded/Not Loaded/Disabled), and override information

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
plugins/pluginctl/src/plugin-management.js Updated default plugins directory from 'plugins' to 'user-plugins'
frontend/src/plugin/pluginsSlice.ts Added type, isLoaded, and overriddenBy fields to PluginInfo type
frontend/src/plugin/index.ts Implemented applyPluginPriority function and updated plugin loading logic
frontend/src/components/App/PluginSettings/PluginSettings.tsx Added Type column and updated Status column to show override information
frontend/src/components/App/PluginSettings/PluginSettingsDetails.tsx Added type chip display to plugin details header
backend/pkg/plugins/plugins.go Implemented PluginMetadata struct and updated functions to handle three plugin directories
backend/pkg/config/config.go Added UserPluginsDir configuration and defaultUserPluginDir function
backend/cmd/server.go Updated to pass UserPluginDir to configuration
app/electron/plugin-management.ts Added defaultUserPluginsDir function and updated install/update methods
Multiple snapshot files Updated test snapshots to reflect new Type column and Status display changes
Comments suppressed due to low confidence (1)

frontend/src/plugin/index.ts:1

  • The comment on line 286 states plugins with the same name are treated as separate entries, but the actual implementation in applyPluginPriority groups them by name and applies priority logic. This comment is misleading about the actual behavior.
/*

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

@joaquimrocha seems there's some issues with CI checks?

Can you please add some tests for some of the changes?

What happens to plugins people already have installed in the old plugins directory? Will plugins installed to the old folder be ignored by this new code?

@joaquimrocha
Copy link
Contributor Author

What happens to plugins people already have installed in the old plugins directory?

I guess they will still be interpreted as dev plugins. Hmm, this actually can be a problem if we installed the plugins in the old fashion into plugins/ and now they are assumed to be dev plugins which will take over priority over any updates. Let me quickly see if I can have a migration path.

@joaquimrocha
Copy link
Contributor Author

@joaquimrocha seems there's some issues with CI checks?

Can you please add some tests for some of the changes?

I have new stories in the last commit.

@joaquimrocha joaquimrocha force-pushed the plugin-loading-changes branch 6 times, most recently from 3336ce8 to f10ee45 Compare October 29, 2025 09:51
@illume
Copy link
Contributor

illume commented Oct 29, 2025

One of the checks is failing.

Some of the commit subject lines need the context part added.
For backend changes we are more strict about requiring tests now.

@joaquimrocha joaquimrocha force-pushed the plugin-loading-changes branch from a8ec734 to 7fad445 Compare October 29, 2025 13:08
@illume illume changed the title Plugin loading changes plugins: Add "user" plugins, plugin settings UI updates, and change loading priority Oct 29, 2025
@skoeva skoeva force-pushed the plugin-loading-changes branch from 7fad445 to 87803ab Compare October 29, 2025 13:44
@illume illume changed the title plugins: Add "user" plugins, plugin settings UI updates, and change loading priority plugins: Add concepts of (shipped/user-installed/dev) types of plugins with settings UI updates, and loading priorities for different plugin types Oct 29, 2025
@skoeva skoeva force-pushed the plugin-loading-changes branch from f4628a9 to e9bf6ad Compare November 5, 2025 16:59
@skoeva
Copy link
Contributor

skoeva commented Nov 5, 2025

updated the failing snapshots and rebased against main again

@illume
Copy link
Contributor

illume commented Nov 5, 2025

Maybe not for this PR, but this issue would be a good improvement:

  • I think we should have the plugin path listed for each plugin. A couple of us got confused where plugins were.
  • It was not clear to a couple of us that the user-plugins folder was where Plugin Catalog installed plugins were installed. Shall we show the user after installing a plugin where the plugin was installed to?

@joaquimrocha
Copy link
Contributor Author

Maybe not for this PR, but this issue would be a good improvement:

  • I think we should have the plugin path listed for each plugin. A couple of us got confused where plugins were.
  • It was not clear to a couple of us that the user-plugins folder was where Plugin Catalog installed plugins were installed. Shall we show the user after installing a plugin where the plugin was installed to?

I can add an "open folder" button to each plugin but in its details view. WDYT?

@joaquimrocha joaquimrocha force-pushed the plugin-loading-changes branch from e9bf6ad to b18d540 Compare November 5, 2025 20:33
@vyncent-t
Copy link
Contributor

vyncent-t commented Nov 5, 2025

  1. Tried the latest changes on my end, for me the app-catalog does not have the disabled button for the non priority one, the multiple versions flag works though.

Steps

  • Install a plugin from the plugins repo that we currently ship, prometheus etc (ensure it is in the .config/Headlamp/plugins )
  • Run the linux built app
  • Nav to the settings page then plugins tab
  • Note that the flag for multiple versions works but the toggles are both still on

Seen w/ @illume @skoeva

Tried installing dev plugins using app-catalog 0.7.0 and an older dev app-catalog 0.6.3, both are still shown as enabled toggles like in this image.

Used:
Shipped app-catalog 0.7.0 + Dev app-catalog 0.7.0
Shipped app-catalog 0.7.0 + Dev app-catalog 0.6.3

image

@vyncent-t
Copy link
Contributor

  1. Tried the latest changes on my end, for me the app-catalog does not have the disabled button for the non priority one, the multiple versions flag works though.

Steps

  • Install a plugin from the plugins repo that we currently ship, prometheus etc (ensure it is in the .config/Headlamp/plugins )
  • Run the linux built app
  • Nav to the settings page then plugins tab
  • Note that the flag for multiple versions works but the toggles are both still on

Seen w/ @illume @skoeva

Tried installing dev plugins using app-catalog 0.7.0 and an older dev app-catalog 0.6.3, both are still shown as enabled toggles like in this image.

Used: Shipped app-catalog 0.7.0 + Dev app-catalog 0.7.0 Shipped app-catalog 0.7.0 + Dev app-catalog 0.6.3

image

Also noticed that "Reload" button not working here may be due to the use of the .config/Headlamp/user-plugins, maybe we can have the changes made in that dir kick off a reload the same way .config/Headlamp/plugins does. WDYT? @joaquimrocha

@skoeva skoeva force-pushed the plugin-loading-changes branch from b18d540 to b750fb1 Compare November 6, 2025 14:53
@joaquimrocha joaquimrocha force-pushed the plugin-loading-changes branch from b750fb1 to 2b0c30b Compare November 6, 2025 15:00
@skoeva skoeva force-pushed the plugin-loading-changes branch from 2b0c30b to 42c3ba6 Compare November 6, 2025 17:52
@joaquimrocha joaquimrocha force-pushed the plugin-loading-changes branch from 42c3ba6 to f52c88f Compare November 6, 2025 22:17
@vyncent-t
Copy link
Contributor

@joaquimrocha
Just tried the changes and everything looks good 👍 great stuff

  • Can use reload button after installing plugin, plugin now renders in list
  • Can use manual app refresh after installing plugin, plugin now renders in list
  • Can manually remove plugins from .config/Headlamp/user-plugins and refresh app (ghost plugins gone)
  • Disabled plugins that are not loaded now also working

Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

Looking good.

Some issues in CI though...

  • rebase to fix this conflict in app/electron/preload.ts
  • make backend-lint
  • make frontend-test
    • snapshots need an update
    • src/components/App/PluginSettings/PluginSettingsDetails.tsx:17:10 - error TS2300: Duplicate identifier 'Typography'.

@illume illume requested a review from Copilot November 7, 2025 10:15
Copy link
Contributor

Copilot AI left a 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 38 out of 38 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 17 to 22
import { Typography } from '@mui/material';
import Box, { BoxProps } from '@mui/material/Box';
import Button from '@mui/material/Button';
import Chip from '@mui/material/Chip';
import Stack from '@mui/material/Stack';
import Typography from '@mui/material/Typography';
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

Typography is imported twice - once from '@mui/material' on line 17 and again on line 22. Remove the duplicate import on line 17.

Copilot uses AI. Check for mistakes.
Comment on lines +324 to +327
if !os.IsNotExist(err) {
logger.Log(logger.LevelInfo, map[string]string{"pluginPath": pluginPath},
err, "Not including plugin path, error checking main.js")
}
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

The comment says 'Only log if it's not a "does not exist" error (which is expected during deletion)' but this logging is at LevelInfo. During deletion, there may be race conditions where the directory is being removed. Consider using a more specific log level or adding context about whether this is during a deletion operation.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, This would be nice to have, but we can skip it for now.

Comment on lines +207 to 209
if (newEnabledState && p.name === plugName && p.type !== plugType) {
return { ...p, isEnabled: false };
}
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

When enabling one version of a plugin, this automatically disables all other versions with the same name. This behavior should be documented in the function's JSDoc comment as it's a significant side effect that affects user data.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, This would be nice to have, but we can skip it for now.

func Delete(userPluginDir, pluginDir, filename, pluginType string) error {
// Validate plugin type if provided
if pluginType != "" && pluginType != "user" && pluginType != "development" {
return fmt.Errorf("invalid plugin type '%s': must be 'user' or 'development'", pluginType)
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

The 'shipped' plugin type exists in the system but is not allowed for deletion. The error message should clarify that shipped plugins cannot be deleted, rather than implying 'shipped' is invalid. Consider: "invalid plugin type '%s': must be 'user' or 'development' (shipped plugins cannot be deleted)"

Suggested change
return fmt.Errorf("invalid plugin type '%s': must be 'user' or 'development'", pluginType)
return fmt.Errorf("invalid plugin type '%s': must be 'user' or 'development' (shipped plugins cannot be deleted)", pluginType)

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@illume illume Nov 7, 2025

Choose a reason for hiding this comment

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

nit, This message improvement would be nice-to-have, we could skip it for now.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2025
@skoeva skoeva force-pushed the plugin-loading-changes branch from f52c88f to 2eff944 Compare November 7, 2025 14:48
This will be used for the catalog-installed plugins, so we end up with
static, user, and dev plugins.
Static are plugins shipped in Headlamp.
User are plugins installed by the user (via the catalog).
Dev are plugins used during development, like when we are adding a new
feature to a plugin.
Allow specifying plugin type ('user' or 'development') when deleting
plugins. This prevents accidentally deleting the wrong plugin when
plugins with the same name exist in both directories.

When type is specified, only that directory is checked. When omitted,
maintains backward compatibility by checking both directories in
priority order (user-plugins first, then development).
Now that we have introduced user-installed plugins as a different
location.
Otherwise, when there are many plugins in the page, the button is pushed
down and can appear as if there is no Save button.
From the PluginSettingsDetails.
@skoeva skoeva force-pushed the plugin-loading-changes branch from 2eff944 to 2e5b032 Compare November 7, 2025 14:51
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. backend Issues related to the backend cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. frontend Issues related to the frontend plugins size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Define behavior for clashing plugins

5 participants