From f14bfc3222361d2f39686f083a506c014d6fadbc Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Mon, 2 Nov 2026 11:57:01 +0000 Subject: [PATCH] Address review of template inheritance - Unlinking a field enables Apply on its own and sends the current value. - The link button no longer toggles the checkbox of its label. - Link and disconnect icons for all four states, pending ones highlighted. - instance.is_template is hidden. - ctl create takes --as-template. - Clearer names for the InstanceManager inheritance helpers. Part of #200 Co-Authored-By: Claude Opus 5.5 --- docs/configuration.md | 3 +- docs/managing-a-cluster.md | 4 +- packages/controller/src/InstanceManager.ts | 30 ++++---- packages/ctl/src/commands_instance.ts | 6 +- packages/lib/src/config/definitions.ts | 1 + .../web_ui/src/components/BaseConfigTree.tsx | 73 ++++++++++--------- .../src/components/InstanceConfigTree.tsx | 4 +- test/integration/clusterio.js | 6 +- 8 files changed, 68 insertions(+), 59 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 49685ef8..41683905 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -527,6 +527,7 @@ If enabled this instance is a template other instances can inherit config values Templates cannot be assigned to a host and therefore never start. Hidden from `instance list` in clusterioctl unless `--templates` is passed, and listed separately in the web interface. An instance cannot stop being a template while other instances inherit from it. +Hidden in the web interface, set when creating the template. Defaults to false. @@ -540,7 +541,7 @@ When set on an existing instance fields with the same value as the template or t `instance.name`, `instance.is_template`, `instance.template_id`, the port fields, `factorio.rcon_password` and hidden fields are never inherited. A template may itself inherit from another template, but not from itself through a chain of templates. The fields currently inherited are stored in the hidden `instance.inherited_fields` field. -In the web interface inherited values are locked and must be unlocked before they can be changed. +In the web interface inherited values are linked and read only until they are unlinked. Defaults to null meaning no template. diff --git a/docs/managing-a-cluster.md b/docs/managing-a-cluster.md index 67cd5a13..5a048c27 100644 --- a/docs/managing-a-cluster.md +++ b/docs/managing-a-cluster.md @@ -86,9 +86,9 @@ Templates are only listed if `--templates` is passed. ### Create instance - ctl> instance create [--id ] [--from ] [--template ] [--is-template] + ctl> instance create [--id ] [--from ] [--template ] [--as-template] -Create a new instance. `name` is the instance name. `--id` is the optional id. `--from` copies the config of an existing instance. `--template` makes the new instance inherit all config values from the given template. `--is-template` creates the instance as a template, see `instance.is_template` in the [configuration docs](configuration.md). Use `--is-template --from ` to make a template out of an existing instance. +Create a new instance. `name` is the instance name. `--id` is the optional id. `--from` copies the config of an existing instance. `--template` makes the new instance inherit all config values from the given template. `--as-template` creates the instance as a template, see `instance.is_template` in the [configuration docs](configuration.md). Use `--as-template --from ` to make a template out of an existing instance. ### Kill instance diff --git a/packages/controller/src/InstanceManager.ts b/packages/controller/src/InstanceManager.ts index ffbeddc9..c9feda88 100644 --- a/packages/controller/src/InstanceManager.ts +++ b/packages/controller/src/InstanceManager.ts @@ -115,13 +115,13 @@ export default class InstanceManager { addInstanceHooks(instance: InstanceRecord): void { instance.config.on("fieldChanged", (field: string, curr: unknown, prev: unknown) => { if (field === "instance.template_id") { - this._link(instance); + this._linkMatchingFields(instance); } else if (!this._inheriting && instance.config.isInherited(field)) { - this._override(instance, field, curr, prev); + this._unlinkEditedField(instance, field, curr, prev); } this.records.set(instance); this._notifyConfigFieldChanged(instance, field, curr, prev); - this._propagate(instance, field, curr); + this._updateInheritors(instance, field, curr); }); } @@ -203,7 +203,7 @@ export default class InstanceManager { } } config.set("instance.inherited_fields", inherited); - config.update(InstanceManager._resolve(config, template.config, Object.keys(inherited)), false); + config.update(InstanceManager._inheritedValues(config, template.config, Object.keys(inherited)), false); } /** @@ -234,7 +234,7 @@ export default class InstanceManager { inherited[field] = overrides.filter(p => p !== prop); } instance.config.set("instance.inherited_fields", inherited); - this._resolveInto(instance, [field]); + this._copyInheritedValues(instance, [field]); } /** @@ -279,7 +279,7 @@ export default class InstanceManager { } /** Values the given inherited fields take from the template, with object overrides applied */ - private static _resolve(config: lib.InstanceConfig, template: lib.InstanceConfig, fields: string[]) { + private static _inheritedValues(config: lib.InstanceConfig, template: lib.InstanceConfig, fields: string[]) { const inherited = config.get("instance.inherited_fields"); const values: Record = {}; for (const field of fields) { @@ -303,12 +303,12 @@ export default class InstanceManager { } /** Update the given inherited fields of a live instance from its template */ - private _resolveInto(instance: InstanceRecord, fields: string[]): void { + private _copyInheritedValues(instance: InstanceRecord, fields: string[]): void { const template = this._templateOf(instance.config); if (!template) { return; } - const values = InstanceManager._resolve(instance.config, template.config, fields); + const values = InstanceManager._inheritedValues(instance.config, template.config, fields); this._inheriting += 1; try { instance.config.update(values, true); @@ -317,8 +317,8 @@ export default class InstanceManager { } } - /** Track a change to an inherited field made outside of inheritance */ - private _override(instance: InstanceRecord, field: string, curr: unknown, prev: unknown): void { + /** Unlink a field, or the changed properties of an object field, edited on the instance */ + private _unlinkEditedField(instance: InstanceRecord, field: string, curr: unknown, prev: unknown): void { const entry = instance.config.get("instance.inherited_fields")[field]; if (!(entry instanceof Array)) { this.overrideField(instance, field); @@ -333,12 +333,12 @@ export default class InstanceManager { instance.config.set("instance.inherited_fields", inherited); if (removed.length) { // Removed properties fall back to the template value - this._resolveInto(instance, [field]); + this._copyInheritedValues(instance, [field]); } } /** Recompute inherited fields after template_id changed, values equal to the template or default are inherited */ - private _link(instance: InstanceRecord): void { + private _linkMatchingFields(instance: InstanceRecord): void { const template = this._templateOf(instance.config); const inherited: InheritedFields = {}; if (template) { @@ -360,11 +360,11 @@ export default class InstanceManager { } } instance.config.set("instance.inherited_fields", inherited); - this._resolveInto(instance, Object.keys(inherited)); + this._copyInheritedValues(instance, Object.keys(inherited)); } /** Copy a changed value from a template to the instances inheriting it */ - private _propagate(template: InstanceRecord, field: string, value: unknown): void { + private _updateInheritors(template: InstanceRecord, field: string, value: unknown): void { if (!lib.InstanceConfig.isInheritable(field)) { return; } @@ -372,7 +372,7 @@ export default class InstanceManager { if (!instance.config.isInherited(field)) { continue; } - this._resolveInto(instance, [field]); + this._copyInheritedValues(instance, [field]); this._controller.instanceConfigUpdated(instance).catch(err => { logger.error(`Failed to send config of instance ${instance.id} to its host:\n${err.stack}`); }); diff --git a/packages/ctl/src/commands_instance.ts b/packages/ctl/src/commands_instance.ts index c52610c6..ff57d1af 100644 --- a/packages/ctl/src/commands_instance.ts +++ b/packages/ctl/src/commands_instance.ts @@ -39,11 +39,11 @@ instanceCommands.add(new lib.Command({ "id": { type: "number", nargs: 1, describe: "Instance id" }, "from": { type: "number", nargs: 1, describe: "Clone config from instance id" }, "template": { type: "string", nargs: 1, describe: "Template to inherit config from" }, - "is-template": { type: "boolean", nargs: 0, describe: "Create the instance as a template" }, + "as-template": { type: "boolean", nargs: 0, describe: "Create the instance as a template" }, }); }], handler: async function( - args: { name: string, id?: number, from?: number, template?: string, isTemplate?: boolean }, + args: { name: string, id?: number, from?: number, template?: string, asTemplate?: boolean }, ctl: Ctl, ) { let instanceConfig = new lib.InstanceConfig("control"); @@ -56,7 +56,7 @@ instanceCommands.add(new lib.Command({ instanceConfig.set("instance.template_id", await lib.resolveInstance(ctl, args.template)); fields.push("instance.template_id"); } - if (args.isTemplate) { + if (args.asTemplate) { instanceConfig.set("instance.is_template", true); fields.push("instance.is_template"); } diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index 03c602f5..256441db 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -608,6 +608,7 @@ export class InstanceConfig extends classes.Config { access: ["controller", "control"], type: "boolean", initialValue: false, + hidden: true, }, "instance.template_id": { title: "Template", diff --git a/packages/web_ui/src/components/BaseConfigTree.tsx b/packages/web_ui/src/components/BaseConfigTree.tsx index 14b7287a..f352acb4 100644 --- a/packages/web_ui/src/components/BaseConfigTree.tsx +++ b/packages/web_ui/src/components/BaseConfigTree.tsx @@ -4,9 +4,7 @@ import { } from "antd"; import DisconnectOutlined from "@ant-design/icons/DisconnectOutlined"; import LinkOutlined from "@ant-design/icons/LinkOutlined"; -import LockOutlined from "@ant-design/icons/LockOutlined"; import ReloadOutlined from "@ant-design/icons/ReloadOutlined"; -import UnlockOutlined from "@ant-design/icons/UnlockOutlined"; import * as lib from "@clusterio/lib"; @@ -69,7 +67,7 @@ function renderInput(inputComponents: Map, def: lib.Fiel return `Unknown type ${def.type}`; } -/** Lets fields follow the value of another config, inherited values are locked until unlocked */ +/** Lets fields follow the value of another config, inherited values are locked until unlinking */ export type ConfigInheritance = { canInherit: (config: lib.Config, field: string) => boolean; isInherited: (config: lib.Config, field: string, prop?: string) => boolean; @@ -80,10 +78,10 @@ export type ConfigInheritance = { type Linking = Map; -type LockState = { +type LinkState = { inheritance?: ConfigInheritance; - unlocked: Set; - setUnlocked: (unlocked: Set) => void; + unlinking: Set; + setUnlinking: (unlinking: Set) => void; linking: Linking; setLinking: (linking: Linking) => void; }; @@ -103,7 +101,7 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { let [changedFields, setChangedFields] = useState>(new Set()); let [errorFields, setErrorFields] = useState(new Map()); let [applying, setApplying] = useState(false); - let [unlocked, setUnlocked] = useState>(new Set()); + let [unlinking, setUnlinking] = useState>(new Set()); let [linking, setLinking] = useState(new Map()); const available = props.available ?? true; const control = useContext(ControlContext); @@ -170,7 +168,7 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { await props.inheritance!.inherit(field, prop); } setChangedFields(new Set()); - setUnlocked(new Set()); + setUnlinking(new Set()); setLinking(new Map()); } @@ -183,7 +181,7 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { setConfig, props.ConfigClass, onValuesChange, - { inheritance: props.inheritance, unlocked, setUnlocked, linking, setLinking }, + { inheritance: props.inheritance, unlinking, setUnlinking, linking, setLinking }, ); return <> @@ -197,7 +195,8 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { onClick={async () => { setApplying(true); const fields: Record> = {}; - for (const field of changedFields) { + // Unlinked fields are sent even when unchanged so they stop following the template + for (const field of new Set([...changedFields, ...unlinking])) { const value = form.getFieldValue(field); if (propsMap.has(field)) { const [fieldName, prop] = propsMap.get(field)!; @@ -237,7 +236,7 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { props.onApply(); } }} - disabled={changedFields.size === 0 && linking.size === 0} + disabled={changedFields.size === 0 && linking.size === 0 && unlinking.size === 0} > Apply @@ -246,7 +245,7 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { onClick={() => { form.resetFields(); setChangedFields(new Set()); - setUnlocked(new Set()); + setUnlinking(new Set()); setLinking(new Map()); setErrorFields(new Map()); }} @@ -284,7 +283,7 @@ function computeTreeData( setConfig: (newConfig: lib.Config) => void, ConfigClass: BaseConfigTreeProps["ConfigClass"], onValuesChange: (changedValues: object) => void, - lock: LockState, + link: LinkState, ) { let restartTip = void) => -