John, we're done as far as I'm concerned. Dave
John Fischer wrote: > Dave, Sundar and Clay, > > Just a reminder... > > I have update the webrev for the installadm list subcommand fixes > for the issues that you brought up. The updated webrev is located > at: > > http://cr.opensolaris.org/~johnfisc/list > > Once I get your approval then I think that I am done. I am most > interested in Clay's issues as the class and criteria lists is some > contention. > > Also I have updated my source tree via Mercurial merging Clay's changes > that went into installadm.c and delete_service.py. > > If I can get closure before you go on break then that would be > great. > > Thanks, > > John > > John Fischer wrote: >> All, >> >> I have updated the webrev for the installadm list subcommand fixes. >> It is located at: >> >> http://cr.opensolaris.org/~johnfisc/list >> >> It is updated with all the feedback so far except for the class >> implementation and list-manifest suggestions by Clay. We decided >> not to do those after a lengthy discussion with Ethan about the >> issue(s). If anyone wants a detailed explanation of why please >> feel free to ask. >> >> Thanks, >> >> John >> >> >> John Fischer wrote: >>> Clay, >>> >>> Thanks for the help. I appreciate you taking the time to type this >>> up. I'll update the webrev tomorrow morning. >>> >>> Thanks, >>> >>> John >>> >>> Clay Baenziger wrote: >>>> Hi John, >>>> As we talked about in IRC and some high level thoughts: >>>> *Please update your code to be in sync with slim_source (based off >>>> the Python 2.6 Python files) >>> >>> Will do. >>> >>>> *Please move findTFTProot() from delete_service to >>>> installadm_common so that it's more obvious there are multiple >>>> consumers. Further it seems, which_arch would be a general >>>> function well provided through installadm_common. >>> >>> Already moved. >>> >>>> *I'm rather concerned that you're rewriting list-manifest opposed >>>> to simply using what's already written. The only bug I see >>>> affecting list-manifest is pretty trivial: >>>> 4298 installadm list -n should give different output if there is >>>> no custom manifest >>>> Further, I think this bug: >>>> 4646 list: showing added manifest for a non running service. >>>> Would probably better be: >>>> 4646 list: Should provide warning manifests listed are for a >>>> non-running service >>>> (as we want to allow publication/deletion/listing of manifests >>>> when the service is offline since there's no requirement for the >>>> service to be online and this is how one can setup a service to >>>> "go live" without affecting other things) >>> >>> Don't recall why I decided not to mess with list-manifest. I'll >>> see if I can find my notes. >>> >>>> *I think you should take bug 4320 - installadm list -n should use >>>> the -c option as default. And you should actually take ownership >>>> (and mark as FIXINPROGRESS 4298 - installadm list -n should give >>>> different output if there is no custom manifest. >>> >>> Defect 4320 does not match the design specification for list. >>> I'll talk to Ethan about it. The design specification is located >>> at: >>> >>> >>> http://hub.opensolaris.org/bin/download/Project+caiman/auto_install/DesigndocdeltaforAISpring2009.2.pdf >>> >>> >>> >>> Defect 4298 should report an error if installadm list -m -n service_name >>> does not have a custom manifest. This is currently what the list >>> subcommand does. >>> >>>> *Do you have any example output as I can't visualize what this >>>> looks like? (Also, have you run the output past Frank as he'd >>>> given feedback on list-manifest?) >>> >>> OK. See the design specification for more detailed output: >>> >>> Manifest Criteria >>> -------- -------- >>> devpublisher.xml arch = i86pc >>> mac = 01:C2:52:E6:4B:E0 >>> ipv4 = 010.000.002.015 >>> devpublisher3.xml arch = i86pc >>> mac = 01:C2:52:E6:4B:E6 - 01:C2:52:E6:4B:E9 >>> devpublisher4.xml arch = i86pc >>> ipv4 = 192.168.168.151 >>> devpublisher5.xml arch = i86pc >>> ipv4 = 192.168.168.251 >>> mac = 01:C4:52:E6:4B:E6 - 01:C4:52:E6:4B:E9 >>> devpublisher6.xml arch = i86pc >>> ipv4 = 192.168.168.012 >>> mac = 01:C4:51:E6:4B:E6 - 01:C4:51:E6:4B:E9 >>> mem = 2048 MB >>> somethingelse.xml arch = i86pc >>> mac = 01:C2:E2:E6:4B:E9 >>> >>> >>>> Code bits: >>>> list.py: >>>> line 189 find_sparc_clients >>>> line 337 find_x86_clients >>>> I like the layout of these functions, however, couldn't >>>> this data be rolled up into a x86_client and sparc_client >>>> class to allow caching and easier reuse across all AI >>>> components? (This would be very useful for the delete*.py >>>> bits) >>> >>> Hmm... OK. This will take me a little while longer. May not make >>> tomorrow's new webrev. >>> >>>> line 364 get_menu_info >>>> Please use the GrubMenu class in installadm_common, unless >>>> I'm missing what's special here? >>> >>> Already saw that one when you asked about moving findTFTPboot() into >>> installadm_common.py and have a working list using it. >>> >>>> line 311 (and elsewhere) >>>> Instead of concatenating two paths with a '/' please use >>>> os.path.join(). >>> >>> Will do. >>> >>>> line 569 >>>> "no local service\n" or "no local services\n"? Also (in >>>> other places too), please follow PEP8's recommendation to >>>> leave operators on the upper line when a line wrap is >>>> encountered. And lastly I think you should be lined up >>>> with the underscore from above. I think the lines would >>>> be: >>>> estr = _('%s: error: no local services\n') %\ >>>> os.path.basename(sys.argv[0]) >>> >>> Ah, OK. >>> >>>> line 628 >>>> You're doing an assignment here so rdict and second are >>>> the same object: >>>> >>> def c(foo): >>>> ... b=foo >>>> ... del(b[1]) >>>> ... >>>> >>> a >>>> {1: 1, 2: 2, 3: 3} >>>> >>> c(a) >>>> >>> a >>>> {2: 2, 3: 3} >>>> You might want to see: >>>> http://docs.python.org/library/copy.html >>> >>> Ah. I see. OK will do. >>> >>>> line 633: this line assumes that rdict has list objects as its >>>> values. Otherwise, one will get an AttributeError likely. If >>>> you're assuming that each dictionary only contains lists it could be >>>> written for quicker execution, I believe, as: >>>> >>> a={1: ['a'], 2: [2], 3: [3]} >>>> >>> b={1: ['b'], 'c': [4]} >>>> >>> def merge(foo, bar): >>>> ... c={} >>>> ... c.update(foo) >>>> ... c.update(bar) >>>> ... for key in set(foo.keys()) & set(bar.keys()): >>>> ... c[key]=foo[key]+bar[key] >>>> ... return c >>>> >>> merge(a,b) >>>> >>> {1: ['a', 'b'], 2: [2], 3: [3], 'c': [4]} >>> >>> OK >>> >>>> line 809: get_criteria_info: >>>> Please use more descriptive variable names, I can't parse >>>> what's going on here >>> >>> Changed criteria to mancriteria, crit to key, tk to tkey, tv dropped >>> using svalue. Also modified the function as MAX/MIN code was >>> essentially the same. See below: >>> >>> def get_criteria_info(mancriteria): >>> """ >>> Iterates over the criteria which consists of a dictionary with >>> possibly arch, min memory, max memory, min ipv4, max ipv4, >>> min mac, >>> max mac, cpu, platform, min network and max network >>> converting it >>> into a dictionary with on arch, mem, ipv4, mac, cpu, platform, >>> network. Any min/max attributes are stored as a range within >>> the >>> new dictionary. >>> >>> Args >>> criteria = dictionary of the criteria >>> >>> Returns >>> dictionary of combinded min/max and other criteria >>> >>> Raises >>> None >>> """ >>> >>> tdict = {'arch':'', 'mem':'', 'ipv4':'', 'mac':'', >>> 'platform':'', 'network':'', 'cpu':''} >>> >>> for key in mancriteria.keys(): >>> if not mancriteria[key] or mancriteria[key] == '': >>> continue # no criteria for instance key >>> >>> svalue = AIdb.formatValue(key, mancriteria[key]) >>> >>> if key.find('MAX') == 0 or key.find('MIN') == 0: >>> tkey = key[3:] # strip off the MAX or MIN for a new >>> keyname >>> if tdict[tkey] != '': # dealing with range >>> if tdict[tkey] != svalue: >>> if max(svalue, tdict[tkey]) == svalue: >>> tdict[tkey] = tdict[tkey]+' - '+svalue >>> else: >>> tdict[tkey] = svalue+' - '+tdict[tkey] >>> else: # first value, not range yet >>> tdict[tkey] = svalue >>> else: # not a range manifest criteria >>> tdict[key] = svalue >>> >>> return tdict >>> >>> Hopefully, these changes help. >>> >>>> line 891: ensure >>> >>> Several corrected. >>> >>>> >>>> Thank you, >>>> Clay >>>> >>>> >>>> On Tue, 1 Dec 2009, John Fischer wrote: >>>> >>>>> All, >>>>> >>>>> Updated webrev for this set of changes at: >>>>> >>>>> http://cr.opensolaris.org/~johnfisc/list >>>>> >>>>> These changes include feedback during a phone conversation from Ethan >>>>> as well as the installadm man page change to document the list >>>>> subcommand. >>>>> >>>>> Thanks, >>>>> >>>>> John >>>>> >>>>> John Fischer wrote: >>>>>> Ethan, Clay and Sundar, >>>>>> >>>>>> When you get a chance can you review my webrev at: >>>>>> >>>>>> http://cr.opensolaris.org/~johnfisc/list >>>>>> >>>>>> These changes are for the installadm list subcommand fix and >>>>>> will address: >>>>>> >>>>>> 4330 - installadm list should show which server provide the >>>>>> install >>>>>> services. >>>>>> 4113 - nice to have an option to show what service the client is >>>>>> using >>>>>> 4298 - installadm list -n should give different output if >>>>>> there is >>>>>> no custom manifest >>>>>> 4597 - installadm list: expect usage statement when giving >>>>>> service >>>>>> name without "-n" flag. >>>>>> 4646 - list: showing added manifest for a non running service. >>>>>> 5300 - list: does not show informational message for non running >>>>>> service. >>>>>> 8496 - list: no verbiage indicating that a service does not exist >>>>>> when a non-existent service is given. >>>>>> 8529 - 'installadm list' command lists same service three times >>>>>> 6811 - list: should have similar output between list and list -n >>>>>> 8015 - list-manifests output is hard to read >>>>>> 9094 - installadm list: prints colons for empty MAC fields, and >>>>>> doesn't account for colons or periods in MAC field's width >>>>>> 4175 - install list error slips out >>>>>> >>>>>> The changed and added files are: >>>>>> >>>>>> usr/src/cmd/installadm/Makefile >>>>>> usr/src/pkgdefs/SUNWinstalladm-tools/prototype_com >>>>>> usr/src/cmd/installadm/installadm.c >>>>>> usr/src/cmd/installadm/installadm.h >>>>>> usr/src/cmd/installadm/list.py >>>>>> >>>>>> These changes also include a change to the Makefile and prototype_com >>>>>> file for the delete_service and delete_client to be consistent with >>>>>> the other python scripts within the /usr/lib/installadm directory. >>>>>> >>>>>> The remote service listing is being postponed due to a timing issue >>>>>> with multiple remote services that grows with the number of services >>>>>> within the domain. >>>>>> >>>>>> Thanks, >>>>>> >>>>>> John >>>>>> _______________________________________________ >>>>>> caiman-discuss mailing list >>>>>> caiman-discuss at opensolaris.org >>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>> _______________________________________________ >>>>> caiman-discuss mailing list >>>>> caiman-discuss at opensolaris.org >>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
