improve ordering of options

[vicinus]

I'm aware that the ordering of options is relevant for haproxy and there are cases were you have to configure the ordering via an array. But I'm lazy and I want the module to do as much work by itself as is possible and custom sorting will remove a lot of warnings automatically.

I think the change will not break existing haproxy setups because the options are already sorted and the change only reorders the options like haproxy internally does in https://github.com/haproxy/haproxy/blob/master/src/cfgparse.c. But I'm no expert so I can be mistaken.

I'm willing to improve the change like make it optional per haproxy instance, add tests or any other improvement necessary.

[antaflos]

This looks interesting and useful IMO! But can you maybe add a comment or two explaining the sort code block for those of us not that well versed in Ruby?

I think the existing tests should pretty much cover this, but there should be probably a parameter added to the haproxy class to enable and disable this more advanced sorting, and it should default to disabled for now, lest it disrupt any running HAProxy setups when upgrading this module.

And I think the server "option" should also be considered when sorting, and always put last. This is useful for cases where haproxy::balancermember is not used but the backend servers are specified directly as options to the haproxy::backend type, as in:

haproxy::backend { 'bk_example_app':
  options => {
    'acl'          => 'allowed_networks src 10.1.0.0/16',
    'http-request' => 'deny unless allowed_networks',
    'server'       => [
      'app01 app01.backend.example.com:8080 check', 
      'app02 app02.backend.example.com:8080 check',
    ]
  }
}

[vicinus]

As I have often trouble finding the right amount of explanation here my first try explaining how the sorting is working:

<%# ruby sorts arrays like strings: the first elements are compared, if they -%>
<%# are equal the next elements are compared and so on. The following sort -%>
<%# provides a two element array, the first element a classification of the -%>
<%# option with a default value of 0 if the option is not found in the -%>
<%# option_order hash and the second element the option itself. -%>

I would add the explanation before the first sort.

I have trouble coming up with a name for the option to enable the "new sorting". At the moment I'm going with sort_options_alphabetic. The default would be true and not use the "new sorting". If someone has a better/shorter idea I'm all ears.

[vicinus]

I added, that the option server is sorted to the end if present. I also added a short documentation how the custom sorting works. Add last and most extensive I added the parameter sort_options_alphabetic to enable the new sorting and updated the documentation regarding the new parameter.

[hunner]

I think this looks good! Could you provide an acceptance test similar to one of the existing ones, but pass sort_options_alphabetic => false to make sure that it works? And a unit test that passes that as well to verify that it compiles.

[vicinus]

I have added a unit test and an acceptance test.

I'm not very happy how the option to enable sort_options_alphabetic globally is coded, but the problem is, that it is possible to only use haproxy::instance without using the class haproxy. So the only class used then is haproxy::params. Another option would be to add another class. I can change the code accordingly if that's preferred.

[hunner]

You're talking about putting the parameter here to globally modify it? Yeah, passing parameters to *::params is not a pattern we use. If we HAVE to have global parameters that apply to all defines, we try to put it in the base class. If that doesn't work for some reason, then we use a *::globals class (iirc, postgresql and mongodb do this).

But yeah, moving it somewhere else would be nice.

[hunner]

Looks like there is a merge conflict also, so can't be merged currently. Thanks for working on this!

[vicinus]

Ok, I have created the class haproxy::globals and moved the parameter sort_options_alphabetic there. I also resolved the merge conflict, but as I never had to resolve a merge conflict in my pull request, I'm not sure if there is anything more to do. Is it possible to squash the merge commit in the previous commit?

[hunner]

Is it impossible to put the sort_options_alphabetic parameter in the base haproxy class and follow this pattern as usual, instead of having a haproxy::globals class? We only use globals when it is otherwise impossible to use the base class.

[vicinus]

It is impossible because you can use the haproxy module without using the haproxy class, only using haproxy::instance resources.

[hunner]

Ah right, because the base class declares a default instance.