Restrict the regex to only allow 0 or 1. Not 10.

[deanwilson]

Restrict allowed yumrepo values to 0 | 1. Not 10

Issue number #21458

[puppetcla]

CLA Signed by deanwilson on 2010-10-21 21:00:00 -0700

[djmitche]

or abc19938def

+1, and I assume there was an irc convo about not using the boolean support.

[zaphod42]

@djmitche Actually the extent of the conversation was me pointing out that it existed. I don't think we can use it though, since the :parent is already taken up by the IniProperty. We could make IniProperty inherit from Boolean, but that would interfere with the non-boolean properties, I think. Maybe it can be done with a couple different subclasses of IniProperty for each type of property that is being dealt with?

[deanwilson]

That sounds like a lot of quite invasive changes to the type - which is pending a split to type and provider anyway. This was the smallest code change I could make to ensure the regex did what was intended.

[adrienthebo]

This change has to touch a bunch of different locations but it's the same regex. Perhaps this regex could be extracted to a constant and then that constant could be referenced in each newvalue?

[adrienthebo]

Thank you for this contribution! Would it be possible to get some unit test coverage for these changes to help prevent regressions?

[adrienthebo]

I noticed this hasn't seen any activity in about a week. Is there a chance you'll be able to work on this in the coming week? No pressure, just checking in.

[deanwilson]

Normally I'd be ok with putting at least some basic unit tests in with a pull request but the yumrepo type is a horrible, horrible place full of evil and neglect and I'd rather maintain my own fork than touch its unified type and provider approach.

I'll be happy to add some tests when it gets split in to seperate types and providers but in the mean time please don't make me go back in there... "# these tests were ported from the old spec/unit/type/yumrepo_spec.rb, pretty much verbatim" serves as adequate warning to those of us not of the 'Labs.

[djmitche]

"please don't make me go back in there" -- Best pull req comment I've seen in a while!

I've gotten myself in trouble before accepting changes without tests. This is one of those areas where tech-debt accumulates quickly. IMHO, and I'm not an authority, the best approach is to leave the pull request as-is until someone (@deanwilson or someone else) can refactor the code to be more testable.

[djmitche]

"please don't make me go back in there" -- Best pull req comment I've seen in a while!

I've gotten myself in trouble before accepting changes without tests. This is one of those areas where tech-debt accumulates quickly. IMHO, and I'm not an authority, the best approach is to leave the pull request as-is until someone (@deanwilson or someone else) can refactor the code to be more testable.

[adrienthebo]

@deanwilson I had not realized how horrible those specs were until I looked myself; sorry about that. They're pretty ghastly.

I see two main options here.

1. Merge it as-is. It's a reasonable change, has a small surface area, and I don't think it could cause regressions. This does sweep the problem under the table though, and the fact that we've done that in the past is why we're here now.
2. Start thinking seriously about a significant overhaul of the type. We can throw out the existing tests, split the code into type and provider, and get a clean start. It'll take more work but it's a more sustainable solution.

If anyone is feeling hardcore I would be totally willing to help them out on the rewrite; I would like to see this fixed and will chip in whatever effort I can. Alternately @deanwilson if you're happy with this as-is, then we can just merge it for now. What do you think?

[deanwilson]

There are at least two partial implementations of a split yumrepo type/provider already attached to tickets in http://projects.puppetlabs.com/issues/ - one by puppetmasterd himself - so I'm a little skepticial of how quickly this'd get done knowing it's been tried multiple times before. Doing this as a proper split provider will probably need the Inifile lib pulled in to core too; which will lead to more discussions. I'm also pretty sure that the AugeasProviders version won't happen until after the splits been done so I don't see a quick win here.

This patch was a side effect of the s3_enabled one; which is the patch I really care about ;) so while I'd say this should be a small enough change that it could go in without the unit test safety net I'm not going to push.

[adrienthebo]

In that case I am happy with merging this in as-is. Right now the build isn't passing acceptance tests so this can't be merged right now, but as soon as that goes green I'll merge this. Thanks for the contribution!

[adrienthebo]

summary: merged into master in 9557b51; this should be released in 3.3.0. Thanks again for the contribution!