search

Graylog2 / graylog-ansible-role

Ansible role which installs and configures Graylog
Apache License 2.0
212 stars 127 forks source link

various syntax improvements, Readme amendments and one task refactoring #47

Closed danvaida closed 7 years ago

danvaida commented 7 years ago

This PR is mostly for cosmetic purposes. The changes under this category are:

Readme has been updated to reflect changes and hopefully make more sense from the point of view of installing the role and its dependencies.

The shell task responsible for accepting the Oracle license when installing the JDK has been replaced with a debconf task. This also offers idempotence.

I do realize that some people might haver other standards when it comes to the syntax, but I believe that what I propose makes sense and I'm happy to elaborate if asked to.

CLAassistant commented 7 years ago

CLA assistant check
All committers have signed the CLA.

danvaida commented 7 years ago

PR is going to be updated soon. I just found some issues now. Adding tests to solve this once and for all.

bbaassssiiee commented 7 years ago

Some common best practices for Ansible: Dynamic variables (i.e. Jinja) should have double quotes, variables that embed non-alphanumerics should have single quotes. Plain strings don't need quoting, unless you want to make it a string explicidly.

danvaida commented 7 years ago

@bbaassssiiee I will try to be more specific.

  1. Regarding the quoted booleans, take these lines as an example: https://github.com/Graylog2/graylog-ansible-role/pull/47/files#diff-7eeda618087b49ae876084ab6c73fdbbR30 and https://github.com/Graylog2/graylog-ansible-role/pull/47/files#diff-7eeda618087b49ae876084ab6c73fdbbR164 Although both ways (quoted and unquoted) will work, I proposed to quote the booleans that end up in the configuration files of various services (i.e. elasticsearch, mongo). The booleans used by Ansible in other than templating tasks (such as when conditionals), I proposed to leave unquoted. This is more like a personal preference to differentiate things. However, I can understand and agree to not quoting them as I don't have a very strong opinion on the matter. If people find my logic useful, we can keep it.
  2. Regarding the requirements: indeed, if possible, one should pull them as specific versions, as seen by Galaxy (if the role is in Galaxy).
  3. Regarding your point with double quoting dynamic vars and single quoting vars containing non-alphanumeric chars: can you please point me exactly to where in the code of my PR is this not respected? I'd dare to say the contrary. I also recommend taking a look at this slide in particular: http://www.slideshare.net/DanVaida/ansible-roles-done-right/13?src=clipshare

Anyway, I'll go ahead and amend as according to point 2. above.

bbaassssiiee commented 7 years ago

Point 3 was as a general principle, and you talked about it, so great!

danvaida commented 7 years ago

when I'll have some more time, I plan to make use of the travis build matrix and try to have something like this: https://github.com/saucelabs-ansible/pip#tests. I consider points 2. and 3. as clarified so if you guys are willing to live with 1. this is ready to be merged.

mariussturm commented 7 years ago

@danvaida thanks for your work! Could you please sign the cla in order to get this merged?

danvaida commented 7 years ago

@mariussturm signed and commits amended to reflect the correct author. Next steps will be to add some integration tests and refactor the way the tests are ran with travis. BTW, please edit the .travis.yml to reflect your organisation's build job, not mine :)

mariussturm commented 7 years ago

Thanks a lot for your work!