Merge lp:~jamesbeedy/charm-haproxy/options_newline into lp:charm-haproxy

[james beedy]

Add backend options on new line.

Fixes: https://bugs.launchpad.net/charm-haproxy/+bug/1770866

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Cory Johns (johnsca)]

I didn't realize I was still associated with this charm at all, much less requested on this review. I have to admit, I haven't done any work on this charm and was only associated with it because I changed the owner during a promulgation request. Apologies for missing the review request for so long.

From what I was able to gather from reading up on haproxy's configuration, this MR doesn't actually seem correct. Specifically, I think that it will break server-line settings[1] that do in fact need to go on the same line.

Really, it seems that the way the haproxy charm handles server_options[2] conflates server-line settings with options that need to be on their own line, making it basically impossible to handle both in a reasonable way.

Perhaps an "easy" fix would be to extend the list of known options[3] to include the ones that are failing, but I really think that create_listen_stanza should be refactored to avoid this sort of issue going forward.

That said, I'm really not very familiar with the charm, so I could be completely off base, so I've added the haproxy-team and specifically Tom Haddon to the review requests, as they will be much more knowledgeable about the correct solution for this.

[1]: https://cbonte.github.io/haproxy-dconv/1.7/configuration.html#5.2
[2]: https://bazaar.launchpad.net/~haproxy-team/charm-haproxy/trunk/view/head:/hooks/hooks.py#L350
[3]: https://bazaar.launchpad.net/~haproxy-team/charm-haproxy/trunk/view/head:/hooks/hooks.py#L58

review: Needs Fixing

[james beedy (jamesbeedy)]

@johnsca I'll give this a second review/revise per your notes here. Thanks for the bump

Unmerged revisions

114. By james beedy on 2018-05-12

Make sure backend 'options' get a new line.

Fixes: #1770866

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2016-11-22 11:15:13 +0000
+++ hooks/hooks.py	2018-05-12 16:21:32 +0000
@@ -419,14 +419,12 @@
                 server_options) in enumerate(server_entries):
             server_line = "    server %s %s:%s" % \
                 (server_name, server_ip, server_port)
+            server_line = server_line.format(i=i)
+            service_config.append(server_line)
             if server_options is not None:
                 if isinstance(server_options, basestring):
-                    server_line += " " + server_options
-                else:
-                    server_line += " " + " ".join(server_options)
-            server_line = server_line.format(i=i)
-            service_config.append(server_line)
-
+                    service_config.append("    {}".format(server_options))
+ 
 
 # -----------------------------------------------------------------------------
 # create_monitoring_stanza:  Function to create the haproxy monitoring section