devops-coop / ansible-haproxy

Installs and configure HAProxy
Apache License 2.0
96 stars 97 forks source link

Defaults log #51

Closed ghost closed 7 years ago

ghost commented 8 years ago

There's a difference between the documentation and the template: https://github.com/FloeDesignTechnologies/ansible-haproxy/blob/master/vars/main.yml#L26 https://github.com/FloeDesignTechnologies/ansible-haproxy/blob/master/templates/defaults.cfg#L6-L8

By the way, in this template implementation if we want to generated in the /etc/haproxy/haproxy.cfg this line:

defaults
  log global

We have to code the yaml file:

haproxy_defaults:
  log:
    - address: global
      facility: ' '

This trick is a bit uggly. Maybe (like you did for default_backend and use_backend) you can add something in order to code like that:

haproxy_defaults:
  log: global
r-daneel commented 7 years ago

Would making the facility parameter optional solve this issue without breaking the construct ? I agree we should fix the template to reflect the change of structure.

benwebber commented 7 years ago

Would making the facility parameter optional solve this issue without breaking the construct ?

That would maintain the existing contract 👍 .


Honestly, the current construct is quite verbose. I think a simple string/boolean would be more flexible.

String:

haproxy_defaults:
  log: 127.0.0.1:514 local2
defaults
  log 127.0.0.1:514 local2

Boolean (to disable logging):

haproxy_defaults:
  log: false
defaults
  no log

Perhaps in version 2?

r-daneel commented 7 years ago

The construct is indeed a bit verbose, but as you mention, the contract is out and we are bound to it. Also, IMHO, I'd try to avoid allowing unformatted strings when a clear and documented syntax exists.

For now, just making the facility parameter optional should help avoiding the workaround described.

As for changing the contract for a future version, I'd discuss that more in depth once we're ready to design such evolutions.