feat: IaC hints, YAML editor for worker configs, and EE badges#7956
feat: IaC hints, YAML editor for worker configs, and EE badges#7956rubenfiszel merged 4 commits intomainfrom
Conversation
Add subtle text notes with doc links to help K8s users discover that instance settings and worker group configs can be managed via the WindmillInstance CRD. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deploying windmill with
|
| Latest commit: |
ee3bd85
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://faf4a4bb.windmill.pages.dev |
| Branch Preview URL: | https://feat-crd-config-hints.windmill.pages.dev |
- Move IaC hint from form view to YAML mode in Instance Settings - Add YAML editor drawer for worker group configs with diff review - Add EE-only badges to gated fields in worker config drawer - Make init_bash available in CE (frontend + backend gate) - Promote Init/Periodic scripts to top-level sections - Add "YAML editor" button to worker config drawer action bar - Strip cache_clear and empty fields from YAML output - Sort default/native groups first in YAML editor Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rubenfiszel
changed the title
|
Claude finished @rubenfiszel's task in 1m 56s —— View job PR Review: IaC hints, YAML editor for worker configs, and EE badges
SummaryThis PR makes several good UX improvements: the YAML editor for bulk worker config management is a useful addition, EE badges add clarity for CE users, and promoting init script to CE is a reasonable change. The code is generally clean and follows existing patterns. Issues FoundMedium — Duplicated config-cleaning logic (inline comment) Medium — Medium — No protection against deleting well-known groups (inline comment) Low — No loading indicator during YAML save (inline comment) Low — Missing restart warning for periodic script (inline comment) Nit — Inconsistent event handler syntax (inline comment) Backend ReviewThe Rust change ( What Looks Good
Testing InstructionsNavigate to the Workers page ( |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| @@ -129,10 +129,11 @@ async fn update_config( | |||
|
|
|||
| #[cfg(not(feature = "enterprise"))] | |||
There was a problem hiding this comment.
When constructing the CE config, you allow the 'init_bash' field similar to 'worker_tags' and 'cache_clear'. Consider adding additional validation (e.g. type or length checks) for 'init_bash' if needed to prevent potential misuse.
| let yamlConfigOriginal = $state('') | ||
| let yamlDiffMode = $state(false) | ||
|
|
||
| function openYamlDrawer() { |
There was a problem hiding this comment.
In the openYamlDrawer function, worker groups are sorted with priority for 'default' and 'native'. The sorting logic is clear; however, consider encapsulating this logic in a helper for reuse and ensure that string comparisons are case‐consistent if needed.
| const oldNames = new Set(Object.keys(workerGroups ?? {})) | ||
| const newNames = new Set(newGroups.keys()) | ||
|
|
||
| // Delete removed groups |
There was a problem hiding this comment.
In the saveYamlConfig function, each removal and update of a worker group configuration issues separate API calls. For scalability, consider batching these requests to reduce latency when many groups are modified.
| {#if nconfig !== undefined} | ||
| <div class="mt-8"></div> | ||
| <Label label="Alerts" tooltip="Alert is sent to the configured critical error channels"> | ||
| <Label label="Alerts" tooltip="Alert is sent to the configured critical error channels" eeOnly={!hasEnterpriseFeatures}> |
There was a problem hiding this comment.
Typographical note: The tooltip text reads "Alert is sent to the configured critical error channels", which is inconsistent with the label "Alerts". Consider using a plural form (e.g., "Alerts are sent to the configured critical error channels") for clarity.
|
|
||
| <div class="mt-8"></div> | ||
| <Section label="Python dependencies overrides" collapsable={true} class="flex flex-col gap-y-6"> | ||
| <Section label="Python dependencies overrides" collapsable={true} class="flex flex-col gap-y-6" eeOnly={!hasEnterpriseFeatures}> |
There was a problem hiding this comment.
It looks like the attribute 'collapsable' might be a typographical error. Typically, it is spelled 'collapsible'. Please verify if this was intended or if it should be corrected.
| on:click={() => { yamlDiffMode = false }} | ||
| >Back to editor</Button> | ||
| <Button | ||
| unifiedSize="md" |
There was a problem hiding this comment.
Typographical inconsistency: In this hunk, the event handler is specified as onClick (with a capital C) whereas elsewhere on:click is used. Consider using on:click to maintain consistency with the Svelte convention.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const workersConfig = Object.entries(workerGroups).map( | ||
| ([name, { cache_clear, ...config }]) => { | ||
| const cleaned: Record<string, any> = { name } | ||
| for (const [k, v] of Object.entries(config)) { | ||
| if (v == null) continue | ||
| if (Array.isArray(v) && v.length === 0) continue | ||
| if (typeof v === 'object' && !Array.isArray(v) && Object.keys(v).length === 0) continue | ||
| cleaned[k] = v | ||
| } | ||
| return cleaned | ||
| } | ||
| ) |
There was a problem hiding this comment.
Duplicated config-cleaning logic. This inline cleanup block (lines 788–799) duplicates the same logic in openYamlDrawer() (lines 393–411). If the cleaning rules ever change (e.g. stripping more fields), they'd need to be updated in two places. Consider extracting a shared helper like cleanWorkerConfig(name, config).
| const workersConfig = Object.entries(workerGroups).map( | |
| ([name, { cache_clear, ...config }]) => { | |
| const cleaned: Record<string, any> = { name } | |
| for (const [k, v] of Object.entries(config)) { | |
| if (v == null) continue | |
| if (Array.isArray(v) && v.length === 0) continue | |
| if (typeof v === 'object' && !Array.isArray(v) && Object.keys(v).length === 0) continue | |
| cleaned[k] = v | |
| } | |
| return cleaned | |
| } | |
| ) | |
| const workersConfig = Object.entries(workerGroups).map( | |
| ([name, config]) => cleanWorkerGroupConfig(name, config) | |
| ) |
(where cleanWorkerGroupConfig is a shared function also used by openYamlDrawer).
| // Delete removed groups | ||
| for (const name of oldNames) { | ||
| if (!newNames.has(name)) { | ||
| await ConfigService.deleteConfig({ name: 'worker__' + name }) | ||
| } | ||
| } | ||
|
|
||
| // Create or update groups | ||
| for (const [name, config] of newGroups) { | ||
| await ConfigService.updateConfig({ | ||
| name: 'worker__' + name, | ||
| requestBody: config | ||
| }) | ||
| } |
There was a problem hiding this comment.
No loading state during sequential API calls. The save function fires sequential deleteConfig and updateConfig calls without showing a loading indicator. If there are many groups this could take a while with no feedback. Consider adding a loading state (let yamlSaving = $state(false)) and disabling the Save button while saving.
Also, the operations are not atomic — if one updateConfig call fails midway, you'll have a partially applied state. This may be acceptable for an admin-level feature, but worth noting.
| sendUserToast('Each entry must have a "name" field', true) | ||
| return | ||
| } | ||
| newGroups.set(c.name, { ...c, name: undefined }) |
There was a problem hiding this comment.
name property leaks into the config object. Setting name: undefined via spread keeps the name key in the object (with value undefined), which may serialize as "name": null depending on JSON handling. A cleaner approach:
| newGroups.set(c.name, { ...c, name: undefined }) | |
| const { name: _, ...rest } = c | |
| newGroups.set(c.name, rest) |
| // Delete removed groups | ||
| for (const name of oldNames) { | ||
| if (!newNames.has(name)) { | ||
| await ConfigService.deleteConfig({ name: 'worker__' + name }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential accidental deletion of protected groups. If a user accidentally removes default or native from the YAML, this will delete those groups. Consider adding a confirmation dialog before deleting groups, or at minimum warning when well-known groups (default, native) would be deleted.
| variant="default" | ||
| on:click={() => { yamlDiffMode = false }} | ||
| >Back to editor</Button> | ||
| <Button |
There was a problem hiding this comment.
Nit: Inconsistent event handler syntax — this uses onClick (prop) while the buttons above use on:click (event). Both work, but mixing them in the same component is a bit inconsistent.
| {#if (nconfig.init_bash ?? '') !== (config?.init_bash ?? '')} | ||
| <div class="mb-2"> | ||
| <Alert size="xs" type="info" title="Worker restart required"> | ||
| Workers will get killed upon detecting any changes in this section (scripts or | ||
| interval). It is assumed they are in an environment where the supervisor will restart | ||
| them. | ||
| Workers will get killed upon detecting changes to the init script. It is assumed they | ||
| are in an environment where the supervisor will restart them. | ||
| </Alert> | ||
| </div> |
There was a problem hiding this comment.
The "Worker restart required" alert previously covered both init and periodic script changes. Now that they're split into separate sections, this alert only appears for init script. The periodic script section (below) no longer shows a restart warning at all when its content changes. Was this intentional? If periodic script changes also trigger worker restarts, a similar alert should be added there.
- Extract serializeWorkerGroupsAsYaml() shared helper (dedup) - Fix name:undefined leak using proper destructuring - Protect default/native groups from accidental deletion - Add loading indicator during YAML save - Add restart warning for periodic script changes - Use consistent on:click event syntax in YAML drawer actions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Improves the IaC/YAML experience for worker group configuration and adds clarity to EE-gated fields in the worker config drawer.
Changes
defaultandnativegroups firstcache_clearand empty fields (priority_tags: {},env_vars_allowlist: [], etc.)EEbadges to disabled fields in the worker config drawer (Workers assignment, High-priority tags, Alerts, Env vars, Autoscaling, Python deps, Periodic script)windmill-api-configs), promoted Init Script and Periodic Script to top-level sections, added contextual info about runs page visibility and script editor testingTest plan
Generated with Claude Code