Fixes #4292: Make the CFEngine port in ncf configurable

[Kegeruneku]

Ticket: http://www.rudder-project.org/redmine/issues/4292

To be merged in master

[jooooooon]

This looks good in principle but I disagree with the approach partially.

ncf is about making cfengine easy. But having to pass the port number in each body call is a unnecessary effort put on the user. I would prefer to have the ncf body just use the port number from the global config directly. This would be simpler for users and less error prone, also trivial to implement.

Aside from that your copy_from body doesn't actually use it's port parameter, so this won't actually work as is.

No other problems I see though. Good work on adding clear comments.

[Kegeruneku]

I agree with the wrong definition of the body, not using the declared port at all.

However, I don't get the other remarks. The user have NOTHING to do with the port name, and will not even have to see it. It is given by the configuration and editable if needed with sensible defaults, that's all that he will even need to see.

[Kegeruneku]

As you can see in file_copy_from_remote_source_recursion, the port is only used in this bundle and get from the configuration and not from the bundle arguments.

[jooooooon]

I'm suggesting you go one step further: never pass the port number. Your body would therefore look like this:

body copy_from ncf_remote_cp_port_method(from,server,method)
{
  servers     => { "$(server)" };
  source      => "$(from)";
  portnumber  => "${configuration.cfengine_port}";
  compare     => "${method}";
}

I don't see any reason for this not to be the case - the only valid reason for using a parameter to the body is to make the portnumber configurable, but by having it in ncf.conf, it is already configurable.

If you disagree, can you please explain what advantage(s) your approach has over the one I suggest here?

[Kegeruneku]

This approach is cleaner, I'll do it!

[Kegeruneku]

The only setback I see here is that it "violates" a bit the idea of having self contained and independant bodies

[Kegeruneku]

Corrected !

[VinceMacBuche]

Merging