Trimmed down to just the items you had further questions about: On 03/19/10 03:12 PM, jeanm wrote: > Dave, > > Thanks for the review. Comments/questions inline. > > On 03/18/10 03:45 PM, Dave Miner wrote: ... >> What about a common interface for obtaining size requirements for the >> bits to be installed? > > My thought was that the get_progress_estimate method was the place where > this functionality was required. > Since this is TBD I didn't want to go deep and have it all be wrong > although I will if you'd like. > Or are you thinking a more general interface than that? i.e. we would > need the functionality in other places? >
I think it's a different thing, possibly needed at a different time. What we've talked about in the progress estimation was returning some type of standardized units, but that doesn't necessarily correlate to a required size, which may be needed in order to validate a disk layout in a UI (even a batch UI such as AI). >> How would the application obtain the lists of files that are >> referenced here? Would it make sense for the class to be able to do >> this itself by defining an interface that the media might provide? > > I wasn't clear. I meant a file containing this list. My thought was that > the application would keep a file that listed the files to be > transferred within it. That way the list is associated with the > application and easily modified by the developer. > I'd think it's more a property of the media, though. >> >> I'm not clear on whether, for something that looks like the live CD, >> there's a single cpio transfer invocation, or multiple. The file list >> noted here seems to indicate there might be multiple, but the use case >> in 3.6 seems to indicate a single one. > > I think this is an area where showing an xml tree would really help. The > application could make 1 call to > the checkpoint cpio transfer module. With the cpio transfer module, > there may or may not be multiple calls > to cpio. Something like this: > > <transfer> > <file_list_file = "/tmp/file.txt"/> > </transfer> > <transfer> > <file_list_file = "/tmp/file2.txt"/> > <cpio_args = "-pdm"/> > </transf- > In this case, the application would make one call to cpio.execute() > which would read the xml data and perform 2 actual > cpio calls, one to tranfer the files in /tmp/file.txt with the default > cpio_args, the other to transfer the files in > /tmp/file2.txt with specifc cpio_args. > > It is possible to have this: > > <transfer> > <file_list_file = "/tmp/file.txt"/> > <file_list_file = "/tmp/file2.txt"/> > </transfer> > > Again, I think it would be simpler to have the cpio.execute() call > perform 2 cpio calls for this case instead of merging > the files. Not sure if this falls within leaving it up to the > implementer or should be called out. > I agree that delving into the data design here would be helpful, as the difference between the above two is important to the application. I'm still a little stuck on the issue of whether the application should have to tell this module what file sets to transfer, though. I'm thinking it would be better if the class defined an interface that the media must adhere to so that the class can do this itself. That's in essence how the existing transfer works and it seems OK. >> >> WRT to the do_clobber functionality here, would it be a good idea to >> define an interface here for that specific implementation to be >> associated with the media instead of embedded in the class? > > Yeah I think it would. To make sure we're communicating, something like: > > <transfer> > <clobber> True</clobber> > > ...... > > Is that in line with what you're thinking? > I was thinking something a bit deeper, in that the media would provide some sort of "reversal" function that does the clobbering, rather than having the class implement deep assumptions about the transformations that were applied in building the media. Would seem to be more general. >> >> Would it be a good idea for the dryrun to allow for copying to >> /dev/null rather than not executing at all? > Interesting thought and I could see how it would be useful to have data > somewhere for the checkpoints downstream. > So if I'm given a destination of /rpool/dc then instead of installing > the packages there, I would > install to /dev/null. Then do the next checkpoints just "know" to look > in /dev/null? Do I modify the destination to be > /dev/null? I think there's a bit of definition needed for what dryrun should do. It's a validation, in essence, and I guess it's a matter of whether there's just a parameter-level validation (in other words, everything "looks" OK to the class) or should it go a little deeper and actually try a dummy transfer that doesn't alter anything (which is why I mentioned /dev/null). Dave
