Implement suggested changes from #40

[TwizzyDizzy]

This PR refers to #40.

Alright, have a look at these commits... my remarks:

• not sure, whether this checking-foo in the templates could (and maybe even should) be done elsewhere or whether it is even good practice
• concerning the TLS commit: if one adds this possibility, I would think that it might be wise to also implement the according changes in templates/mongod.conf.j2 to have the server even speak TLS. Do you have any thoughts on this? Not sure if this is a road one wants to go down, as it implicates a lot of choices and a decision tree that might not by trivial.

Cheers
Thomas

[xenithorb]

Thanks for the contribution! I now see what you're doing here and I'm totally cool with merging it when we get it where I like it. To start off:

not sure, whether this checking-foo in the templates could (and maybe even should) be done elsewhere or whether it is even good practice

Your hunch is correct about this. Instead of the conditional checking, ansible basically has a built in mechanism to prioritize variable assignments. Defaults are at the bottom of the barrel and can always be overridden. Which is to say, anything in defaults/main.yml can and should be overriden by the users of the role. In addition, anything that you define as variable that you intend to be overridden should be present in defaults and defined with your preferred default, even if it's null. This is where people generally look to override options. Leveraging defaults also has the added benefit of deduplicating your jinja2 code there. Am I correct that you've done python2/jinja2 before ansible?

So, the logic that you've worked through here is sound and can be replicated by using default() and ternary() directly in the defaults file defaults/main.yml where need be. Though, my cursory look of this is that the majority of it isn't necessary (the conditionals).

For example:

{% if rocket_chat_mongodb_user is defined and rocket_chat_mongodb_password is defined %}
{% set rocket_chat_mongodb_credentials = rocket_chat_mongodb_user+":"+rocket_chat_mongodb_password+"@" -%}
{% else %}
{% set rocket_chat_mongodb_credentials = "" -%}
{% endif %}
{% if rocket_chat_mongodb_database is defined %}
{% set rocket_chat_mongodb_database = rocket_chat_mongodb_database -%}
{% else %}
{% set rocket_chat_mongodb_database = "rocketchat" -%}
{% endif %}
{% if rocket_chat_mongodb_use_tls == true %}
{% set rocket_chat_mongodb_use_tls = "?ssl=true" -%}
{% else %}
{% set rocket_chat_mongodb_use_tls = "?ssl=false" -%}
{% endif %}

Should end up looking something like this in defaults/main.yml:

rocket_chat_mongodb_user: ~
rocket_chat_mongodb_pass: ~
rocket_chat_mongodb_creds:
  "{{ 
       ((rocket_chat_mongodb_user is defined) and (rocket_chat_mongodb_pass is defined))
         | ternary(rocket_chat_mongodb_user ~ ':' ~ rocket_chat_mongodb_pass ~ '@', omit)
   }}"
rocket_chat_mongodb_database: rocketchat
rocket_chat_mongodb_use_tls: false
rocket_chat_mongodb_URI: 
  "{{ rocket_chat_mongodb_creds ~ rocket_chat_mongodb_server ~ 
       ':' ~ rocket_chat_mongodb_port ~ '/' ~ rocket_chat_mongodb_database ~
       ( rocket_chat_mongodb_use_tls | ternary('?ssl=true', omit) )
   }}"

Then you either just need to paste the string I have for mongodb_URI into the template and get rid of it in defaults, or put {{ rocket_chat_mongodb_URI }} in place of the others.

You can also leave it so where not all of it is done inside a single {{ }} if you'd like. It's nothing more than a style choice in this case. It would probably look something like this in that case:

env MONGO_URL="mongodb://{{ rocket_chat_mongodb_credentials ~ rocket_chat_mongodb_server }}:{{ rocket_chat_mongodb_port }}/{{ rocket_chat_mongodb_database ~ ( rocket_chat_mongodb_use_tls | ternary('?ssl=true',None) ) }}"

Note:

1. Possibly some of that logic can be moved out to the template itself (Perhaps you never define rocket_chat_mongodb_URI and do it right in the template?)
2. None may need to be used in place of where I put omit I forget at the moment which works better, my inclination is None probably, but I put omit there in case you were unaware of its existence
3. Did not test this at all
4. There is a general inclination to keep defaults as free of logic as possible - which is contrary to what I've just done above - sometimes I fail to see a better spot for it. Combined variables I would generally put into a ./vars/ file, maybe we need one specifically for mongodb that we can include at the beginning of tasks/mongodb.yml

---

In the end what I want to see more of is less duplicated code and more usage of the defaults file. Thanks!

[xenithorb]

Forgot to address:

concerning the TLS commit: if one adds this possibility, I would think that it might be wise to also implement the according changes in templates/mongod.conf.j2 to have the server even speak TLS. Do you have any thoughts on this? Not sure if this is a road one wants to go down, as it implicates a lot of choices and a decision tree that might not by trivial.

My present knowledge that I can recall of mongo isn't so great at the moment, but the general direction I would lean is:

1. UNIX sockets if at all possible, that's what I'd really like to see on monolithic deploys and rely on filesystem permissions. Bypass the TCP/IP stack completely.
2. Then, if necessary, maybe we can see about implementing TLS by default on the local machine

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ engelgabriel
❌ TwizzyDizzy

---

TwizzyDizzy seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}