)]}'
{"trove/guestagent/common/operating_system.py":[{"author":{"_account_id":10725,"name":"Sergey Gotliv","email":"sgotliv@redhat.com","username":"sgotliv"},"change_message_id":"bad758233892f9adef48381e03ccfa8738cc5253","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    elif os.patch.isfile(\"/etc/debian_version\"):"},{"line_number":30,"context_line":"        return DEBIAN"},{"line_number":31,"context_line":"    else:"},{"line_number":32,"context_line":"        return None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def service_discovery(service_candidates):"}],"source_content_type":"text/x-python","patch_set":13,"id":"1abeadc6_c5b4bcd3","line":32,"updated":"2014-08-25 11:19:33.000000000","message":"Looks better. \n\nI think that in case we can\u0027t detect an OS - the error/exception is much better than None.","commit_id":"403f4b24d6d305895f425c7def3667839588abd2"},{"author":{"_account_id":2340,"name":"Ionut Artarisi","email":"iartarisi@suse.cz","username":"mapleoin"},"change_message_id":"453c1335bb175ee9d28cac141848fe9cd05e36af","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    elif os.patch.isfile(\"/etc/debian_version\"):"},{"line_number":30,"context_line":"        return DEBIAN"},{"line_number":31,"context_line":"    else:"},{"line_number":32,"context_line":"        return None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def service_discovery(service_candidates):"}],"source_content_type":"text/x-python","patch_set":13,"id":"1abeadc6_6b2bb733","line":32,"in_reply_to":"1abeadc6_0b47a31e","updated":"2014-08-25 12:13:42.000000000","message":"Also, I don\u0027t think that an unknown operating system is an exceptional state (after all there are only two currently supported so far). This should be part of the normal workflow when detecting operating systems.","commit_id":"403f4b24d6d305895f425c7def3667839588abd2"},{"author":{"_account_id":10725,"name":"Sergey Gotliv","email":"sgotliv@redhat.com","username":"sgotliv"},"change_message_id":"044aa9446042dd74e8591e5603fa34142b926dd6","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    elif os.patch.isfile(\"/etc/debian_version\"):"},{"line_number":30,"context_line":"        return DEBIAN"},{"line_number":31,"context_line":"    else:"},{"line_number":32,"context_line":"        return None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def service_discovery(service_candidates):"}],"source_content_type":"text/x-python","patch_set":13,"id":"1abeadc6_a8aa3583","line":32,"in_reply_to":"1abeadc6_2580108b","updated":"2014-08-25 12:02:09.000000000","message":"I am trying to read this method only and forget for the moment the entire patch.\nThis code is common, right? So what does it mean OS is None? How is that possible?\n\nWhat next, I have to check returned value against None in every place and then decide what to do?\n\nAnyway, I think both approaches can be valid, but mine is better :-).","commit_id":"403f4b24d6d305895f425c7def3667839588abd2"},{"author":{"_account_id":10725,"name":"Sergey Gotliv","email":"sgotliv@redhat.com","username":"sgotliv"},"change_message_id":"3417e45bab1060fd71da5600405f60c996f25362","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    elif os.patch.isfile(\"/etc/debian_version\"):"},{"line_number":30,"context_line":"        return DEBIAN"},{"line_number":31,"context_line":"    else:"},{"line_number":32,"context_line":"        return None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def service_discovery(service_candidates):"}],"source_content_type":"text/x-python","patch_set":13,"id":"1abeadc6_a109422e","line":32,"in_reply_to":"1abeadc6_31ca096f","updated":"2014-08-26 12:36:18.000000000","message":"You probably misunderstand my previous comment. \n\nI suggest to throw exception here! This situation is abnormal, If you can\u0027t detect OS you are done, there is no reason to move forward and print nice but useless warnings.\n\nIf you want in your particular case catch this exception and move forward but in general because this is a common code don\u0027t impose the caller check Noneness.","commit_id":"403f4b24d6d305895f425c7def3667839588abd2"},{"author":{"_account_id":6268,"name":"Robert Myers","email":"myer0052@gmail.com","username":"rmyers"},"change_message_id":"bdd8cbb3f8d37178b4f9c0b3fde6bb11532a451f","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    elif os.patch.isfile(\"/etc/debian_version\"):"},{"line_number":30,"context_line":"        return DEBIAN"},{"line_number":31,"context_line":"    else:"},{"line_number":32,"context_line":"        return None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def service_discovery(service_candidates):"}],"source_content_type":"text/x-python","patch_set":13,"id":"1abeadc6_31ca096f","line":32,"in_reply_to":"1abeadc6_6b2bb733","updated":"2014-08-25 15:08:16.000000000","message":"I agree that None is fine to return here. The last thing we want is to assume that the os is a certain type. That is much worse than getting a None value for os. Also the way this should work should be contained in a few files. For example the datastore managers should differ to the package managers to install rather than using this function directly to determine what to run. So only common code that is operating system specific should use this function.","commit_id":"403f4b24d6d305895f425c7def3667839588abd2"},{"author":{"_account_id":2340,"name":"Ionut Artarisi","email":"iartarisi@suse.cz","username":"mapleoin"},"change_message_id":"61f2ecf794701fcf146b3dcff9b7fe9819b163cb","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    elif os.patch.isfile(\"/etc/debian_version\"):"},{"line_number":30,"context_line":"        return DEBIAN"},{"line_number":31,"context_line":"    else:"},{"line_number":32,"context_line":"        return None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def service_discovery(service_candidates):"}],"source_content_type":"text/x-python","patch_set":13,"id":"5a890539_0cdb667c","line":32,"in_reply_to":"1abeadc6_a109422e","updated":"2014-11-18 09:56:29.000000000","message":"I disagree and I tried to explain this in the bug as well. If the OS is not detected, everything should proceed as normal since the OS itself can (and arguably SHOULD) do the packages installation/configuration itself rather than letting packages install other packages.","commit_id":"403f4b24d6d305895f425c7def3667839588abd2"},{"author":{"_account_id":2340,"name":"Ionut Artarisi","email":"iartarisi@suse.cz","username":"mapleoin"},"change_message_id":"8741d1b1b29acec10fd030169c652f447454463c","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    elif os.patch.isfile(\"/etc/debian_version\"):"},{"line_number":30,"context_line":"        return DEBIAN"},{"line_number":31,"context_line":"    else:"},{"line_number":32,"context_line":"        return None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def service_discovery(service_candidates):"}],"source_content_type":"text/x-python","patch_set":13,"id":"1abeadc6_0b47a31e","line":32,"in_reply_to":"1abeadc6_a8aa3583","updated":"2014-08-25 12:12:09.000000000","message":"I think None is a pretty commonly used return value for an unknown state. You have to check the return value of this function anyway and as the function name is prefixed by \u0027get_\u0027 it even follows the normal naming pattern of python functions which return None when no proper result was found. Raising an error from a get_* method is rather uncommon.","commit_id":"403f4b24d6d305895f425c7def3667839588abd2"},{"author":{"_account_id":2340,"name":"Ionut Artarisi","email":"iartarisi@suse.cz","username":"mapleoin"},"change_message_id":"d16ad47e5f916d508cb4982c7442213e0ec6bf34","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    elif os.patch.isfile(\"/etc/debian_version\"):"},{"line_number":30,"context_line":"        return DEBIAN"},{"line_number":31,"context_line":"    else:"},{"line_number":32,"context_line":"        return None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def service_discovery(service_candidates):"}],"source_content_type":"text/x-python","patch_set":13,"id":"1abeadc6_2580108b","line":32,"in_reply_to":"1abeadc6_c5b4bcd3","updated":"2014-08-25 11:32:26.000000000","message":"Why? The whole point of this commit is to allow the sysadmin to install the required packages/services rather than having to rely on trove\u0027s hardcoded values. There are warnings in the logs output from the NoopPackagerMixin.","commit_id":"403f4b24d6d305895f425c7def3667839588abd2"}],"trove/guestagent/pkg.py":[{"author":{"_account_id":9746,"name":"Doug Shelley","email":"dougshelley66@gmail.com","username":"dougshelley"},"change_message_id":"13faec3b93778d994a05130d2f7f50613a76335f","unresolved":false,"context_lines":[{"line_number":399,"context_line":"    \"\"\"A no-op packager which does no action, but logs all calls.\"\"\""},{"line_number":400,"context_line":"    def __getattribute__(self, name):"},{"line_number":401,"context_line":"        def noop(*args, **kwargs):"},{"line_number":402,"context_line":"            LOG.warn(_(\"The %(name)s method on the Noop Packager was \""},{"line_number":403,"context_line":"                       \"called with: %(args)s %(kwargs)s\") %"},{"line_number":404,"context_line":"                     dict(name\u003dname, args\u003dargs or \u0027\u0027, kwargs\u003dkwargs or \u0027\u0027))"},{"line_number":405,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_661e5061","line":402,"updated":"2014-07-23 13:56:00.000000000","message":"It seems like this message could be made more descriptive. I assume an operator/user isn\u0027t going to know what \"Noop\" is. The issue here is that they are trying to use an operating system which isn\u0027t properly supported by Trove, right? Wouldn\u0027t it make sense to say that in this message?","commit_id":"3b1a8f4a1f9df3c8aa7651e54b51ab78965cfca5"},{"author":{"_account_id":2340,"name":"Ionut Artarisi","email":"iartarisi@suse.cz","username":"mapleoin"},"change_message_id":"0b11b350e52f6858edce72029d17e25cf8af6981","unresolved":false,"context_lines":[{"line_number":399,"context_line":"    \"\"\"A no-op packager which does no action, but logs all calls.\"\"\""},{"line_number":400,"context_line":"    def __getattribute__(self, name):"},{"line_number":401,"context_line":"        def noop(*args, **kwargs):"},{"line_number":402,"context_line":"            LOG.warn(_(\"The %(name)s method on the Noop Packager was \""},{"line_number":403,"context_line":"                       \"called with: %(args)s %(kwargs)s\") %"},{"line_number":404,"context_line":"                     dict(name\u003dname, args\u003dargs or \u0027\u0027, kwargs\u003dkwargs or \u0027\u0027))"},{"line_number":405,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_06b75422","line":402,"in_reply_to":"baada198_661e5061","updated":"2014-07-23 14:15:36.000000000","message":"Makes sense, thanks!","commit_id":"3b1a8f4a1f9df3c8aa7651e54b51ab78965cfca5"},{"author":{"_account_id":8415,"name":"Denis M.","email":"lildee1991@gmail.com","username":"dmakogon"},"change_message_id":"6268e3535aba7808a562bb1afc4c3db5ec87c7f1","unresolved":false,"context_lines":[{"line_number":410,"context_line":"class BasePackage(type):"},{"line_number":411,"context_line":""},{"line_number":412,"context_line":"    def __new__(meta, name, bases, dct):"},{"line_number":413,"context_line":"        os \u003d operating_system.get_os()"},{"line_number":414,"context_line":"        if os \u003d\u003d operating_system.REDHAT:"},{"line_number":415,"context_line":"            bases +\u003d (RedhatPackagerMixin, )"},{"line_number":416,"context_line":"        elif os \u003d\u003d operating_system.DEBIAN:"}],"source_content_type":"text/x-python","patch_set":11,"id":"baada198_60594628","line":413,"updated":"2014-07-28 13:15:32.000000000","message":"i\u0027d suggest to do next:\n\nwrite specific dict that will have OS type as a keys and package mixing class as values, and when you\u0027ll try to ask for specific OS you would receive existing mapping or NoopPackageMixing if such OS not supported. And you wouldn\u0027t need to do this bunch of `if` statements.","commit_id":"9f63237e8e87bcd3be006ecc89c3d71694238380"},{"author":{"_account_id":2340,"name":"Ionut Artarisi","email":"iartarisi@suse.cz","username":"mapleoin"},"change_message_id":"89f8cc94267d0ce04e9a1cd03b138cf227f6049f","unresolved":false,"context_lines":[{"line_number":410,"context_line":"class BasePackage(type):"},{"line_number":411,"context_line":""},{"line_number":412,"context_line":"    def __new__(meta, name, bases, dct):"},{"line_number":413,"context_line":"        os \u003d operating_system.get_os()"},{"line_number":414,"context_line":"        if os \u003d\u003d operating_system.REDHAT:"},{"line_number":415,"context_line":"            bases +\u003d (RedhatPackagerMixin, )"},{"line_number":416,"context_line":"        elif os \u003d\u003d operating_system.DEBIAN:"}],"source_content_type":"text/x-python","patch_set":11,"id":"1abeadc6_5d9281f6","line":413,"in_reply_to":"baada198_60594628","updated":"2014-08-22 09:46:33.000000000","message":"This is a 3-branch conditional. It\u0027s right at the edge of it making sense to implement the dictionary-conditional. I think this is just nit-picking. But since no one else is reviewing this, wth, I\u0027ll do it.","commit_id":"9f63237e8e87bcd3be006ecc89c3d71694238380"},{"author":{"_account_id":10725,"name":"Sergey Gotliv","email":"sgotliv@redhat.com","username":"sgotliv"},"change_message_id":"9e740d4c757b25456cc457bbbaeac0ff9a1591e6","unresolved":false,"context_lines":[{"line_number":419,"context_line":"class BasePackage(type):"},{"line_number":420,"context_line":""},{"line_number":421,"context_line":"    def __new__(meta, name, bases, dct):"},{"line_number":422,"context_line":"        os \u003d operating_system.get_os()"},{"line_number":423,"context_line":"        mixin \u003d {"},{"line_number":424,"context_line":"            operating_system.REDHAT: RedhatPackagerMixin,"},{"line_number":425,"context_line":"            operating_system.DEBIAN: DebianPackagerMixin"}],"source_content_type":"text/x-python","patch_set":12,"id":"1abeadc6_c152f167","line":422,"updated":"2014-08-25 10:54:07.000000000","message":"Currently this code returns only 2 possible values\nREDHAT and DEBIAN, therefore NoopPackagerMixin is unreachable. \n\nDo you want to change os detection code either? \n\nPersonally I would prefer not to merge an unreachable code.","commit_id":"bec336073398f5ef1da3a50e6175e18d3504a525"},{"author":{"_account_id":9664,"name":"amrith","email":"amrith.kumar@gmail.com","username":"amrith"},"change_message_id":"e0e6498265cd766bb4d3ea5a8150aa20a2587ac4","unresolved":false,"context_lines":[{"line_number":419,"context_line":"class BasePackage(type):"},{"line_number":420,"context_line":""},{"line_number":421,"context_line":"    def __new__(meta, name, bases, dct):"},{"line_number":422,"context_line":"        os \u003d operating_system.get_os()"},{"line_number":423,"context_line":"        mixin \u003d {"},{"line_number":424,"context_line":"            operating_system.REDHAT: RedhatPackagerMixin,"},{"line_number":425,"context_line":"            operating_system.DEBIAN: DebianPackagerMixin"}],"source_content_type":"text/x-python","patch_set":12,"id":"1abeadc6_a540a008","line":422,"in_reply_to":"1abeadc6_05ab14d7","updated":"2014-08-25 11:09:41.000000000","message":"hence my observation that this was a SuSE specific patch.","commit_id":"bec336073398f5ef1da3a50e6175e18d3504a525"},{"author":{"_account_id":2340,"name":"Ionut Artarisi","email":"iartarisi@suse.cz","username":"mapleoin"},"change_message_id":"76db654387795d525935bab53de441b8ed854fee","unresolved":false,"context_lines":[{"line_number":419,"context_line":"class BasePackage(type):"},{"line_number":420,"context_line":""},{"line_number":421,"context_line":"    def __new__(meta, name, bases, dct):"},{"line_number":422,"context_line":"        os \u003d operating_system.get_os()"},{"line_number":423,"context_line":"        mixin \u003d {"},{"line_number":424,"context_line":"            operating_system.REDHAT: RedhatPackagerMixin,"},{"line_number":425,"context_line":"            operating_system.DEBIAN: DebianPackagerMixin"}],"source_content_type":"text/x-python","patch_set":12,"id":"1abeadc6_05ab14d7","line":422,"in_reply_to":"1abeadc6_c152f167","updated":"2014-08-25 11:05:27.000000000","message":"Hm, good point about not merging an unreachable branch. I was kindof basing my patch on https://review.openstack.org/#/c/108394 , but I think modifying the get_os default is the best way to go to keep this neutral.","commit_id":"bec336073398f5ef1da3a50e6175e18d3504a525"}]}