parameterize the restrict options #69
Conversation
|
for issue #60 |
|
Looks great! |
|
Thanks :) I'll need to get the given Solaris defaults confirmed. |
|
Since they are all there to fix issues with Solaris, feel free to submit them as one PR if you would like. |
|
Should update readme that restrict_options can also be an array and add the restrict_localhost param. |
| } | ||
| } | ||
| else { | ||
| fail("restrict_options must be an array (prefered) or a string (will be deprecated).") |
There was a problem hiding this comment.
Why will string be deprecated?
There was a problem hiding this comment.
It's a suggestion only
Well, the new handling in the template is using an arrays. In my eyes it is preferable to have input and output variables in the same format. It avoids possible conversation errors and surprises on the user side. If you use a string it result in two lines with -4 and -6 as prefix. Using an array it will output exaxtly your input.
Above string to array conversation is a dirty workaround in my eyes. It's (hopefully) only there to not break things and expectations until the next major version update.
There was a problem hiding this comment.
Totally agree. If we bump the major version, we can introduce other breaking changes. Is there anything else we should change? I was thinking of merging this and your solaris code and making that the last of v3 and then we could move to v4, but I do not want to do that unless there are really compelling reasons to break compatibility.
There was a problem hiding this comment.
On a major change, both restrict options could be merged into one.
There was a problem hiding this comment.
we could rename $disable_monitor to $enable_monitor to have positive wording :)
There was a problem hiding this comment.
technicaly restrict_options and restrict_localhost could get merged into one (would break backward compatibility now)
Phil-Friderici
force-pushed
the
solaris_fix
branch
from
c8e41c6
to
d839437
Compare
Phil-Friderici
force-pushed
the
solaris_fix
branch
from
d839437
to
f7f3682
Compare
|
Small change on Solaris 11 default values was needed, but now we having solid defaults there :) |
| @@ -702,4 +761,63 @@ | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| describe 'with enable_driftfile set to' do | |||
There was a problem hiding this comment.
shouldn't this be part of the platforms hash?
There was a problem hiding this comment.
Yes, I agree, that would be better. I could also reuse $driftfile, if it's empty I disable the complete block. This way we don't need an additional variable. I'll give it a try, let's see if it looks good.
There was a problem hiding this comment.
Changed the way it works. If $driftfile is empty the complete section will not be used anymore.
Because it's not mandatory but desireable to turn of driftfile for Solaris I have not touched the default values anymore. But spec tests are prepared for such a default in future times.
Phil-Friderici
force-pushed
the
solaris_fix
branch
4 times, most recently
from
42d1dcd
to
ef86e9b
Compare
|
Think now it's complete. I'll ask the guy who needed that to test it tomorrow. Let's see if he's happy with that. |
Phil-Friderici
force-pushed
the
solaris_fix
branch
2 times, most recently
from
b7d7f72
to
d1463f0
Compare
|
All thumbs up from the Solaris guy :) |
Phil-Friderici
force-pushed
the
solaris_fix
branch
from
d1463f0
to
929a115
Compare
Phil-Friderici
force-pushed
the
solaris_fix
branch
from
929a115
to
6681abb
Compare
parameterize the restrict options
|
Release in v3.5.2 Thanks! |
|
Nice and smooth one :) Thanks @ghoneycutt |
1st step for better Solaris compatibility. Please have a look if it's going into the right direction.
Asked local Solaris specialist if the defaults valid for Solaris 9 to 11. Will confirm after I got the reply.