Thanks Sundar, Dave and Clay!! I know that it was a lot to review and truly appreciate the help!!
John Sundar Yamunachari wrote: > John, > > The changes look good. > > - Sundar > > 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 >
