-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tls settings to the redis::sentinel class and the redis-sentinel.conf template #443
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Please add some unit tests for the new parameters 😄
Thanks @root-expert is there somewhere I can get a pointer on writing unit tests. That's where my puppet knowledge falls down. |
The unit tests should be added in the redis_sentinel_spec file. You can add your new parameters with a random value in this block and check that the random value is rendered correctly in this block. For more in depth knowledge for testing puppet you can check rspec-puppet.com and our CONTRIBUTING.md for instructions on how to run unit tests. (You can always just push and let our CI do the hard job 😄 ) |
Perfect! Will do. Might be on Monday but it looks straightforward. |
I changed tls_replication to use the Enum['yes', 'no'] but that makes it not the same as the identical variable in the redis class. Which way do you prefer? |
I think I am going to put this back to a boolean. It would be very confusing the people to have the same variable as a Boolean for one module and an Enum for the submodule. |
I believe the new options should follow Enum style, but I agree it can be confusing. |
I have been thinking about the default values for the tls_ options in the sentinel class. Does it make sense to default them to the values from the redis class? For most instalations they should be in sync all the time. That would mean that the cert paths, and replcation values default to undef still but pulled from redis::tls_* |
This works in puppet on my servers, however doesn't pass the spec tests because the $redis:: variables are not defined. Is there a way to define a more complete precondition so that we can consume those values? The redis class is required in the sentinel code so there will never be a case where we can have the redis::sentinel without redis already being in the catalog. |
Take a look here |
Thanks! I was confused because it already had that set for the custom params. I hadn't noticed it was failing for the default params case. |
I am still not able to get the spec tests to work. Have I missed something? |
Let's use plain defaults @tparkercbn 😄 |
Yep. That's what I think too. I am going to keep the tls_replication setting a Boolean and tls_auth_clients with the default to 'no' so that the types are consistent between the main and sentinel classes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for the contribution!
Thank you for all your help! :) |
Pull Request (PR) description
Added tls settings to the redis sentinel submodule. If you have tls only enabled for redis not having tls on the sentinels makes them useless.