On Tue 08 Apr 2008 at 02:06PM, Stephen Hahn wrote: > * [EMAIL PROTECTED] <[EMAIL PROTECTED]> [2008-04-08 20:41]: > > We need rudimentary SSL support in the pkg client for our upcoming > > release. This set of changes adds basic SSL support to the client and fixes > > a bunch of other bugs I created in my last few putbacks. > > > > Webrev is available here: > > > > http://cr.opensolaris.org/~johansen/pkg-sslcli/ > > client.py: > > *. Since we don't have mirror support, let's not have the -m option > yet. > > ssl-headers.conf: > > *. I hadn't finished this piece yet. Probably best to drop it for > now. > > Let me see if I can whip up some pkg(1) diffs for you.
Question 1: in image-create, we specify authorities using -a, and in the form authority=http://path/to/authority In these new commands the syntax seems to be different. Why is that? I sort of expected this to look something like: pkg add-authority -a test1=http://test1 pkg del-authority test1 pkg prefer-authority test1 The confusing thing for me about using "set" in this context is that it sort of implies that there's only one thing we're setting. Instead, we're maintaining a list of things. Question 2: Could we see some sample output? I mostly reviewed client.py and the test cases-- didn't do thorough coverage of the rest. client.py: 866 + print \ 867 + "pkg: set-authority: one and only one authority may be set" 868 + usage() Pass this string as a parameter to usage(), don't forget to wrap it in _(). Note: a lot of other strings you are adding are missing _()... I tried to clean this up in my recent wad. 874 + if not os.path.exists(ssl_key): 875 + print _("pkg: set-authority: SSL key file '%s' does not exist" 876 + ) % ssl_key Use error() instead, it ensures that output goes to stderr and does the pkg: prefixing for you. (also applies to a number of other places) 925 + try: 926 + opts, pargs = getopt.getopt(args, "H") 927 + for opt, arg in opts: 928 + if opt == "-H": 929 + omit_headers = True 930 + except getopt.GetoptError, e: 931 + print "pkg: authorities: illegal option -- %s" % e.opt 932 + usage() IIRC the exception handling is not needed-- if you simply leave the exception uncaught, it'll get caught in main_func(). (ditto for line 861) 940 + if pfx == preferred_authority: 941 + pfx += " (preferred)" 942 + print "%15s %30s" % (pfx, url) 943 + else: 944 + print "%15s %30s" % (pfx, url) Given that " (preferred)" is 12 characters in length, is the first column wide enough? 959 + print "\nAuthority: %s" % pfx 960 + print "Origin URL: %s" % url 961 + print "SSL Key: %s" % ssl_key 962 + print "SSL Cert: %s" % ssl_cert 963 + print "Catalog Last Updated: %s" % dt Consider copying the output style of 'pkg info'? 1013 + ssl_key = os.path.abspath(ssl_key) Why abspath()? (os.path.exists() returns false for broken symlinks). t_commandline.py: Please extend test_pkg_missing_args, and test_pkg_bogus_opts for your changes. I'd also like to see a more thorough vetting of the functionality in the test cases-- there's a lot of error paths you have which aren't being tested: SSL key specified SSL key specified, file doesn't exist Cert specified Cert doesn't exist Origin URL missing for new authority More than one authority given (i.e. the error message on line 864 of client.py) Attempted removal of preferred authority Listing a specific authority Listing a specific authority which does not exist Thanks, -dp -- Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
