On 04/ 7/11 11:50 AM, Shawn Walker wrote:
On 04/ 6/11 12:03 AM, Brock Pytlik wrote:
On 04/ 4/11 02:25 PM, Shawn Walker wrote:
On 03/30/11 09:36 PM, Brock Pytlik wrote:
Here's the system repository work that Tim and I have been working on
for quite a while now.
http://cr.opensolaris.org/~bpytlik/ips-sysrepo-v1/
Thanks for the fast review Shawn. Comments are inline.
general comments: has onu been tested with these changes? There's
likely going to need to be some sort of coordination with whoever
maintains onu and at the very least a flag day when these changes go
into a build.
I haven't tested with onu. I'm happy to do it but can you explain what
concerns you have so I make sure I test the right things?
onu has some special logic for upgrading zones that may break.
There may have also been a subtle change in behaviour here that onu
was depending on, so testing with it doesn't hurt.
Sure, I'm happy to do a smoke test.
[snip]
src/client.py:
line 205: needs updating likely given my comments on 4118-4121 below
lines 226-242: needs updating for --search changes
I've added --search-first, I think that's all the needs changing for
set-publisher
It seems like -P should be removed from the man page if we've added
--search-first. I'd ask Danek.
Ok, I'll check with him.
...
[snip]
[snip]
src/modules/client/image.py:
line 659: what necessitated this change? If the ordering is
important, a comment needs to be added.
It's there because otherwise catalog retrieval for the system publishers
fails.
I'll add a comment but I'll note that if we need to comment every
ordering constraint for every line of code, we're going to have a rather
large number of comments.
I don't see how that's possible. No catalog retrieval should be
happening at this point yet. In particular, the format of the image
hasn't even yet been established so it's not really safe to be doing so.
I think this issue needs a bit more exploration, or I need to better
understand the challenge you encountered here.
Ok, here's the exception:
======================================================================
ERROR: cli.t_pkg_sysrepo.py TestSysrepo.test_01_basics
----------------------------------------------------------------------
Traceback (most recent call last):
File "./pkg5unittest.py", line 500, in run
testMethod()
File "./api/../cli/t_pkg_sysrepo.py", line 383, in test_01_basics
self.__prep_configuration("all-access")
File "./api/../cli/t_pkg_sysrepo.py", line 266, in __prep_configuration
self.image_create()
File "./pkg5unittest.py", line 1458, in image_create
ssl_cert=ssl_cert, ssl_key=ssl_key, props=props)
File
"/export/home/bpytlik/IPS/bug-17535/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/client/api.py",
line 3866, in image_create
user_provided_dir=user_provided_dir, props=props)
File
"/export/home/bpytlik/IPS/bug-17535/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/client/image.py",
line 241, in __init__
progtrack=progtrack, purge=True)
File
"/export/home/bpytlik/IPS/bug-17535/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/client/image.py",
line 662, in __set_dirs
self.__load_config()
File
"/export/home/bpytlik/IPS/bug-17535/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/client/image.py",
line 547, in __load_config
self.get_catalog(self.IMG_CATALOG_INSTALLED).\
File
"/export/home/bpytlik/IPS/bug-17535/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/client/image.py",
line 2480, in get_catalog
cat = self.__get_catalog(name)
File
"/export/home/bpytlik/IPS/bug-17535/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/client/image.py",
line 2514, in __get_catalog
croot = os.path.join(self._statedir, name)
AttributeError: 'Image' object has no attribute '_statedir'
So I guess it's not "catalog retrieval", but instead "catalog loading",
which means statedir must be set.
src/modules/client/imageconfig.py:
[snip]
line 817: maybe add a comment explaining why this is ok or should
be done
Would you prefer I raise an exception if we can't get information from
the smf service? Trying on the default seemed better to me but I'm with
with just erroring out.
My concern was that we might never work right in this particular case
and wouldn't know it. I leave what to do up to you, although if you
leave it as is, I'd appreciate a comment at the least.
I'll add a comment for now. I know Ed had a concern about this to,
though I'm not certain it was the same one. I don't have a strong
opinion for what the right thing to do here was.
...
[snip]
...
[snip]
[snip]
src/tests/pkg5unittest.py:
line 1473: it was intentional that you could specify a repourl and no
prefix; that's a valid input for image-create; why this change?
Really? It looks to me like before, if test was None, you'd get a
command line of:
'pkg image-create -F -p None=<repourl> ...
If it prefix could be None, I don't think that was actually what we
wanted. Perhaps I've misunderstood what you meant though, can you
clarify?
Your change says 'not prefix' which is very different from 'prefix is
None'. There were (or are) some unit tests that set prefix="" here.
I'm surprised they didn't break with this change, but I'd prefer this
say 'prefix is None'.
Sure, that I can do.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
