search

saltstack-formulas / redis-formula

Redis state
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
40 stars 186 forks source link

Allow redis_settings.snapshots to be empty #62

Closed amontalban closed 7 years ago

myoung34 commented 7 years ago

Have you tested this and it's working as expected? @blbradley is there a better way than repeating the if != '' ?

blbradley commented 7 years ago

Yes, if redis_settings.snapshots were an empty iterable. Either [] or {}. Should be pretty easy to set a default.

amontalban commented 7 years ago

@myoung34 yes I tested with Redis 3.0 (We don't need to do snapshots as we use in-memory cache and don't care about it being persistent).

Let me know if you have any improvement for this, thanks!

myoung34 commented 7 years ago

@amontalban can you set the default in map.jinja to be : {}? that should let you remove the duplicated if conditionals (i'm pushing for DRY here)

blbradley commented 7 years ago

If that default is set, I'm pretty sure the added conditional logic in this PR is not required.

amontalban commented 7 years ago

@myoung34 I have tried the formula setting in pillar snapshots: {} and it worked.

So I was thinking what is the best way to define it empty snapshots: {} or snapshots:. If it is former then this PR is not needed.

Regarding changing default values I vote against because the current defaults are what Redis recommends.

Thanks and sorry for my late reply!

myoung34 commented 7 years ago

The former makes most sense at a glance, so thats what I would prefer. The latter reads like invalid yaml (although it isnt), i prefer to see what the value is, even if its just {}

amontalban commented 7 years ago

@myoung34 cool thanks for the quick reply. I'm closing this up sorry for the trouble.

myoung34 commented 7 years ago

Asking questions is never trouble!