enable diskless replication configuration #340
Conversation
|
@brianbianco merge pls. |
|
@shortdudey123 Could you review this? It looks good to me. Has a guard for the redis 2.8+ only features in the config template and adds the options for them. |
templates/default/redis.conf.erb
Outdated
| @@ -250,6 +250,69 @@ dir <%=@datadir%> | |||
| # but to INFO and SLAVEOF. | |||
| # | |||
| slave-serve-stale-data <%=@slaveservestaledata%> | |||
| <% if @version[:major].to_i == 2 && @version[:minor].to_i >= 6 %> | |||
There was a problem hiding this comment.
This conditional logic doesn't seem match the comment
# Since Redis 2.6 by default slaves are read-only
Edit: Actually the conditional does match, but the code block would indicate this should only be added to the config file for Redis < 2.6
There was a problem hiding this comment.
Since this is 2.6+, the if statement should be adjusted as such
templates/default/redis.conf.erb
Outdated
| # administrative / dangerous commands. | ||
| slave-read-only <%=@slavereadonly%> | ||
| <% end %> | ||
| <% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or @version[:major].to_i > 2 =%> |
There was a problem hiding this comment.
This won't match a version string of 2.9.1. I'm pretty sure they're done with normal releases in the 2.x series, but possible security releases may bump minor versions in the future.
templates/default/redis.conf.erb
Outdated
| # administrative / dangerous commands. | ||
| slave-read-only <%=@slavereadonly%> | ||
| <% end %> | ||
| <% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or @version[:major].to_i > 2 =%> |
mazeika
force-pushed
the
add_diskless_replication
branch
from
b1015e3
to
06a7762
Compare
|
@shortdudey123 fixed both version checks. |
mazeika
force-pushed
the
add_diskless_replication
branch
from
06a7762
to
24239ce
Compare
templates/default/redis.conf.erb
Outdated
| @@ -250,6 +250,69 @@ dir <%=@datadir%> | |||
| # but to INFO and SLAVEOF. | |||
| # | |||
| slave-serve-stale-data <%=@slaveservestaledata%> | |||
| <% if @version[:major].to_i == 2 && @version[:minor].to_i >= 6 || @version[:major].to_i == 3 %> | |||
There was a problem hiding this comment.
I'd change it to look like this so it works against Redis 4+
<% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 6) || @version[:major].to_i >= 3 -%>
<% ######## Redis 2.6 and higher ######## -%>
templates/default/redis.conf.erb
Outdated
| # administrative / dangerous commands. | ||
| slave-read-only <%=@slavereadonly%> | ||
| <% end %> | ||
| <% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or (@version[:major].to_i > 2 && @version[:minor].to_i >= 9 ) =%> |
There was a problem hiding this comment.
This file didn't pass a syntax test (erb -x -T '-' redis.conf.erb | ruby -c) because of the =%> close tag. ERB / ruby syntax testing would probably be helpful, though I'm not yet familiar w/ the current testing methods in this cookbook to figure out how to add it.
I'd change this line to look like
<% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or (@version[:major].to_i == 2 && @version[:minor].to_i >= 9) or @version[:major].to_i >= 3 -%>
<% ######## Redis 2.8.19 and higher ######## -%>
24239ce
to
4d2357d
Compare
|
@devsibwarra thanks for suggestions, fixed. |
templates/default/redis.conf.erb
Outdated
| <% end %> | ||
|
|
||
| <% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or (@version[:major].to_i == 2 && @version[:minor].to_i >= 9) or @version[:major].to_i >= 3 -%> | ||
| <% ######## Redis 2.8.19 and higher ######## -%> |
There was a problem hiding this comment.
Just noticed your comment under the README says the repldisklesssync option Requires redis 2.8.18+. Update the tiny version operator here to include that version and tweak the comment accordingly.
<% if (@version[:major].to_i == 2 && @version[:minor].to_i == 8 && @version[:tiny].to_i >= 18) or (@version[:major].to_i == 2 && @version[:minor].to_i >= 9) or @version[:major].to_i >= 3 -%>
<% ######## Redis 2.8.18 and higher ######## -%>
Note: I also tweaked the minor version from >= 8 to == 8 to properly match versions. It should be fine in the old method, but having it more precise should avoid confusion
mazeika
force-pushed
the
add_diskless_replication
branch
from
4d2357d
to
7b7d8fc
Compare
|
@devsibwarra done. |
|
@shortdudey123 i think we need bump cookbook version too? |
|
A new release will be done once i can churn through more of https://github.com/brianbianco/redisio/milestone/12 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Rebased #235