configurable zookeeper environment vars …

[lhoss]

(useful p.eg to configure logging to file)

@ernestas-poskus this is the new feature that helps to fix the logging (to file) (ref: #34 (comment) )

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"

[ernestas-poskus]

zookeeper_env will always be defined unless you don't assign anything. Instead check for length, of course loop will not work but lets prevent from entering it.

[lhoss]

good catch! (and better solution to just uncommenting the variable altogether in the defaults (aka making it 'undefined')

[lhoss]

Actually adding the check only in the template (only avoiding 'entering the loop') doesn't bring much (except saving <1ms).
IMO the proper fix is to add the check in the task, avoid to write an empty zookeeper-env.sh (if it's not used). (With this change, one has to be aware that it will be more complicate to move from a file with settings to an emptied env file!)

- name: Configure zookeeper-env.sh
...
  when: zookeeper_env is defined and zookeeper_env|length  > 0

@ernestas-poskus ok for you (to only add that 'when' line ) ?

[ernestas-poskus]

when: zookeeper_env is defined and zookeeper_env|length > 0

even better 👍

[lhoss]

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]

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]

@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]

@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]

committed review-fix

[ernestas-poskus]

neat ! thank you for contribution