Code review comment for lp:~jamesbeedy/charm-haproxy/options_newline

Revision history for this message
Cory Johns (johnsca) wrote :

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

« Back to merge proposal