Real fix for issue #37

[mattpascoe]

My last suggestion for quotes was incorrect, the real problem was tabs within the result of sysctl -n. I have made those adjustments and done actual testing on my instance. Here is a pull request to update the prior problem created in v1.0.5.

[thias]

OK, it makes sense. But unfortunately there's more to it than just this... Your code fixes your case, but probably breaks others : If someone specifies an original value with the tabs, it will no longer match.

IIRC, I already used to have some code in there which replaced all tabs with spaces and also squeezed spaces (think about those who copy/paste the tabulated values and end up with multiple spaces instead), but it was a bit ugly and hard to maintain. It looks like something similar might need to come back for this enforcing to work...

Thoughts? Better code suggestions?

[mattpascoe]

Agreed, this is a fun thing to deal with. I've not looked through the entirety of your code to see how values are filtered so I can't speak to the whole situation. I would however suggest that once a value is placed into the puppet manifest, there should then be a standard cleanup filter that is apply to it, remove funky characters as needed, remove tabs etc etc. This way, within the code it is always treated and expected to conform to that standard and you dont need to care what the end user or sysctl -n outputs.

What I'm observing on my end is that my manifest uses a single space, the code writes a sysctl.d file with only spaces in it. This is good and consistent. The problem is that for some reason the sysctl -n output is placing tabs in the output. So this is where we have to normalize it back to a standard place. (my OS is OEL, I assume the tab thing happens on others?)

I guess bottom line is the module needs to enforce a consistency but that could be a bit tedious to manage. Don't think I have any better thoughts than that.

Also, be aware that the 1.0.5 code is currently silently throwing an error in debug output:
Debug: /Stage[main]/Profiles::Kernel/Sysctl[kernel.sem]/Exec[enforce-sysctl-value-kernel.sem]/unless: /usr/bin/test: extra argument32000'`. Might want to reverse that patch if we can't come up with a better fix.

[thias]

I've reverted the incorrect change now. I also thought about disabling enforcing by default for a moment, but in the end I think that the proper fix is to have people set a value which is exactly what it should be : If you "fix" yours to have tabs where the original values reported by sysctl have them, everything should just work...

I do agree that it would be much better to keep things working, which is sort of what I did with another unrelated fix where I keep letting people pass numericals for value when in reality it really should always be a string... The difference is that the numerical/string problem had a very simple solution.

Again, code ideas are welcome. Your suggestion of having a single place where tabs/spaces get mangled consistently is a good one, unfortunately with Puppet's DSL and execs currently in my module, this isn't really possible.

[mattpascoe]

Yep, I see where you are coming from. So I just tried to go make my manifest have tabs in the data I'm trying to set. It does work, however it also has the added annoyance that my vi is configured to insert spaces instead of tabs. :) I was able to define it like:

value => "1234\t5678"

Ahh the joys of data..

At this point I'd say we kill this pull request and if at some point in the future a new idea to resolve this comes along then great. for now I can move forward.

... I wonder if on values with multiple entries like this, could we enforce a tab replacement on any single space? That is the way it should be to meet sysctl -n output.. this way its one place to manipulate the data and it becomes more of a 'helper' for the common case of someone typing a space between the elements in value. if its anything other than one space, leave it alone?