On 02/ 5/10 09:29 PM, Danek Duvall wrote:
Shawn Walker wrote:
http://cr.opensolaris.org/~swalker/pkg-11522-2-full/
Looks pretty good; these are mostly nits.
client.py:
- line 163: join with line 164?
- Did we really agree on requiring --all instead of just defaulting to
that if we don't get a publisher prefix?
I thought we did for image-update, but the last email I had from Stephen
on this indicated that he wanted users to either specify --all or a
publisher prefix for the set-publisher case. I'm happy to change this
if he's ok with that. My original suggestion to him was that if I found
a single publisher, require --all, if I only found one, then don't.
Since set-publisher -p will only add publishers that don't already
exist, and update existing ones only if they have a matching origin, I'd
vote to not require the --all. The set-publisher command will tell you
about any publishers it adds or updates anyway if there are multiple.
I should also note that pkg.depotd currently doesn't support multiple so
this seems moot anyway...
- line 2482: Won't this happen on many older depots? Or do you get back
something sane from the API when you connect to one?
So there are two types of depots here:
1) those that don't support publisher/0
2) that that support publisher/0, but didn't require certain
configuration fields and thus don't provide complete configuration
In either case, we want -p to fail because auto configuration isn't
possible. The user has to use -g, -o, etc. and provide the prefix.
- line 2523: can this be pulled out and shared between the two clauses?
I don't follow; I only know that we added a publisher as opposed to
updating one because I do the check inside this block.
t_pkg_depotd.py:
- line 576: I thought generally we wanted to have depot URL roots have a
trailing slash.
The client automatically adds them for repository URIs, but for legal
URIs, etc. it doesn't. So if I wanted to be able to generically compare
all URIs in the unit test, I needed to ensure that the raw configuration
data doesn't have them for consistency. This doesn't actually change or
affect the client or server's behaviour at all. It's purely a unit test
thing.
pkg5unittest.py:
- line 1482: won't this die after you just rmtree()d dest?
No, because 1478 calls makedirs() to re-create it.
I'm fairly certain my unit tests would fail if this didn't work right
since they did until I added this :)
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
