search

DavidWittman / ansible-redis

Highly-configurable Ansible role to install Redis and Redis Sentinel from source
MIT License
663 stars 288 forks source link

Allow the user to disable vm.overcommit_memory #254

Closed Migsi closed 2 years ago

Migsi commented 4 years ago

This feature is useful, when the user wants to install redis inside a container, where this particular systemctl cannot be set, due to beeing read only. Simply not defining the config key redis_inside_container by default, nothing should break and backward compatibility should be maintained. Also this way it should never happend to be toggled by accident. Description is to be added in README.md

825i commented 3 years ago

Doesn't the var redis_inside_container need to also be added to ansible-redis/blob/master/defaults/main.yml?

Migsi commented 3 years ago

Doesn't the var redis_inside_container need to also be added to ansible-redis/blob/master/defaults/main.yml?

The idea behind this change was to keep maximum compatibility with existing playbooks unsing this role as-is. Due to the variable not beeing defined at all in default cases, vm.overcommit_memory will be enabled as usual.

Allthough I agree that it might be way cleaner to add the variable to the defaults, simply set to false. I'd have proposed that change already, but I'm facing some issues with it. As soon as I add it to the defaults, my (otherwise unchanged) playbook stops running through due to a confusing error message, claiming the variable is not a member of the given dict. As the role should have complete access to all its variables, it is unclear to me if this is actually caused by a bug in ansible or if I'm completely misunderstanding variable scopes in this role/roles in general.

tl;dr: Your request is valid, but issues arise when realized that way. Help appreciated.

EDIT: I'd consider reverting this change and applying https://github.com/DavidWittman/ansible-redis/pull/259 instead, which should autodetect unpriviledged containers and set some required settings accordingly.