Fixes #16347: Group properties #2890

fanf · 2020-04-13T02:08:39Z

https://issues.rudder.io/issues/16347
First working version of group properties.
Works:

inheritance of subgroups is undertood,
if a property is defined on two unrelated groups, it's an error (appart if they are exactly the same prop)
group properties can be added by API
saved in Ldap
event log and save in XML
no UI
when a value is a json object overriden by an other json object, json is merged.

Does not work:

global parameters follows same rules

VinceMacBuche · 2020-04-13T09:48:32Z

Hooks permissions were modified ;)

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/nodes/Node.scala

VinceMacBuche · 2020-04-13T12:51:10Z

.../sources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/nodes/NodeGroup.scala

+    name    : String
+  , value   : JValue
+) {
+  def renderValue: String = value match {


Maybe this could be in GenericPropertyUtils

Or having a trait for Properties having this type and then a function in this trait or an implicit class providing this function

I thought a bit to that, but I went to GroupProperties because:

NodeProperties are reasonably different, both in code (because of provider) and in semantic (because the overriding is totally different, they are the only one that matters in the end: they are the only one to go to node

I'm not sure that if we add other properties, they will be the same.

But in current way of doing things, it's rather bad. We have double serialisation for each of them, which mean a change to define incompatible format and make impossible sharing things like the property editor.
So I think it should be done, not sure how (perhaps a property which is just a name + value, and a case class NodeProperty(property: Property, provider: ...) and smart constructor/extractors...)

I think having an inner Property with common behavior for all properties would be interesting, but maybe we can keep it for another PR

Yes, we can. And I'm not sure we can makes much more than what is in GenericPropertyUtils, because for three cases (global param, node prop, group prop), we have 3 differents way of serializing (totally different for param, and node has more info than group). Perhaps there's something that can be done regarding event log, and even there, we have similar problems

VinceMacBuche · 2020-04-17T14:38:19Z

.../rudder-core/src/main/scala/com/normation/rudder/services/policies/MergeNodeProperties.scala

+ * - if the overriding value is a simple value or an array, it replaces the previous one
+ * - if the overriding value is an object and the overriden value is a simple type or an array,
+ *   the latter one is replaced by the former
+ * - if boeth overriding/overriden are an array, overriding values are appended to overriden array


not sure here if we should only keep the overrding values

now, we have both: [overriden, overriding]. I think it may not be what people want, but I also thing that whatever the choice, half people will want the other one

VinceMacBuche · 2020-04-17T14:55:02Z

.../rudder-core/src/main/scala/com/normation/rudder/services/policies/MergeNodeProperties.scala

+ * - if both overriding/overriden are objects, then each key is overriden recursively as explained,
+ *   and new keys are added.
+ */
+


To sum up,

if overriding is:

a value => overrides everything

an array => overrides value and object, append value to overriden array

an object => overrides value and array but if object :
** if key in both object : merge recursive on the values of theses keys
** keep keys that are only in one of the two object (either overriding or overriden)

something like that

VinceMacBuche · 2020-04-17T19:46:11Z

webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/GroupsApi.scala

@@ -148,6 +140,7 @@ class GroupsApi(
      if(req.json_?) {
        req.json match {
          case Full(arg) =>
+            println("****** json:" + compactRender(arg))


You can remove this print

VinceMacBuche · 2020-04-17T21:46:00Z

.../rudder-core/src/main/scala/com/normation/rudder/services/policies/MergeNodeProperties.scala

+    def buildUp(root: GroupProp, all: Map[NodeGroupId, GroupProp]): PureResult[InvertedTree[GroupProp]] = {
+      // recursively takes parent, keeping criterion line order
+      for {
+        parents <- root.parentGroups.accumulatePure { id =>


What happens if there is a cycle between two groups ?

It should not happen, because in this case there is lot of chance that that the groups are empty (if combined with an AND),

or the composition is OR and then the groups are filtered from parents and it will trigger an error in case of common property...

So it should be ok

cycle are forbidden by construction and check when a group is modified

VinceMacBuche · 2020-04-17T21:48:46Z

I think I'm ok with the PR and did not find any problems within the code.

It should be tested live to see how it behaves within a real Rudder and how we can use this brand new properties and its limits !

I can't merge because I think a commit is not named fixup!

VinceMacBuche · 2020-04-17T21:49:10Z

And no issue number in commits too ;)

VinceMacBuche · 2020-04-17T21:49:32Z

And the small feedback about a println too

fanf · 2020-04-20T00:58:22Z

PR updated with a new commit

VinceMacBuche · 2020-04-20T13:40:05Z

You still need to handle the two first commits to have issue number in fisrt one and a fixup! in second commit

Normation-Quality-Assistant · 2020-04-20T15:16:08Z

OK, merging this PR

fanf requested a review from ncharles April 13, 2020 02:08

fanf added WIP Use that label for a Work In Progress PR that must not be merged yet qa: Can't merge labels Apr 13, 2020

fanf requested a review from VinceMacBuche April 13, 2020 02:09

VinceMacBuche reviewed Apr 13, 2020

View reviewed changes

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/nodes/Node.scala Show resolved Hide resolved

VinceMacBuche reviewed Apr 13, 2020

View reviewed changes

fanf removed WIP Use that label for a Work In Progress PR that must not be merged yet qa: Can't merge labels Apr 14, 2020

fanf force-pushed the ust_16347/group_properties branch from 80ae01e to 1279e30 Compare April 16, 2020 16:43

fanf mentioned this pull request Apr 17, 2020

Fixes #16301: Add JSON support for global parameters #2903

Merged

VinceMacBuche reviewed Apr 17, 2020

View reviewed changes

Fixes #16347: Add Group properties to rudder

271dc76

fanf force-pushed the ust_16347/group_properties branch from 12c4aaf to 271dc76 Compare April 20, 2020 15:13

VinceMacBuche added the Ready for merge label Apr 20, 2020

Normation-Quality-Assistant merged commit 271dc76 into Normation:master Apr 20, 2020

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #16347: Group properties #2890

Fixes #16347: Group properties #2890

fanf commented Apr 13, 2020 •

edited

Loading

VinceMacBuche commented Apr 13, 2020

VinceMacBuche Apr 13, 2020

fanf Apr 14, 2020

VinceMacBuche Apr 17, 2020

fanf Apr 17, 2020

VinceMacBuche Apr 17, 2020

fanf Apr 17, 2020

VinceMacBuche Apr 17, 2020

fanf Apr 17, 2020

VinceMacBuche Apr 17, 2020

VinceMacBuche Apr 17, 2020

fanf Apr 19, 2020

VinceMacBuche commented Apr 17, 2020

VinceMacBuche commented Apr 17, 2020

VinceMacBuche commented Apr 17, 2020

fanf commented Apr 20, 2020

VinceMacBuche commented Apr 20, 2020

Normation-Quality-Assistant commented Apr 20, 2020

Fixes #16347: Group properties #2890

Fixes #16347: Group properties #2890

Conversation

fanf commented Apr 13, 2020 • edited Loading

VinceMacBuche commented Apr 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceMacBuche commented Apr 17, 2020

VinceMacBuche commented Apr 17, 2020

VinceMacBuche commented Apr 17, 2020

fanf commented Apr 20, 2020

VinceMacBuche commented Apr 20, 2020

Normation-Quality-Assistant commented Apr 20, 2020

fanf commented Apr 13, 2020 •

edited

Loading