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 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 -~----------~----~----~----~------~----~------~--~---
