Fixes #8924: Policy mode API (Global, Directive, Node) #1189

VinceMacBuche · 2016-09-05T09:41:59Z

https://www.rudder-project.org/redmine/issues/8924

VinceMacBuche · 2016-09-07T15:49:11Z

Commit modified

VinceMacBuche · 2016-09-07T16:38:45Z

PR rebased

fanf · 2016-09-09T14:20:33Z

rudder-core/src/main/scala/com/normation/rudder/domain/policies/Directive.scala

+     * Can not be empty nor null.
+     */
+  , shortDescription : String
+  , policyMode : Option[PolicyMode]


could you adda a description like for other parameters here?

VinceMacBuche · 2016-09-09T15:15:21Z

Commit modified

VinceMacBuche · 2016-09-12T08:56:04Z

Commit modified

VinceMacBuche · 2016-09-12T08:59:03Z

Commit modified

ncharles · 2016-09-12T10:15:56Z

rudder-core/src/main/resources/ldap/rudder.schema

@@ -484,7 +493,7 @@ objectclass ( RudderObjectClasses:23
  STRUCTURAL
  MUST ( directiveId $ techniqueVersion )
  MAY ( cn $ description $ longDescription $ isEnabled $ isSystem $
-        directivePriority $ directiveVariable  ) )
+        directivePriority $ directiveVariable $ policyMode ) )


you need to update also the 099-1-rudder.ldif file in test/resources/ldap-data/schema

good catch!

VinceMacBuche · 2016-09-12T10:30:58Z

PR rebased

ncharles · 2016-09-12T10:33:43Z

rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPEntityMapper.scala

@@ -558,6 +583,9 @@ class LDAPEntityMapper(
    entry +=! (A_PRIORITY, directive.priority.toString)
    entry +=! (A_IS_ENABLED, directive.isEnabled.toLDAPString)
    entry +=! (A_IS_SYSTEM, directive.isSystem.toLDAPString)
+    for {
+      mode <- directive.policyMode
+    }  entry +=! (A_POLICY_MODE, mode.name)


why this construct ? (compared to a map, or a yield ?)

It's foreach syntax with a for

I don't need the result, so don't need yield/flatmap or map

the readability is poor compared to
directive.policyMode.map( x => entry +=! (A_POLICY_MODE, x.name))

Could do it with a foreach, but i prefer the for way

VinceMacBuche · 2016-09-12T10:39:09Z

PR rebased

ncharles · 2016-09-12T10:42:42Z

rudder-web/src/main/scala/com/normation/rudder/web/rest/node/NodeAPI8.scala

@@ -0,0 +1,112 @@
+/*
+*************************************************************************************
+* Copyright 2013 Normation SAS


2016, isn't it ?

VinceMacBuche · 2016-09-12T10:56:42Z

Commit modified

VinceMacBuche · 2016-09-12T11:02:43Z

Commit modified

VinceMacBuche · 2016-09-12T14:01:41Z

Commit modified

ncharles · 2016-09-15T08:28:50Z

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

-    id         : NodeId
-  , modAgentRun: Option[SimpleDiff[Option[AgentRunInterval]]]
-) extends NodeDiff with HashcodeCaching
+object ModifyNodeAgentRunDiff{


why replacing the case class by an object ?

The goal (discussed with @fanf) is to remove the old events (ModifyNodeAgentRun, ModifyNodeProperties ModifyNodeHeartbeat) by one and only one event that is 'ModifyNode'

I removed possibility to save/create those events but we need to be able to read previous events

Those object are only used as syntax sugar when we are reading the old event (Could be replaced by ModifyNode(agentRun,None,None,None)

ncharles · 2016-09-15T08:29:39Z

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

 )

+object ModifyNodeDiff {


why both a case class and an object ??

ncharles · 2016-09-15T08:31:49Z

rudder-core/src/main/scala/com/normation/rudder/repository/EventLogRepository.scala

-    , principal: EventActor
-    , modifyDiff: ModifyNodePropertiesDiff
-    , reason:Option[String]) = {
+  def saveModifyNode(


i think i prefered the name NodeProperties, as modify node leads me to think we are changing the node, and not properties attached to it

We are changing the node now, not only 'properties'

ncharles · 2016-09-15T08:33:45Z

rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPNodeRepository.scala

-   * If the node is a system one, the methods fails.
-   */
-  private[this] def update(nodeId: NodeId, updateNode: Node => Node, logAction: (Node, Node) => Box[EventLog]) : Box[Node] = {
+  def updateNode(node: Node, modId: ModificationId, actor:EventActor, reason:Option[String]) : Box[Node] = {


again, here i tend to think the method change a node

ncharles · 2016-09-15T08:36:06Z

rudder-core/src/main/scala/com/normation/rudder/services/eventlog/EventLogDetailsService.scala

+  def getModifyNodeDetails(xml:NodeSeq) : Box[ModifyNodeDiff] = {
+    for {
+      entry <- getEntryContent(xml)
+      node                    <- (entry \ "node").headOption ?~! ("Entry type is not node : " + entry)


could you align the <- please ?

ncharles · 2016-09-15T08:41:16Z

rudder-web/src/main/scala/com/normation/rudder/web/components/ShowNodeDetailsFromNode.scala

+    case eb : EmptyBox =>
+      val e = eb ?~! s"Could not get Node '${nodeId.value}' details"
+      logger.error(e.messageChain)
+      throw new Exception(e.messageChain)


i'm really not convinced by throwing an exception when loading the page.
With this, we go from a safe-ish approach where the error The node with id '${nodeId.value}' was not found was displayed to user to an exception, that won't probably be of any help to user :/

VinceMacBuche · 2016-09-15T13:08:17Z

Commit modified

fanf

That seems OK for me. Please rebase the commit and merge !

VinceMacBuche · 2016-09-19T08:03:54Z

PR rebased

Normation-Quality-Assistant · 2016-09-19T10:40:29Z

OK, merging this PR

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch from 467f3a1 to c721692 Compare September 7, 2016 15:49

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch from c721692 to 0279871 Compare September 7, 2016 16:38

fanf reviewed Sep 9, 2016
View reviewed changes

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch 2 times, most recently from 9a7640f to 2124ece Compare September 12, 2016 08:56

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch from 2124ece to 0ae6f60 Compare September 12, 2016 08:59

ncharles reviewed Sep 12, 2016
View reviewed changes

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch from f2e3bf1 to 5c03ec9 Compare September 12, 2016 10:30

ncharles reviewed Sep 12, 2016
View reviewed changes

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch from 5c03ec9 to 388c448 Compare September 12, 2016 10:39

ncharles reviewed Sep 12, 2016
View reviewed changes

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch 2 times, most recently from bf21a01 to 77f1963 Compare September 12, 2016 11:02

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch from 77f1963 to 1c5fa7c Compare September 12, 2016 14:01

ncharles reviewed Sep 15, 2016

View reviewed changes

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch from 5c338cd to 30a354c Compare September 15, 2016 13:08

fanf approved these changes Sep 16, 2016

View reviewed changes

Fixes #8924: Policy mode API (Global, Directive, Node)

bcff59a

VinceMacBuche force-pushed the ust_8924/policy_mode_api_global_directive_node branch from 30a354c to bcff59a Compare September 19, 2016 08:03

fanf added the Ready for merge label Sep 19, 2016

Normation-Quality-Assistant merged commit bcff59a into Normation:master Sep 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #8924: Policy mode API (Global, Directive, Node) #1189

Fixes #8924: Policy mode API (Global, Directive, Node) #1189

VinceMacBuche commented Sep 5, 2016

VinceMacBuche commented Sep 7, 2016

VinceMacBuche commented Sep 7, 2016

fanf Sep 9, 2016

VinceMacBuche commented Sep 9, 2016

VinceMacBuche commented Sep 12, 2016

VinceMacBuche commented Sep 12, 2016

ncharles Sep 12, 2016

VinceMacBuche Sep 12, 2016

VinceMacBuche commented Sep 12, 2016

ncharles Sep 12, 2016

VinceMacBuche Sep 12, 2016

ncharles Sep 12, 2016

VinceMacBuche Sep 12, 2016

VinceMacBuche commented Sep 12, 2016

ncharles Sep 12, 2016

VinceMacBuche commented Sep 12, 2016

VinceMacBuche commented Sep 12, 2016

VinceMacBuche commented Sep 12, 2016

ncharles Sep 15, 2016

VinceMacBuche Sep 15, 2016

VinceMacBuche Sep 15, 2016

ncharles Sep 15, 2016

ncharles Sep 15, 2016

VinceMacBuche Sep 15, 2016

ncharles Sep 15, 2016

ncharles Sep 15, 2016

ncharles Sep 15, 2016

VinceMacBuche commented Sep 15, 2016

fanf left a comment

VinceMacBuche commented Sep 19, 2016

Normation-Quality-Assistant commented Sep 19, 2016

Fixes #8924: Policy mode API (Global, Directive, Node) #1189

Fixes #8924: Policy mode API (Global, Directive, Node) #1189

Conversation

VinceMacBuche commented Sep 5, 2016

VinceMacBuche commented Sep 7, 2016

VinceMacBuche commented Sep 7, 2016

Choose a reason for hiding this comment

VinceMacBuche commented Sep 9, 2016

VinceMacBuche commented Sep 12, 2016

VinceMacBuche commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceMacBuche commented Sep 12, 2016

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 Sep 12, 2016

Choose a reason for hiding this comment

VinceMacBuche commented Sep 12, 2016

VinceMacBuche commented Sep 12, 2016

VinceMacBuche commented Sep 12, 2016

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 Sep 15, 2016

fanf left a comment

Choose a reason for hiding this comment

VinceMacBuche commented Sep 19, 2016

Normation-Quality-Assistant commented Sep 19, 2016