search

AnsibleShipyard / ansible-zookeeper

Ansible playbook for ZooKeeper
MIT License
95 stars 114 forks source link

configurable zookeeper environment vars … #39

Closed lhoss closed 8 years ago

lhoss commented 8 years ago

(useful p.eg to configure logging to file)

@ernestas-poskus this is the new feature that helps to fix the logging (to file) (ref: https://github.com/AnsibleShipyard/ansible-zookeeper/pull/34#issuecomment-238792967 )

Finally I added no settings in the defaults (though this would be useful, also for the purpose of a working example )

To enable for ex. file based logging, you would define following var:

zookeeper_env:
  ZOO_LOG_DIR: "/var/log/zookeeper"
  ZOO_LOG4J_PROP: "INFO,ROLLINGFILE"
lhoss commented 8 years ago

fixed above comment

@ernestas-poskus plz notice however another oddity is that this addition does not apply to the 'old' debian (deb pkg) setup. IMO it would be nicer to also add it there. If so, I'ld propose to first extract the now 3 config-files tasks into a separate include, which is then included in the 3 supported cases (debian-deb, debian-tarball, redhat-tarball ). Ok for you?

lhoss commented 8 years ago

If so, I'ld propose to first extract the now 3 config-files tasks into a separate include, which is then included in the 3 supported cases (debian-deb, debian-tarball, redhat-tarball ).

but maybe better merge this one first, and I open a separate PR for that change

ernestas-poskus commented 8 years ago

@lhoss maybe just extract this new logic into common.yml (leave conditionals inside of it) and include it in main.yml. In later PR if you want you can extract further common logic into common.yml

lhoss commented 8 years ago

@lhoss maybe just extract this new logic into common.yml (leave conditionals inside of it) and include it in main.yml. In later PR if you want you can extract further common logic into common.yml

OK then, will do your proposal (but naming the new include common-config.yml) and definitely do another PR (to extract the other 2 config task, into that new include), IMO much nicer to have all the config task again in the same include (ps: there I'ld like to add a new tag 'config' .. even though it's similar then 'deploy' )

lhoss commented 8 years ago

committed review-fix

ernestas-poskus commented 8 years ago

neat ! thank you for contribution