Fixes #26325: Resolved properties conflicts still appear as errors in status #6179
Conversation
There was a problem hiding this comment.
There's just some comments to address, but the logic seems correct.
Still, it shows that we need to rework the underlying datastructures. Our property repo should likely holds the linearized structures for all nodes and group in a better data structure that what we have
...udder-core/src/test/scala/com/normation/rudder/services/nodes/TestMergeGroupProperties.scala
Outdated
Show resolved
Hide resolved
...es/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/properties/Properties.scala
Outdated
Show resolved
Hide resolved
.../rudder/rudder-core/src/main/scala/com/normation/rudder/properties/MergeNodeProperties.scala
Show resolved
Hide resolved
| val resolved = err.conflicts.toList.flatMap { | ||
| case (nodeProp, parentProps: NonEmptyChunk[List[ParentProperty]]) => | ||
| for { | ||
| // reverse to get the parent which has the highest priority | ||
| firstParent <- groupProp.parentGroups.reverse.headOption | ||
| parent <- allGroupProps.get(firstParent) | ||
| // we assume that there is a single property value for the same name (upstream we have List and not a Map) | ||
| parentProp <- parent.properties.find(_.name == nodeProp.name) | ||
| // parentProps probably needs to be a NonEmptyChunk(VerticalPropTree[{node=(1),groups=(*),global=(1)}]) | ||
| // and what we want is a VerticalPropTree that has the parent group as its dominant group i.e. most prioritized groups, the last one | ||
| // we probably want to have this natural subdivision, such that the Ordering[ParentProperty] will disappear, as it's explicitly : .node, .groups (already sorted), .global | ||
| parentHierarchy <- parentProps.collectFirst { | ||
| case l | ||
| if l.reverse.collectFirst { case g: ParentProperty.Group => g } | ||
| .map(_.id) | ||
| .contains(parent.groupId) => | ||
| l | ||
| } | ||
| } yield { | ||
| parentProp -> parentHierarchy | ||
| } | ||
| } |
There was a problem hiding this comment.
use a for-comprehension on Option :
- if
None, the conflict is not resolved - the type
NonEmpty[List[ParentProperty]]is very confusing,List[ParentProperty]seems to have some logic and structure in it, but it would be too big to refactor this part now :- I left a comment about what it means in this specific context : the list contains the parent group properties
ParentProperty.Group, which are conflicting, and the last of them is the winner - the non-empty chunk is a list of "hierarchies", i.e. multiple stacks of node-groups-global for the same property
- I left a comment about what it means in this specific context : the list contains the parent group properties
The current algorithm is limited to the "last direct parent", but the parent itself may have parents and resolve from other parents, it does not look like a real use case, so we can leave it as is :
There was a problem hiding this comment.
yes, the refactoring should go in 8.3, because it's a fairly invasive one and it is likely to have user facing impact
There was a problem hiding this comment.
also, it would really help if we had a map instead of a List :
case class NodeGroup(
...
properties: Map[PropertyName, GroupProperty]
), there are currently many assumptions with the list, and we always use it implicitly as a map.
But this would also be a huge change
…rors in status Fixes #26325: Resolved properties conflicts still appear as errors in status
|
PR updated with a new commit |
…r as errors in status Fixes #26325: Resolved properties conflicts still appear as errors in status
|
PR updated with a new commit |
…l appear as errors in status Fixes #26325: Resolved properties conflicts still appear as errors in status
|
PR updated with a new commit |
https://issues.rudder.io/issues/26325
The logic for overriding properties did not exist yet for groups : until now the specific case of a group defining the order of override of properties from its parents has always resulted in a conflict for the group.
We need to handle this case : to resolve the conflicts (on the left part) we look at the parent properties, and for a given property, we look at the most prioritized parent that defines it (and we leave it as a conflict if there isn't any).