New custom property support - #2972
Open
raviks789 wants to merge 213 commits into
Open
Conversation
raviks789
force-pushed
the
dictionary-support
branch
9 times, most recently
from
April 2, 2025 10:43
dd916d4 to
f19501d
Compare
raviks789
force-pushed
the
dictionary-support
branch
2 times, most recently
from
April 11, 2025 15:55
2ddc75b to
3583514
Compare
raviks789
force-pushed
the
dictionary-support
branch
10 times, most recently
from
May 14, 2025 15:29
e818e3e to
3260c35
Compare
raviks789
force-pushed
the
dictionary-support
branch
8 times, most recently
from
May 27, 2025 11:27
01bbc41 to
e1fac6d
Compare
Child DictionaryItems are assembled lazily, so allChildrenUnchanged() was hitting isUnchanged() before the 'var' element even existed.
The required flag feature had no documentation at all, so add a section covering how it works on templates versus objects, inherited values, and the fixed-array empty-value fix. Also correct the claim that service sets get a Custom Variables tab, since that never shipped. Note the current scope of sensitive masking, the basket limitation around datalist entries, the migration backfill gap, and that PUT now preserves the required flag on relink. Cleaned up the trailing whitespace in the REST doc while touching that section.
…ting objects, not the template itself
The descendant selector was also matching nested control groups that happen to contain an item-required-checkbox further down, throwing off their alignment.
Share the value cleanup between deleting a property and retyping it away from a dictionary, and match stored icinga_*_var rows by varname instead of property_uuid, since that column is just an optional hint that isn't always populated while varname is the actual key.
updateUsedCustomVarNames had the same property_uuid matching issue as the delete and retype cleanup, so a rename never reached the stored varname or nested key when that column was unset.
updateUsedCustomVarNames only prefixed the parent key when renaming a nested field inside a dynamic dictionary, so a grandchild rename under a fixed dictionary or array looked for the field at the top level and silently missed it. Now reuses resolveRootProperty to build the full path regardless of nesting depth or root type.
Exporter never carried a DirectorProperty's child items or datalist link since they aren't plain DB columns, so both sides of a basket diff always compared an empty tree and every nested change looked like nothing changed. Now Exporter includes them via export(), and BasketDiff restores the snapshot's own items/datalist afterwards instead of trusting a re-export of the live row. ObjectImporter also needed to drop those two keys before setProperties(), same as it already does for fields and customVariables.
Swapped generic keys and values like k, dup_field, and s3cr3t-value for things a real user would actually type, DNS servers, disk thresholds, TLS paths, API keys, template names and the like across the custom variable, migrate, REST API and sensitive element tests.
vlan_id was the only key under management_interface, so removing it collapses management_interface away too, that has always been how removeDictionaryItem cleans up an emptied container. The test wrongly expected it to survive as an empty object.
Reuse the cached target properties instead of reloading each one from the DB, call storeToDb() once per object instead of once per property, and guard the targetProperties lookup with an isset check before reading key_name from it. Added a test covering the restored property_uuid on icinga_host_var.
…n loadCurrentProperties()
str_starts_with() and ucfirst() require their passed arguments to be string.
…st to string Ensure the string arguments for json_decode() and array_key_exists() is explicitly cast to string.
…alculateUuidMap()
…tionary from nesting These container types were only rejected at the top level by the CustomVariableForm dropdown, not by beforeStore() itself, so a basket restore or any other direct write could still persist one nested inside another. dynamic-array stays nestable since it's a legitimate field of the other containers, but it can no longer nest inside itself. Dropped isNestedField from CustomVariableForm, it only existed to gate a second nesting level that no longer applies now that these types are capped at the top level everywhere. Removed the five tests that relied on building a two-level container tree since that shape is no longer legal.
Add a test that drives the guard through import()/store(), the actual basket-restore path the review was worried about, instead of only create(). Add a test exercising the real CustomVariableForm::assemble() (the testable double no-ops it out) to prove the value_type dropdown still gates container types correctly at each nesting level. Add the missing positive case for sensitive under fixed-dictionary, and a dangling parent_uuid case since that column has no FK. Renamed a few test fixtures for readability while touching this area.
…happen parent_uuid had no FK, so deleting a property row directly (e.g. during basket restore) could leave its children pointing at nothing. Add ON DELETE CASCADE on both engines, plus the parent_uuid index Postgres doesn't create automatically for FK columns the way InnoDB does.
The nesting note said a dynamic-array can never nest inside a fixed-array/fixed-dictionary/dynamic-dictionary, which stopped being true once beforeStore() started allowing it - the only nesting it actually blocks is a dynamic-array inside another dynamic-array.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Icinga 2 supports dictionaries as native variable type, but Director had not completely supported the dictionary type custom variables. Operators using service apply-for rules to generate services from dictionary type host custom variables were forced to define those variables outside Director entirely, losing managed configuration. This PR closes both gaps together: Director now supports a dictionary type custom variable and service apply-for rules can iterate over them directly.
Changes
--dry-runand--deleteoptions. The export command includes custom variable definitions.