Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix broken unit tests #368

Closed
wants to merge 1 commit into from
Closed

fix broken unit tests #368

wants to merge 1 commit into from

Conversation

bastelfreak
Copy link
Member

chronological order:

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

@bastelfreak bastelfreak self-assigned this Oct 8, 2020
@bastelfreak
Copy link
Member Author

merge this, rebase and merge #367 afterwards

@bastelfreak
Copy link
Member Author

okay I'm not sure why the tests fail on Ubuntu 16 :(

ekohl
ekohl reviewed Oct 9, 2020
View reviewed changes
templates/redis-sentinel.conf.erb Outdated
@@ -5,7 +5,9 @@ port <%= @sentinel_port %>
dir <%= @working_dir %>
daemonize <%= @daemonize ? 'yes' : 'no' %>
pidfile <%= @pid_file %>
<% if @facts['os']['release']['major'] != '16.04' -%>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not what I had in mind. This is much more like it:

puppet-redis/manifests/instance.pp

Line 402 in d443861

$supports_protected_mode = !$redis_version_real or versioncmp($redis_version_real, '3.2.0') >= 0

@bastelfreak bastelfreak force-pushed the tests branch 4 times, most recently from 9005b1b to 22763bd Compare October 10, 2020 12:46
ekohl
ekohl reviewed Oct 10, 2020
View reviewed changes
manifests/sentinel.pp Outdated Show resolved Hide resolved
@alexjfisher
Copy link
Member

I'm going to knock up an alternative PR

@bastelfreak bastelfreak force-pushed the tests branch 5 times, most recently from 1cfcb00 to 094f04c Compare October 10, 2020 21:12
@bastelfreak
Add procted-mode only to services that support it
779eb44 
chronological order:
* voxpupuli#272 was opened
* voxpupuli#365 was opened
* voxpupuli#365 was merged
* voxpupuli#272 was merged without a rebase
* tests are broken
bastelfreak
bastelfreak commented Oct 10, 2020
View reviewed changes
manifests/sentinel.pp
) inherits redis::params {
require 'redis'

if $package_ensure =~ /^([0-9]+:)?[0-9]+\.[0-9]/ {
if ':' in $package_ensure {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to duplicate the whole block because $package_ensure for sentinel can be something differen than in init.pp/instance.pp

alexjfisher
alexjfisher reviewed Oct 11, 2020
View reviewed changes
manifests/sentinel.pp
@@ -139,9 +139,23 @@
Stdlib::Absolutepath $working_dir = $redis::params::sentinel_working_dir,
Optional[Stdlib::Absolutepath] $notification_script = undef,
Optional[Stdlib::Absolutepath] $client_reconfig_script = undef,
String[5] $minimum_version = $redis::params::minimum_version,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like in instance.pp, I think this is considered private and should just be in the class body instead.

@basti-nis
Copy link
Contributor

basti-nis commented Nov 10, 2020 •
edited
Loading

I've get the tests to work with following workaround in the template:

<% if @protected_mode -%>
protected-mode <%= @protected_mode ? 'yes' : 'no' %>
<% end -%>

i know, it's doubled if statement but i want to have it for later use, when the final solution exists.

the default value for protected mode is "no" if not set, so it's also fine for newer versions starting from 3.2.0.

let's give the option to the user, he should know how to and when to use this.

My workaround is implemented in #356 (sorry for the massive commits :( )

Edit:
ok, i've be wrong... protected mode ist enabled by default

@basti-nis
Copy link
Contributor

basti-nis commented Nov 10, 2020 •
edited
Loading

I think about it a little bit.

When protected mode is enabled by default, then you can write the config only if we want to disable it.
Then you can also check for OS Version or Redis version also.
What do you guys think about that?

Or like @alexjfisher mentioned:
Maybe this could work:
Line 138 - 149
https://github.com/basti-nis/puppet-redis/blob/f417d65e198245139b12e87a6ee7e334762be0a5/manifests/sentinel.pp

Line 8 - 10
https://github.com/basti-nis/puppet-redis/blob/f417d65e198245139b12e87a6ee7e334762be0a5/templates/redis-sentinel.conf.erb

@basti-nis basti-nis mentioned this pull request Nov 10, 2020
Closed
@ekohl
Copy link
Member

ekohl commented Nov 12, 2020

In #374 I took an alternative approach.

@ekohl ekohl closed this Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl left review comments

@alexjfisher alexjfisher alexjfisher left review comments

Assignees

@bastelfreak bastelfreak

Labels
skip-changelog tests-fail
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bastelfreak @alexjfisher @basti-nis @ekohl