On Wed, Jul 14, 2010 at 03:59:32PM -0700, Shawn Walker wrote: > On 07/14/10 03:45 PM, [email protected] wrote: > ... > >On Tue, Jul 13, 2010 at 05:22:42PM -0700, Shawn Walker wrote: > >>On 07/13/10 11:53 AM, [email protected] wrote: > >>>The webrev is here: > >>> > >>> http://cr.opensolaris.org/~johansen/webrev-xpub-1/ > >> > ... > >>====================================================================== > >>src/modules/client/transport/repo.py: > >>====================================================================== > >> line 598: missing spaces after '{' and before '}'. Also, it seems > >> like you could trivially add support for add to catalog here, by > >> (adding) and checking for X-IPkg-Add-To-Catalog. > > > >I added the spaces, but I'm confused about the rest of this comment. > >I re-implemented the transport parts here based upon what was in > >FileTransaction and HTTPTransaction. The HTTPTransaction class didn't > >support the add to catalog method. Are you saying that it should? Do I > >need to make depot changes to enable this? > > Right, the publication API didn't yet support add to catalog > (--no-catalog in pkgsend terms) but it's trivial to add this simply > by checking for the header in the transport system and in close_0 in > server/depot.py. > > I know this is an RFE technically, but I felt since with very little > code we could add feature parity between file and http transport > access it was worth doing. > > I'm not insisting you have to do this in this changeset, it could > certainly be deferred, but it seemed right to do this now while > reving the transport usage. > > Note that for the same reasons we didn't rev /close/ (that is 0 -> > 1) for X-IPkg-Refresh-Index, I don't think we need to for > X-IPkg-Add-To-Catalog.
Ok, this makes sense. All I need to do is pass the header from client to server, and then on the depot if I see the header call repo.close(add_to_catalog=True). Correct? > >>====================================================================== > >>src/modules/client/transport/transport.py: > >>====================================================================== > ... > >> line 2051: so why cycle through origins for publication operations? > >> it seems like we should never have more than one origin for > >> publication. Same for other publication opeartions on 2096, etc. > > > >The __gen_origins code is also responsible for retrying at repositories > >where there are failures. This is used so that we retry a destination > >more than once. Multiple destinations probably isn't a good idea. > > Ah, ok. A comment stating that the iteration was to ensure we retry > for failure as opposed to retrying different origins would be > helpful. Plus, there should probably be an assertion somewhere that > there is only one origin for publication operations. I can make this change, but what I'll probably have to do is call a separate routine that checks the length of the origins list for publication operations that have supplied a Publisher object as an argument to the transport. If that sounds too convoluted, let me know. > >>====================================================================== > >>src/util/distro-import/importer.py: > >>====================================================================== > ... > >> line 1406: replace 'lib/pkg.depotd' with 'bin/pkgrepo'; also update > >> docstring on line 1404 > >> > >> line 1407: replace with: > >> argstr = "%s -s %s refresh" % (cmdname, path_to_repo) > > > >These two are non-trivial changes to validate. It took me a full day > >just to get the importer working to the point where I could run a test. > >Since I've already tested this with pkg.depotd, I'd prefer not to change > >this since I would have to re-test it. That takes hours to get set up. > > I suppose since the importer is going away any day now that's ok, > but we should eventually change this. If you want to file a bug and > move on that's ok with me too. What's going to replace the importer? I wasn't aware of such plans. I'll see if I have time to do this. Maybe the import will go faster now that I've done it a couple of times before. > >>====================================================================== > >>src/util/publish/merge.py: > >>====================================================================== > >> lines 182-231: The code in these seems common to our publication > >> utilities. Any way to centralise these somewhere? (misc? a new > >> module? perhaps pkg.util?) I realise the globals make this > >> difficult. > > > >There have been some subtle variations in publisher setup between merge, > >importer, and pull. I thought about commonizing this code, but I was > >concerned that it would lead to a weirder method shared amongst them > >all. > > Ok, if you can find a way to do it without burning much time, I'm > happy. If not, move on... Since you and Danek both mentioned this I should probably try to take care of it. There are essentially two variations of setup_publishsers, and I can re-write them into one common rotuine. The create_transport routine should probably just configure an empty GenericTransportConfig and have the caller customize its attributes. I'm not sure if that's any better than what we have today. For setup_publishers, the caller would need to pass the transport and transport config, as well as another argument. If you're ok with a routine that has 5 arugments, then I'll go ahead and make it common. I'm a bit uncertain about the best place to put this, somewhere common to all clients would make sense. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
