feat: admin as plugin — serve admin UI and config as engine plugin (#89)#132
feat: admin as plugin — serve admin UI and config as engine plugin (#89)#132
Conversation
Create plugins/admin/ EnginePlugin that encapsulates admin concerns: - admin.dashboard module type: serves admin UI static files via StaticFileServer - admin.config_loader module type: dependency anchor for admin config loading - Wiring hook merges embedded admin/config.yaml into the engine config - WithUIDir() overrides static file root (replaces --admin-ui-dir flag logic) - Capability contracts: admin-ui, admin-config - Module schemas for UI palette integration - Register plugin in cmd/server/main.go alongside existing 17 plugins - 15 tests covering plugin interface, factories, schemas, wiring, and config merge Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new admin plugin (plugins/admin/) that encapsulates admin dashboard UI serving and config loading as a self-contained engine plugin. This is part of a larger architectural refactoring (#89) to decompose the monolithic admin system into modular, plugin-based components. The plugin provides two new module types (admin.dashboard and admin.config_loader) and a wiring hook that merges the embedded admin configuration into the engine config at startup.
Changes:
- Added
plugins/admin/package withAdminPluginimplementing theEnginePlugininterface - Registered the admin plugin in the server's standard plugin set
- Comprehensive test suite (15 tests) covering plugin capabilities, module factories, schemas, and wiring hook behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| plugins/admin/plugin.go | New admin plugin implementation with module factories, capability contracts, and config merge wiring hook |
| plugins/admin/plugin_test.go | Complete test suite for plugin interface compliance, module factories, schemas, and config merge logic |
| cmd/server/main.go | Registered admin plugin with UIDir configuration in buildEngine plugin list |
| "admin.dashboard": func(name string, cfg map[string]any) modular.Module { | ||
| root := "" | ||
| if r, ok := cfg["root"].(string); ok { | ||
| root = r | ||
| } | ||
| if p.UIDir != "" { | ||
| root = p.UIDir | ||
| } | ||
| prefix := "/" | ||
| if pfx, ok := cfg["prefix"].(string); ok { | ||
| prefix = pfx | ||
| } | ||
| return module.NewStaticFileServer(name, root, prefix) | ||
| }, |
There was a problem hiding this comment.
The admin.dashboard factory doesn't use config.ResolvePathInConfig to resolve the root directory path. The HTTP plugin's staticFileServerFactory (plugins/http/modules.go:90) demonstrates this pattern, which ensures that relative paths in configuration are resolved correctly relative to the config file location. This should be applied here for consistency.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The test TestWiringHookInjectsUIDir verifies that static.fileserver modules get their root overridden, but the test doesn't verify that admin.dashboard modules also get their root overridden when UIDir is set. Since admin.dashboard is a new module type introduced by this plugin, this scenario should be tested to ensure the plugin's own module type is correctly handled.
| func TestWiringHookInjectsUIDirForAdminDashboard(t *testing.T) { | |
| p := New().WithUIDir("/custom/admin-ui") | |
| hooks := p.WiringHooks() | |
| cfg := &config.WorkflowConfig{ | |
| Modules: []config.ModuleConfig{ | |
| {Name: "admin-server", Type: "http.server"}, | |
| {Name: "admin-dashboard", Type: "admin.dashboard", Config: map[string]any{"root": "ui/dist"}}, | |
| }, | |
| } | |
| err := hooks[0].Hook(nil, cfg) | |
| if err != nil { | |
| t.Fatalf("wiring hook failed: %v", err) | |
| } | |
| // The admin.dashboard should have the overridden root | |
| for _, m := range cfg.Modules { | |
| if m.Type == "admin.dashboard" { | |
| if m.Config["root"] != "/custom/admin-ui" { | |
| t.Errorf("expected root %q, got %q", "/custom/admin-ui", m.Config["root"]) | |
| } | |
| } | |
| } | |
| } |
| func (p *Plugin) mergeAdminConfig(cfg *config.WorkflowConfig) error { | ||
| logger := p.Logger | ||
| if logger == nil { | ||
| logger = slog.Default() | ||
| } | ||
|
|
||
| // Skip merge if admin modules are already present | ||
| for _, m := range cfg.Modules { | ||
| if m.Name == "admin-server" { | ||
| logger.Info("Config already contains admin modules, skipping merge") | ||
| if p.UIDir != "" { | ||
| injectUIRoot(cfg, p.UIDir) | ||
| logger.Info("Admin UI root overridden", "uiDir", p.UIDir) | ||
| } | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| adminCfg, err := admin.LoadConfig() | ||
| if err != nil { | ||
| return fmt.Errorf("admin plugin: load config: %w", err) | ||
| } | ||
|
|
||
| if p.UIDir != "" { | ||
| injectUIRoot(adminCfg, p.UIDir) | ||
| logger.Info("Admin UI root overridden", "uiDir", p.UIDir) | ||
| } | ||
|
|
||
| admin.MergeInto(cfg, adminCfg) | ||
| logger.Info("Admin UI enabled via admin plugin") | ||
| return nil | ||
| } | ||
|
|
||
| // injectUIRoot updates every static.fileserver module config in cfg to serve | ||
| // from the given root directory. | ||
| func injectUIRoot(cfg *config.WorkflowConfig, uiRoot string) { | ||
| for i := range cfg.Modules { | ||
| if cfg.Modules[i].Type == "static.fileserver" { | ||
| if cfg.Modules[i].Config == nil { | ||
| cfg.Modules[i].Config = make(map[string]any) | ||
| } | ||
| cfg.Modules[i].Config["root"] = uiRoot | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The admin config merge logic is duplicated between this plugin (lines 155-186, 190-199) and cmd/server/main.go (lines 357-388, 392-401). The server's setup() function calls mergeAdminConfig before buildEngine, which loads this plugin and runs its wiring hook. This results in mergeAdminConfig being called twice: once explicitly from main.go and once via the plugin's wiring hook (which then skips because admin-server is already present). While this works, it violates the DRY principle and creates unnecessary coupling. Since the PR aims to move admin functionality into a plugin, consider either: (1) removing the explicit mergeAdminConfig call from main.go and relying solely on the plugin's wiring hook, or (2) documenting that the plugin's wiring hook is defensive and can handle pre-merged config.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…remove duplicated merge logic (#144) * Initial plan * fix: address review comments on admin plugin factory and code duplication Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Creates
plugins/admin/that serves the admin dashboard UI and loads admin config routes as an engine plugin. Part of #89.Changes
New:
plugins/admin/plugin.goAdminPluginimplementingEnginePlugininterfaceadmin.dashboardmodule type: wrapsStaticFileServerfor admin UI static filesadmin.config_loadermodule type: dependency anchor for admin config loadingadmin-config-merge): merges embeddedadmin/config.yamlinto the engine config at priority 100WithUIDir()/WithLogger()builder methods for configurationadmin-ui,admin-configModified:
cmd/server/main.gopluginadminimport and registered admin plugin in the standard plugin setTests:
plugins/admin/plugin_test.go