> Change Summary: > --------------- > * Added new --force option to pkg refresh subcommand > > * Added 'force' parameter to pkg.client.api.ImageInterface.refresh() to > allow forcing an incremental refresh of publisher metadata.
It doesn't make sense to have the --force option. I agree that we should only implicitly refresh the catalog after refresh interval has elapsed; however, if the user requests a refresh, they shouldn't have to type refresh -f (or --force) to get the desired behavior. > webrev: > http://cr.opensolaris.org/~swalker/pkg-7587/ imageconfig.py: - lines 426 and 427: Is this ever true in any other case besides "refresh_seconds" being None? if repo_data["refresh_seconds"] != 0 and not repo_data["refresh_seconds"] If not, please write this as if repo_data["refresh_seconds"] is None publisher.py: - lines 806-848: This construction seems awkward. How about re-writing this as: def __set_last_refreshed(self, value): if not self.meta_root: return if value is not None and not isinstance(value, dt.datetime): raise api_errors.BadRepositoryAttributeValue( "last_refreshed", value=value) elif not value: # If no value was provided, attempt to remove # the tracking file. try: portable.remove(lcfile) except EnvironmentError, e: # If the file can't be removed due to # permissions, a read-only filesystem, # or because it doesn't exist, continue on. if e.errno not in (errno.ENOENT, errno.EACCES, errno.EROFS): raise return lcfile = os.path.join(self.meta_root, "last_refreshed") try: os.makedirs(self.meta_root) except EnvironmentError, e: if e.errno != errno.EEXIST: raise # If a time was provided, write out a special file that # can be used to track the information with the actual # time (in UTC) contained within. try: f = open(lcfile, "wb") f.write("%s\n" % misc.time_to_timestamp( calendar.timegm(value.utctimetuple()))) f.close() except EnvironmentError, e: # If the file can't be written due to # permissions or because the filesystem is # readonly, continue on. if e.errno not in (errno.EACCES, errno.EROFS): raise return While we're on the subject of this function, I also don't think it makes a whole lot of sense to always call makedirs. I would think that you would want to move this case info the open(lcfile)'s exception handler. If you extract the code starting a f = open up to f.close() into its own function, it won't be so hard to call this a second time from the exception handler. image.py: - line 356: Shouldn't the image know what the meta-root path should be here? How about writing this as a function? We could probably use this again somplace else. - line 1438: Why is this separate from retrieve_catalogs, and what does it mean to refresh a publisher? The catalog is what we're trying to refresh. I'm also concerned about accumulating random disjoint policy in different places in the image object. Are there any parts of this function that can be extracted and moved to the publisher object? - line 1590: Let's not add XXX comments, if possible. I know how to handle this case. The catalog's origin must be in the set of origin urls, instead of matching just one. This is another reason why I was asking you about a straight-forward mechanism to get all of a publishers origin URLs either as an iterator or list. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
