Hi Jean, My responses are inline.
- Keith On 03/ 1/10 02:27 PM, jeanm wrote: > Keith, > > Thanks for the feedback. Comments are inline. > > On 02/26/10 04:51 PM, Keith Mitchell wrote: >> Hi Jean, >> >> I reviewed version 2.0 of the document. >> >> My first general comment - since there are certain functions that are >> common across all transfer types, I think there may be value in >> defining a superclass and moving some of the functionality there. >> Subclasses can then implement specific attributes and methods they >> need in addition to the common ones. The parent class could also >> weakly restrict values such as "data source", whereas the subclasses >> could enforce that the source is appropriate (e.g., an IPS repo for >> the IPSTransfer class). > > Agreed. I'm working on putting that into the doc. >> >> Secondly, I wonder if the goal of the transfer module is possibly too >> wide in scope? The direction of the Media/IPS/SVR4Transfer classes >> seems to suggest we're simply providing python interfaces for CLI >> commands (or in the case of IPS, an existing Python API). It seems >> like a transfer module should make setting up and executing a >> "transfer" a straightforward task. In short, I suppose what I'm >> asking is - what benefit does a consumer gain from instantiating >> these classes and calling transfer this way, vs. writing their own >> script or using the 'subprocess' Python module to run CLI commands? >> This isn't mutually exclusive with flexibility, though - optional >> arguments to __init__, for example, could be defined for those >> consumers that *really* have specific needs. > > What I'm trying to do is make the default args such that for most > users it would be straightforward. > I think for CPIO that is achieved. They can give a src, dst, and just > go for the ENTIRE case. Just add on a file list for the FILE LIST case. > Probably got that for SVR4 too. > For IPS I've obviously failed and relooking at it. I see. That approach makes good sense to me. I think the "tone" of the document made it seem (to me) like the focus was on the options being made available, which is why I brought it up. >> >> 3.1: Final paragraph on KeyboardInterrupt: This seems a bit specific. >> I think it would be better if the 'transfer()' method implemented a >> 'finally' clause - this could then handle other uncaught exceptions. >> Additionally, there should be some method of killing the transfer >> process outside of simply a KeyboardInterrupt - for example, if >> "transfer()" is asynchronous, a corresponding "cancel()" method. > > Catching other exceptions seems like a very reasonable addition. In > the past, the transfer was not asynchronous, what use are you thinking > of that this would be desirable for? The particular use case I'm considering is text installer related. For the text installer, all "install" actions are performed in a separate thread, so that the main thread can handle updating the UI. Part of that implementation was allowing the user to quit out mid-install (after an appropriate warning about leaving the disk in an odd state). We make use of the function osol_install.transfer_mod.tm_abort_transfer to cancel a transfer in progress; I suppose what I'm getting at is I think that "abort_transfer" function should be carried forward. > >> >> 3.4.1: General: Default values for attributes (such as _file_list and >> _dir_list) should be None (assuming a Python implementation), >> particularly if they're required. >> > None it is. They are not required. > >> 3.4.1.3.1: Nit: The keyword arguments shouldn't have a leading >> underscore - public interfaces/attributes should start with a letter >> instead (The corresponding attributes, such as self._dir_list, can >> keep they're existing names). > Fixed. >> >> 3.4.1.3.3, others: Nit/additional data point: Define these as python >> 'properties' for pseudo attributes. Most "getter/setter" functions >> are more pythonic when implemented as attributes. Access to these >> functions can then occur via "MediaTransfer.type" instead of >> get_type/set_type (I only bring this up at this stage as it's an >> interface kind of nit) > I agree. This falls under the category of "what I've learned today > about Python". >> >> get_args/set_args (All transfer types) - these seem pretty specific, >> requiring the caller to know a fair amount about the transfer they're >> calling. How much of it can be extrapolated away? For CPIO, for >> example, a set of booleans that indicate the different CLI flags, >> with intelligent defaults, seems more applicable then providing a >> potentially awkward "args" interface. Most consumers could leave the >> flags be; if one needed to toggle a specific arg, they could. > > I actually contemplated that and Dave made the statement of > implementing a fairly low overhead interface with a default value that > doesn't need changing for most customers. I believe the CPIO default > hits the mark. IPS we need to ignore for now due to reworking of the > design. I suppose my thinking is that a "low overhead interface" seems like it would require (a) extra processing for the transfer objects to parse out the input to set_args, (b) extra processing for consumers to understand what they get back from get_args and (c) more long term maintenance due to subtle changes in input/output. I think the addition of the "sample consumer code" section will help me see the benefits here though. >> >> 3.4.2.2.1: It would be handy if the default could be dynamic; for >> 'release' installs, default to /release; for 'dev' installs, default >> to '/dev'. This would resolve several RE type issues as well, I imagine. > > I'm wondering if this is information that could be input to the > application that would then be modified via the Transfer Module > checkpoint? Something like that sounds workable. Making this toggle easy to activate on some level (be it here, or passed in by the app), would be very helpful in solving those issues. >> >> 3.4.2.2.1/2: preferred/alternate publisher seems to be an old >> concept; I believe it's now more of a prioritized list, and much of >> the information (about mirrors and such) can be gathered by querying >> the publisher URI. > Yes but they still seem to list the concept as preferred and that just > gets put first in the search order. But I will review this along with > the rest of the IPS stuff. >> >> 3.4.2.2.4: I echo Dave's and Shawn's sentiments here on whether we >> need to provide a full wrapper around the pkg API. It seems more like >> we should be condensing and using the API, not mapping it directly >> across. For example, I would imagine a "transfer()" call for >> IPSTransfer would image-create, set publisher(s) appropriately, pkg >> install, and then purge history. This events could be stored in a >> list or some such thing of the class, a set of actions that the >> IPSTransfer would perform. The list of actions would have an >> intelligent default, but the caller could also modify the default, or >> pass in its own set of pkg API calls for execution. > > Again, reviewing IPS. Will definitely take this idea into > consideration. Thanks! >> >> 4: It'd be helpful (for me) to see some simple pseudo-code or >> examples of what the caller of the transfer module will and will not >> have to do. e.g., an example of what AI might do to run the transfer >> - would it be something like: >> >> ips_transfer = IPSTransfer(altroot, publisher, ...) >> ips_transfer.verify() # Might verify that there's enough space to >> perform the transfer, >> # and that all the pkgs exist >> ips_transfer.transfer() > > Karen asked for that too. I've never seen it in a design doc but since > there are now two of you I'll add a section onto each of the transfer > types with example code. Thanks. As mentioned above, I think this sort of section would be very useful for understanding this (and other module/architecture design docs) - even moreso than end-user use cases. I don't see the customer as the 'user' of this modules as much as I see the applications as the user, so having the use cases from the app point of view makes more sense to me. Thanks again! - Keith > > Jean >
