So does this mean we just apply Markus's fix and close the ticket, maybe opening a refactor ticket to go with it?
On Aug 14, 2009, at 9:55 AM, Nigel Kersten wrote: > I spent a while last night looking at the existing tests and getting > a broader picture of this section of code, and I concur with all of > Markus' points now. > > I think it's the best approach for the current code given the > lifecycle stage of the 0.25.x release. > > On Fri, Aug 14, 2009 at 8:05 AM, Markus <[email protected]> wrote: > > > > So I am actually planning to make tests for this, I just wanted > some > > > wider consensus that this isn't going to break the REST auth > system, > > > even though the existing tests run fine. > > > I don't think the change you're proposing would break anything, > though. > > But since I failed already once when I made this change, you might > not > > take my advice into account :-) > > Removing the special casing of :opaque strings at the very top of > matchname?() -- so that munge_name is always called -- doesn't break > any > case I have been able to think of. > > Removing the brackets around value in the else clause of parse() (so > that @pattern is a string sometimes instead of always being an array) > breaks a few things, for example the specificity ordering of tests: > > describe "when comparing patterns" do > before :each do > @ip = > Puppet::Network::AuthStore::Declaration.new(:allow,'127.0.0.1') > @host_name = > Puppet::Network::AuthStore::Declaration.new(:allow,'www.hard_knocks.edu > ') > @opaque = > Puppet::Network::AuthStore::Declaration.new(:allow,'hey_dude') > end > it "should consider ip addresses before host names" do > (@ip < @host_name).should be_true > end > it "should consider ip addresses before opaque strings" do > (@ip < @opaque).should be_true > end > it "should consider host_names before opaque strings" do > (@host_name < @opaque).should be_true > end > end > > This works with the present code and works if the "unless @name > == :opaque" is removed from matchname?() but fails if the brackets are > taken off of value in the "@pattern = [value]" near the end of > parse(). > > There are a number of solutions but IMHO whatever we do should be > uniform and simplifying rather than adding special cases. I wouldn't > object to having @pattern _always_ be a string (say, the string > assigned > to self.pattern = ?) but having it change class arbitrarily and then > peppering the code with special case logic to handle it seems like the > road to madness. > > -- Markus > > P.S. If we do decide to take the road to madness (which I still don't > recommend) there's this cool little cantina I know right past the > turnoff to nihilistic pointer envy. > > > > > > > > -- > Nigel Kersten > [email protected] > System Administrator > Google, Inc. > > > -- You will notice that BeOS has taken the best parts from all the major operating systems and made them its own. We've got the power of the Unix command line, the ease of use of the Macintosh interface, and Minesweeper from Windows. -- Tyler Riti --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
