John (and All), Thanks for comments. My comments to yours inline. New webrev:
http://cr.opensolaris.org/~migi/22_09_be_management_3201_misc_v3/ Please let me know if anyone want me to change something else. If not I will commit those changes to the gate Tomorrow morning (Irish time). best Michal jmr wrote: > Michal - ran pylint on packagemanager.py and beadmin.py, you need to do > a little clean up. Other than that and the few comments below looks good. > > JR > > 205 column = gtk.TreeViewColumn(self.parent._("Active on > Reboot"), \ > 206 radio_renderer, active = 5) # 1 = BE_MARK > > Assume should be 5 = BE_DEFAULT This is changed and now we are using names instead of numbers, see below. > > 388 def __create_view_with_be(self, be_list): > > This is being called with gobject.idle_add so presumably you don't need > gobject.idle_add calls within it? > > 402 gobject.idle_add(self.__error_occured, msg, > False) > Absolutelly... changed. > Would be clearer in > > 258 def __activate(self): > If various row numbers where specified using named class variables, to > indicate what they mean, as you do with the various INFO_ZZZ values. Changed. We do have now instead of numbers names: 52 #BE_LIST 53 ( 54 BE_ID, 55 BE_MARKED, 56 BE_NAME, 57 BE_DATE_TIME, 58 BE_CURRENT_PIXBUF, 59 BE_ACTIVE_DEFAULT, 60 BE_SIZE 61 ) = range(7) > Are creation dates going to be available from BEADM ? Would it be better to > use BEADM to provide them if possible and have this zfs approach as the fall > back on failure. The current behaviour is exactly like this. If BEADM provides date we are using the one provided by BEADM, if not we are using zfs approach as an fall back: http://cr.opensolaris.org/~migi/22_09_be_management_3201_misc_v3/src/gui/modules/beadmin.py.html 423 be_date = bee.get("date") if be_date (provided by BEADM) we are using be_date if NOT (fallback) then we are getting list of be_dates: 427 if not be_date and j == 0: 428 dates = self.__get_dates_of_creation(be_list_loop) 429 if dates: so bascally BEADM should give us a be_date if not then we are calling __get_dates_of_creation which returns the list of dates for all be's. We are calling this only once to get all the dates at once, to not call zfs as many times as number of be's (j == 0 <--- only first time) So de facto this code will never be invoked if BEADM provides us the bee.get("date") which is the case in the new libbe. > > 453 def __get_dates_of_creation(self, be_list): The be_list doesn't have the above numbers, as this list is provided by beList() from libbe and it's just 2 value list, first is the error number and second is dictionary. > > > > sts/gui_pylintrc /tmp/packagemanager.py > ************* Module packagemanager > W:629:PackageManager.__on_update_all: Unused variable 'fmris' > E:1068:PackageManager.__ipkg_ipkgui_uptodate: Module 'pkg.client.image' > has no 'CatalogRefreshException' member > E:1226:PackageManager.__get_image_from_directory: Module > 'pkg.client.image' has no 'InventoryException' member > R:1216:PackageManager.__get_image_from_directory: Too many branches (27/25) > W:1226:PackageManager.__get_image_from_directory: Unused variable 'e' > W:1487:PackageManager.__installed_fmris_from_args: Redefining name > 'args' from outer scope (line 1872) > E:1493:PackageManager.__installed_fmris_from_args: Module > 'pkg.client.image' has no 'InventoryException' member > R:1487:PackageManager.__installed_fmris_from_args: Method could be a > function > W:1593:PackageManager.get_installed_version: Access to a protected > member _get_version_installed of a client class > > ************* Module beadmin > C:433: Line too long (93/90) > W:472: Bad indentation. Found 32 spaces, expected 24 > W: 64:Beadmin.__init__: Unused variable 'result' > W:259:Beadmin.__activate: Unused variable 'to_delete' > C:330:Beadmin.__delete_activate_be: Operator not preceded by a space > msg+= "\n\n" > ^^ > W:347:Beadmin.__error_occured: Unused variable 'result' > W:367:Beadmin.__enable_disable_reset: Unreachable code > W:367:Beadmin.__enable_disable_reset: Statement seems to have no effect > W:368:Beadmin.__enable_disable_reset: Statement seems to have no effect > W:430:Beadmin.__create_view_with_be: No exception type(s) specified > W:425:Beadmin.__create_view_with_be: Used * or ** magic > R:448:Beadmin.__convert_size_of_be_to_string: Method could be a function > W:471:Beadmin.__get_dates_of_creation: Except doesn't do anything > R:453:Beadmin.__get_dates_of_creation: Method could be a function > W:471:Beadmin.__get_dates_of_creation: Unused variable 'e' > R:485:Beadmin.__beadm_invoke_command: Method could be a function > W:492:Beadmin.__beadm_invoke_command: Unused variable 'e' > W:511:Beadmin.__check_if_be_supports_capital_f: Except doesn't do anything > R:496:Beadmin.__check_if_be_supports_capital_f: Method could be a function > W:511:Beadmin.__check_if_be_supports_capital_f: Unused variable 'e' > R:533:Beadmin.__set_renderer_active: Method could be a function > R:541:Beadmin.__cell_data_function: Method could be a function > E: 55:Beadmin.__init__: Access to member 'parent' before its definition > line 68 > E: 63:Beadmin.__init__: Access to member 'parent' before its definition > line 68 > > > > > Michal Pryc wrote: >> Hello, >> I am sending this e-mail to caiman-discuss as well to the pkg-discuss. >> For people from the caiman-discuss, which didn't follow the thread, the >> orignal message is at: >> >> http://mail.opensolaris.org/pipermail/pkg-discuss/2008-September/006334.html >> >> In short: >> There is an requirement for packagemanager for 2008.11 to have simple >> dialog which allows users to delete and switch between BE's created >> during image-update. >> >> Screenshots from current working BE Management are at: >> http://cr.opensolaris.org/~migi/be_admin_screenshots_v0/ >> >> Because packagemanager and IPS are back-published to all opensolaris >> builds, those part needs to work with all versions of libbe and beadm. >> That is why in addition to the first webrev, few things needed tweaking. >> I have tested this code on builds 86, 94 and 98. >> >> What was changed? The descriptions will contains numbers e.g. {1-30}, >> those numbers are line numbers for the beadmin.py: >> http://cr.opensolaris.org/~migi/18_09_be_management_3201_misc_v1/src/gui/modules/beadmin.py.html >> >> The version beadm in build 86 was using "-f" flag instead of "-F" for >> destroy action. Current webrev doesn't check for the opensolaris build >> but it checks if output from help message contains only "[-f]" if yes >> then it means that we should use "-f" in all other we should use "-F" as >> this is new version of beadm {496-513, 153-154, 477-478}. This is safer >> then checking for build number and assuming that user have proper >> version of beadm and libbe. >> >> There was another change between build 86 and 98. The libbe.beList() >> returns different format and different information. We are checking if >> there was any error while getting the list of be's and if there was we >> are preventing user to use BE management {392-405}. The older libbe >> didn't have any error checking, so we are assuming that the list if correct. >> >> Another difference was with the dates of creation of be. The new libbe >> contains such info within libbe.beList() but the older version didn't >> have such information, so we are obtaining this from the zfs command >> (but only when libbe didn't give us such info) {413, 417-433, 453-473} >> >> The whole webrev is at: >> >> http://cr.opensolaris.org/~migi/18_09_be_management_3201_misc_v1/ >> >> Also during tests I have discovered that some dialogs are improperly >> used by packagemanager.py (run without destroy) which prevented beadm to >> work properly in some cases. This webrev fixes those. >> >> best >> Michal Pryc >> >> >> Ethan Quach wrote: >> >>> Michal Pryc wrote: >>> >>>> Ethan Quach wrote: >>>> >>>> >>>>> Michal, >>>>> >>>>> beadmin.py: >>>>> >>>>> 447 - I would highly recommend against constructing this path. >>>>> Use bee.get("root_ds") instead (we will likely implement a better api >>>>> to get these elements in the future so that apps don't need to know these >>>>> key names. We won't change the key names of course...) >>>>> >>>>> >>>> The problem was to get the date of the creation of BE, I couldn't find >>>> this through libbe so I need to make this trick of getting date of >>>> creation snapshot. I the bee.get("root_ds") something introduced after >>>> nevada 94? I can't get this value (It's always returning None). >>>> >>>> >>> Sigh. You're right, it wasn't added until build 96, which also added a >>> "date" >>> element which is what you're really after. Constructing this path >>> shouldn't be >>> done because its private, and it also won't work when we enable this code to >>> work inside of zones. Perhaps we do the conditional here again, and >>> *try* to >>> get the "date" element, and if that fails, do what you have now. >>> >> >> _______________________________________________ >> pkg-discuss mailing list >> [email protected] >> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss >> > > _______________________________________________ > pkg-discuss mailing list > [email protected] > http://mail.opensolaris.org/mailman/listinfo/pkg-discuss _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
