On Mon 10 Sep 2012 at 03:11PM, Brock Pytlik wrote: > catalog.py: > I'm not thrilled by this change only because it seems like this is a > list that's likely to fall out of date as other changes are made to > the module. Now, maybe we just count on our testing w/ the nasty > depot to catch that, but it would be nice if this was "someplace"
[back to this after a long hiatus: I was busy then sick.] I've altered the code a bit to make this less likely to get screwed up in the future-- now when we assemble the "sane defaults" catalog in the constructor, we do so by copy'ing it from a class variable called DEFAULT_ELEMS. Then we also use DEFAULT_ELEMS to check the sanity of a catalog when we need to. So these things are synced up. If you wanted to extend the catalog code to have a new required element, I think you'd be hard pressed to have it work if you didn't update the DEFAULT_ELEMS. > General question about the solution to 7193416. Given that an empty > manifest can never be a valid published manifest, what happens now > when the client gets an empty manifest? Do we fail with an > appropriate error (presumably about the manifest not being > valid/matching the hash)? Yes-- with these changes, the client will see this as a transport problem and try again. If it happens over and over, you'll see something like: Invalid content: manifest hash failure: fmri: pkg://test1/[email protected],5.11-0:20120912T230436Z expected: c2b095bd67876466d8368f0985e8803005be87f5 computed: da39a3ee5e6b4b0d3255bfef95601890afd80709. (happened 4 times) It would perhaps be nice to decorate in the specific URL which is causing the problem here, but that's another project. > server/depot.py: > I'm a little confused about the need to have two moving parts for > setting the amount of nastiness. I can call need_nasty_* which > changes the max roll, but I can also set the NASTY_LEVEL in the > initializer of the class. But if (for example) I set my nasty level > to 10000, then there'd be no difference between > need_nasty_occasionally and need_nasty_infrequently. Maybe those > should use a multiple of NASTY_LEVEL to set maxroll? But if we did > that... then setting NASTY_LEVEL would really be sort of irrelevant > I think? So maybe that's really my question, why do we need > NASTY_LEVEL as a parameter? NASTY_LEVEL is a parameter that exists only in t_pkg_nasty. My assumption is that you mean "nasty_level"? nasty_level (settable via --nasty) allows a high-level control over the overall amount of nastiness which is happening. This can, I think, be helpful in simulating different sorts of environments. src/depot.py enforces that this must be between 0 and 100. All of this is the same as it was prior to my wad except that I moved the nasty_level stuff into the dcfg configuration framework in order to avoid the awkward interface/inheritance dance wherein all depot subclasses had to know how to set their nastiness. > Also, a total bikeshedding point: I don't find that occasionally, > infrequently, and rarely have some sort of consistent and implicit > ordering among them. Maybe these could be litle, normal, a lot? Or > minimal, average, most/above-average? Or level_1, level_2, level_3? > Note a huge deal, but it'd be nice not to have to look at pydoc/code > to figure out which one I should use to increase (or decrease) the > amount of nastiness. In the words of Homer Simpson: "It was like that when I got here." Anyway, I changed it to need_nasty() (which is parameterized), need_nasty_2(), need_nasty_3(), need_nasty_4(). I'm not sure it's a big improvement but it's something I guess. > line 1762: Why not let it go negative? (Or alternatively, treat the > number like it's an unsigned int and wrap it around to being a huge > positive number) I can let it go negative-- it seems less useful to me since -1 is never a version we have supported-- so it's more like a unit test, but I don't feel strongly. > line 1890: Why'd we get rid of this test? This part of the functionality is now handled by the before_handler. > 2093: Is this something that we should revisit once 7195051 is > fixed? Would it make sense to either file a separate bug to track > that or include that info as part of 7195051. Well this comment sent me off into a long exploration of this problem. It turns out that, with some additional work, we can enforce that _SIGNATURE must be present, and we can harden up the rest of the catalog.attrs handling sufficiently to eliminate this code entirely. So, that's pretty nice. The fix is a little more complex than the others so I will post a separate webrev for it in order to make it clearer. Thanks also to Shawn for some valuable hints about the finer points of catalog handling with p5ps and other things. > t_catalog: > Just curious. Is there a reason to prefer print >>f, <foo> as > opposed to f.write(<foo>)? I made it f.write(), it looks better. I'll post a new version of the review later today, I hope. -dp -- Daniel Price, Solaris Kernel Engineering _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
