On Fri, Aug 14, 2009 at 10:36 AM, Luke Kanies <[email protected]> wrote:
> > So does this mean we just apply Markus's fix and close the ticket, > maybe opening a refactor ticket to go with it? +1 > > > 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 > > > > > -- Nigel Kersten [email protected] System Administrator Google, Inc. --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
