puppet-redis icon indicating copy to clipboard operation
puppet-redis copied to clipboard

Make it possible to install sentinel independently

Open tomsajan opened this issue 4 years ago • 13 comments

Pull Request (PR) description

This PR introduces the possibility to install redis::sentinel standalone, without redis::server. It comes in handy when you need your sentinels to be for example on a different node than your redis server.

The change itself is a pretty straightforward, I am just adding a require_redis parameter, that makes the requirement of redis class conditional. The default value is true, which includes the redis and it is therefore backward compatible with the current setup.

The only downside of this approach I see currently is that in case someone needs a standalone redis-sentinel from a managed repository, the repository must be added manually as the redis::preinstall is no longer included (as was previously with the redis class)

This Pull Request (PR) fixes the following issues

  • installation of standalone sentinel (no open issue for this one, so far)

Thank you for any input :)

tomsajan avatar Feb 17 '21 11:02 tomsajan

Regarding the puppet5/ubuntu 20.04 tests, the errors don't look like they were caused by my changes...

tomsajan avatar Feb 17 '21 16:02 tomsajan

Dear @tomsajan, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

vox-pupuli-tasks[bot] avatar Jun 21 '22 19:06 vox-pupuli-tasks[bot]

Hi,

Can i help to push this PR? I have this need at work.

thanks

gizmo15 avatar Mar 09 '23 10:03 gizmo15

@gizmo15 it's probably best to submit your own PR.

ekohl avatar Mar 09 '23 11:03 ekohl

Thanks for taking interest in this. To be honest, this slipped from my mind. I'll try to make it work. What is required from this to be accepted? I suppose

  • rebase to current upstream
  • drop leading :: and add tests as suggested by @ghoneycutt ? I may have need some helm with the tests, as I didn't do it yet in puppet, but I will take a look a try my best

tomsajan avatar Mar 09 '23 11:03 tomsajan

Hi, so I merged it to the current master. Could someone advise me please on how to pass the CI tests? Static tests reports outdated reference.md, but I adjusted it as good as I was able and it still complains. :(

Not sure though on how to tackle tests to verify the presence or absence of the redis class...

Also, this MR still has the labe merge-conflicts, although there are non now.

Thanks and have a great day :)

tomsajan avatar Mar 09 '23 21:03 tomsajan

I would recommend to rebase it on master. We don't like it when a PR merges in master. Though I think the bot may not be able to remove the label.

ekohl avatar Mar 10 '23 11:03 ekohl

Hello, any news about this PR ? Thanks.

flepoutre avatar Aug 05 '24 07:08 flepoutre

It doesn't appear it was ever updated as requested during the review, so it can't be merged as-is. The original author would have to update it, or someone else could create a new pull request based on it.

yakatz avatar Aug 05 '24 12:08 yakatz

Let me give it a try for a day or two :)

tomsajan avatar Aug 05 '24 12:08 tomsajan

So I squashed my changes and rebased it all to current master. During the rebase I renamed the option to contain_redis because now contain is used instead of require. There are still some tests that fail.

I am not sure I will be able to test this code as I don't use puppet anymore in my infrastructure, but I will try to come up with something tomorrow.

tomsajan avatar Aug 05 '24 21:08 tomsajan

@tomsajan Hello, thanks for your feedback. I don't really understand the Puppet CI error 🤷‍♂️ Do you have any idea please ? Thanks.

flepoutre avatar Sep 05 '24 09:09 flepoutre

As I need this feature, I'd like to pick up this topic again. As far as I can see, tests are still missing and I'd suggest a different parameter name, e.g. standalone.

saz avatar Feb 28 '25 14:02 saz