Fixes #4444: don't duplicate allowusers - rudder-code part #457
Conversation
|
|
||
| parameterizedValueLookupService.lookupRuleParameterization(Seq(allowedUserVar), allNodeInfos, groupLib, directiveLib, allRules) match { | ||
| case Full(variable) => | ||
| varManagedNodesAdmin = varManagedNodesAdmin.copyWithSavedValues(variable.flatMap(x => x.values).toSet.toSeq) |
There was a problem hiding this comment.
you could just use .distinct in place of .toSet.toSeq (remove one set creation)
| case Full(variable) => | ||
| varManagedNodesAdmin = varManagedNodesAdmin.copyWithSavedValues(variable.flatMap(x => x.values).distinct) | ||
| case Empty => | ||
| logger.warn(s"No variable parametrized found for ${varNameForAdminUsers}") |
There was a problem hiding this comment.
Not sure that log should be at warn level. What an user of Rudder can do about it ?
And if he is looking for something strange, it seems likely that he will be in debug mode.
There was a problem hiding this comment.
Moreover, the spec file say that the variable may be empty, so I don't see any reason to have that info above debug level.
There was a problem hiding this comment.
The variable may be empty, but it SHOULD be there
if it is not, then it means the rule for the policy server doesn't exist, and it is really bad.
There was a problem hiding this comment.
hum, the lookupRuleParameterization can return either Full or Failure, so I guess this case is never used
There was a problem hiding this comment.
OK, so could you please explain to the user what he can do to correct the problem, because here, he just get a warning message without any way to change it.
There was a problem hiding this comment.
unfortunately, user cannot do much about this. This is a system Rule and Group, so he cannot recreate it manually easily.
There was a problem hiding this comment.
OK, so at least could you explain what he should do? Look for ? Debug it ? Because if I'm seing a WARN message, I want to check what is happening, and correct it. Here, I (as an user) can't just do anything but be stressed by the message.
There was a problem hiding this comment.
And it's likelly that I won't even understand what the message is about, so it's just garbage for me (as an user)
|
Francois, I've updated the PR |
…lowusers Fixes #4444: don't duplicate allowusers - rudder-code part
No description provided.