)]}'
{"nova/virt/baremetal/driver.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c8713dc7c9e8427a997b92e2d8178e8ef1aecff1","unresolved":false,"context_lines":[{"line_number":361,"context_line":"        pm \u003d get_power_manager(node\u003dnode, instance\u003dinstance)"},{"line_number":362,"context_line":"        ps \u003d pm.is_power_on()"},{"line_number":363,"context_line":"        pstate \u003d power_state.NOSTATE"},{"line_number":364,"context_line":"        if ps \u003d\u003d True:"},{"line_number":365,"context_line":"            pstate \u003d power_state.RUNNING"},{"line_number":366,"context_line":"        elif ps \u003d\u003d False:"},{"line_number":367,"context_line":"            pstate \u003d power_state.SHUTDOWN"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FWGw%3D","line":364,"updated":"2013-05-14 02:31:26.000000000","message":"I think you should add some documentation for the None case in the code to clarify why you\u0027re just checking True/False explicitly with if/elif rather than:\n\nif ps:\n    xxx\nelse:\n    xxx","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"49bf6bedc2c599adfa824ac375eda3defa4d9455","unresolved":false,"context_lines":[{"line_number":361,"context_line":"        pm \u003d get_power_manager(node\u003dnode, instance\u003dinstance)"},{"line_number":362,"context_line":"        ps \u003d pm.is_power_on()"},{"line_number":363,"context_line":"        pstate \u003d power_state.NOSTATE"},{"line_number":364,"context_line":"        if ps \u003d\u003d True:"},{"line_number":365,"context_line":"            pstate \u003d power_state.RUNNING"},{"line_number":366,"context_line":"        elif ps \u003d\u003d False:"},{"line_number":367,"context_line":"            pstate \u003d power_state.SHUTDOWN"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FWDA%3D","line":364,"updated":"2013-05-14 03:12:08.000000000","message":"I would write this as:\nps \u003d pm.is_power_on()\nif ps:\n  pstate \u003d power_state.RUNNING\nelif ps is False:\n  pstate \u003d power_state.SHUTDOWN\nelse:\n  pstate \u003d power_state.NOSTATE\n\n- it\u0027s more explicit that there are three states, at the cost of one line of code.","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"49f02ee14a7d7adb4345b116e82239264bd75bcd","unresolved":false,"context_lines":[{"line_number":361,"context_line":"        pm \u003d get_power_manager(node\u003dnode, instance\u003dinstance)"},{"line_number":362,"context_line":"        ps \u003d pm.is_power_on()"},{"line_number":363,"context_line":"        pstate \u003d power_state.NOSTATE"},{"line_number":364,"context_line":"        if ps \u003d\u003d True:"},{"line_number":365,"context_line":"            pstate \u003d power_state.RUNNING"},{"line_number":366,"context_line":"        elif ps \u003d\u003d False:"},{"line_number":367,"context_line":"            pstate \u003d power_state.SHUTDOWN"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FVI0%3D","line":364,"in_reply_to":"AAAAOn%2F%2FWDA%3D","updated":"2013-05-14 14:27:35.000000000","message":"Done","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2747d116b7d685c54f13bab5984aa85d95a159eb","unresolved":false,"context_lines":[{"line_number":361,"context_line":"        # NOTE(deva): Power manager may not be able to determine power state"},{"line_number":362,"context_line":"        #             in which case it may return \"None\" here."},{"line_number":363,"context_line":"        ps \u003d pm.is_power_on()"},{"line_number":364,"context_line":"        if ps \u003d\u003d True:"},{"line_number":365,"context_line":"            pstate \u003d power_state.RUNNING"},{"line_number":366,"context_line":"        elif ps \u003d\u003d False:"},{"line_number":367,"context_line":"            pstate \u003d power_state.SHUTDOWN"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOn%2F%2FVBc%3D","line":364,"updated":"2013-05-14 15:03:48.000000000","message":"Nit, but this could just be: if ps:","commit_id":"4b53581a2303241fcd617c3ccc80c0b479051d58"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"54ca54c60dd776ff69f5fe2ad5c02363605ba27a","unresolved":false,"context_lines":[{"line_number":361,"context_line":"        # NOTE(deva): Power manager may not be able to determine power state"},{"line_number":362,"context_line":"        #             in which case it may return \"None\" here."},{"line_number":363,"context_line":"        ps \u003d pm.is_power_on()"},{"line_number":364,"context_line":"        if ps \u003d\u003d True:"},{"line_number":365,"context_line":"            pstate \u003d power_state.RUNNING"},{"line_number":366,"context_line":"        elif ps \u003d\u003d False:"},{"line_number":367,"context_line":"            pstate \u003d power_state.SHUTDOWN"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOn%2F%2FU9o%3D","line":364,"in_reply_to":"AAAAOn%2F%2FU%2FE%3D","updated":"2013-05-14 15:17:44.000000000","message":"Ah, good catch, thanks. This is what happens when I fix patches before coffee :)","commit_id":"4b53581a2303241fcd617c3ccc80c0b479051d58"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"c6945f81433f7fc4ae3036787e671bdbaf74ec1e","unresolved":false,"context_lines":[{"line_number":361,"context_line":"        # NOTE(deva): Power manager may not be able to determine power state"},{"line_number":362,"context_line":"        #             in which case it may return \"None\" here."},{"line_number":363,"context_line":"        ps \u003d pm.is_power_on()"},{"line_number":364,"context_line":"        if ps \u003d\u003d True:"},{"line_number":365,"context_line":"            pstate \u003d power_state.RUNNING"},{"line_number":366,"context_line":"        elif ps \u003d\u003d False:"},{"line_number":367,"context_line":"            pstate \u003d power_state.SHUTDOWN"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOn%2F%2FU%2FE%3D","line":364,"in_reply_to":"AAAAOn%2F%2FVBc%3D","updated":"2013-05-14 15:12:52.000000000","message":"Agreed, this would make it consistent with the code that you added in is_power_on() method in ipmi.py.","commit_id":"4b53581a2303241fcd617c3ccc80c0b479051d58"}],"nova/virt/baremetal/ipmi.py":[{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"49bf6bedc2c599adfa824ac375eda3defa4d9455","unresolved":false,"context_lines":[{"line_number":144,"context_line":"        def _wait_for_power_on():"},{"line_number":145,"context_line":"            \"\"\"Called at an interval until the node\u0027s power is on.\"\"\""},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"            if self.is_power_on() \u003d\u003d True:"},{"line_number":148,"context_line":"                self.state \u003d baremetal_states.ACTIVE"},{"line_number":149,"context_line":"                raise loopingcall.LoopingCallDone()"},{"line_number":150,"context_line":"            if self.retries \u003e CONF.baremetal.ipmi_power_retry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FWDs%3D","line":147,"updated":"2013-05-14 03:12:08.000000000","message":"It\u0027s more idiomatic to say \u0027if self.is_power_on():\u0027 here.","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"49f02ee14a7d7adb4345b116e82239264bd75bcd","unresolved":false,"context_lines":[{"line_number":144,"context_line":"        def _wait_for_power_on():"},{"line_number":145,"context_line":"            \"\"\"Called at an interval until the node\u0027s power is on.\"\"\""},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"            if self.is_power_on() \u003d\u003d True:"},{"line_number":148,"context_line":"                self.state \u003d baremetal_states.ACTIVE"},{"line_number":149,"context_line":"                raise loopingcall.LoopingCallDone()"},{"line_number":150,"context_line":"            if self.retries \u003e CONF.baremetal.ipmi_power_retry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FVIw%3D","line":147,"in_reply_to":"AAAAOn%2F%2FWDs%3D","updated":"2013-05-14 14:27:35.000000000","message":"Done","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"49bf6bedc2c599adfa824ac375eda3defa4d9455","unresolved":false,"context_lines":[{"line_number":166,"context_line":"        def _wait_for_power_off():"},{"line_number":167,"context_line":"            \"\"\"Called at an interval until the node\u0027s power is off.\"\"\""},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            if self.is_power_on() \u003d\u003d False:"},{"line_number":170,"context_line":"                self.state \u003d baremetal_states.DELETED"},{"line_number":171,"context_line":"                raise loopingcall.LoopingCallDone()"},{"line_number":172,"context_line":"            if self.retries \u003e CONF.baremetal.ipmi_power_retry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FWDg%3D","line":169,"updated":"2013-05-14 03:12:08.000000000","message":"And here, if you need to check for False - which I think you do (vs Falsy), you should say \u0027if self.is_power_on() is False:\u0027 - True, False and None are singletons.","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"49f02ee14a7d7adb4345b116e82239264bd75bcd","unresolved":false,"context_lines":[{"line_number":166,"context_line":"        def _wait_for_power_off():"},{"line_number":167,"context_line":"            \"\"\"Called at an interval until the node\u0027s power is off.\"\"\""},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            if self.is_power_on() \u003d\u003d False:"},{"line_number":170,"context_line":"                self.state \u003d baremetal_states.DELETED"},{"line_number":171,"context_line":"                raise loopingcall.LoopingCallDone()"},{"line_number":172,"context_line":"            if self.retries \u003e CONF.baremetal.ipmi_power_retry:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FVIs%3D","line":169,"in_reply_to":"AAAAOn%2F%2FWDg%3D","updated":"2013-05-14 14:27:35.000000000","message":"Done","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c8713dc7c9e8427a997b92e2d8178e8ef1aecff1","unresolved":false,"context_lines":[{"line_number":209,"context_line":"        self._power_off()"},{"line_number":210,"context_line":"        return self.state"},{"line_number":211,"context_line":""},{"line_number":212,"context_line":"    def is_power_on(self):"},{"line_number":213,"context_line":"        res \u003d self._exec_ipmitool(\"power status\")[0]"},{"line_number":214,"context_line":"        if res \u003d\u003d (\"Chassis Power is on\\n\"):"},{"line_number":215,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FWHQ%3D","line":212,"updated":"2013-05-14 02:31:26.000000000","message":"Since this is a public method that is being called by another part of this change, you should add some doc string to this method explaining the values it can return and what those mean.  Otherwise in reading it\u0027s usage, it\u0027s confusing why the result is explicitly checked against True and False rather than just doing:\n\nif is_power_on():\n    xxx\nelse:\n    xxx","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"49f02ee14a7d7adb4345b116e82239264bd75bcd","unresolved":false,"context_lines":[{"line_number":209,"context_line":"        self._power_off()"},{"line_number":210,"context_line":"        return self.state"},{"line_number":211,"context_line":""},{"line_number":212,"context_line":"    def is_power_on(self):"},{"line_number":213,"context_line":"        res \u003d self._exec_ipmitool(\"power status\")[0]"},{"line_number":214,"context_line":"        if res \u003d\u003d (\"Chassis Power is on\\n\"):"},{"line_number":215,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FVIc%3D","line":212,"in_reply_to":"AAAAOn%2F%2FWHQ%3D","updated":"2013-05-14 14:27:35.000000000","message":"Done","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c8713dc7c9e8427a997b92e2d8178e8ef1aecff1","unresolved":false,"context_lines":[{"line_number":211,"context_line":""},{"line_number":212,"context_line":"    def is_power_on(self):"},{"line_number":213,"context_line":"        res \u003d self._exec_ipmitool(\"power status\")[0]"},{"line_number":214,"context_line":"        if res \u003d\u003d (\"Chassis Power is on\\n\"):"},{"line_number":215,"context_line":"            return True"},{"line_number":216,"context_line":"        elif res \u003d\u003d (\"Chassis Power is off\\n\"):"},{"line_number":217,"context_line":"            return False"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FWHE%3D","line":214,"updated":"2013-05-14 02:31:26.000000000","message":"I\u0027d hope that these strings wouldn\u0027t change at some point and break this code, would it at least be a bit better to check on a substring, e.g. if \"is on\" in res:?  Or convert the original string to lower case and check against that?","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"49f02ee14a7d7adb4345b116e82239264bd75bcd","unresolved":false,"context_lines":[{"line_number":211,"context_line":""},{"line_number":212,"context_line":"    def is_power_on(self):"},{"line_number":213,"context_line":"        res \u003d self._exec_ipmitool(\"power status\")[0]"},{"line_number":214,"context_line":"        if res \u003d\u003d (\"Chassis Power is on\\n\"):"},{"line_number":215,"context_line":"            return True"},{"line_number":216,"context_line":"        elif res \u003d\u003d (\"Chassis Power is off\\n\"):"},{"line_number":217,"context_line":"            return False"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FVE4%3D","line":214,"in_reply_to":"AAAAOn%2F%2FWDQ%3D","updated":"2013-05-14 14:27:35.000000000","message":"I\u0027ve added a note to the origin of these messages. If ipmitool were to change, yes, we\u0027d have to address that, but I would rather not simply guess at how it might change.\n\nI also dislike doing substring matching here -- if the output of ipmitool were to change, and we only matched a substring, what\u0027s to guarantee that an incompatible change would simply go unnoticed?","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"49bf6bedc2c599adfa824ac375eda3defa4d9455","unresolved":false,"context_lines":[{"line_number":211,"context_line":""},{"line_number":212,"context_line":"    def is_power_on(self):"},{"line_number":213,"context_line":"        res \u003d self._exec_ipmitool(\"power status\")[0]"},{"line_number":214,"context_line":"        if res \u003d\u003d (\"Chassis Power is on\\n\"):"},{"line_number":215,"context_line":"            return True"},{"line_number":216,"context_line":"        elif res \u003d\u003d (\"Chassis Power is off\\n\"):"},{"line_number":217,"context_line":"            return False"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FWDQ%3D","line":214,"in_reply_to":"AAAAOn%2F%2FWHE%3D","updated":"2013-05-14 03:12:08.000000000","message":"That could be done, but I think it should be a separate patch - dealing with ipmitool variation needs to be done based on examining it\u0027s behaviour, rather than guesswork.","commit_id":"84565d35776b7edd5d0cbe2748c0ef4eb872aa08"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2747d116b7d685c54f13bab5984aa85d95a159eb","unresolved":false,"context_lines":[{"line_number":234,"context_line":""},{"line_number":235,"context_line":"        :returns: True if on; False if off; None if unable to determine."},{"line_number":236,"context_line":"        \"\"\""},{"line_number":237,"context_line":"        # NOTE(deva): string matching based on"},{"line_number":238,"context_line":"        #             http://ipmitool.cvs.sourceforge.net/"},{"line_number":239,"context_line":"        #               viewvc/ipmitool/ipmitool/lib/ipmi_chassis.c"},{"line_number":240,"context_line":"        res \u003d self._exec_ipmitool(\"power status\")[0]"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAOn%2F%2FVBI%3D","line":237,"updated":"2013-05-14 15:03:48.000000000","message":"Nice link, thanks - this is helpful.","commit_id":"4b53581a2303241fcd617c3ccc80c0b479051d58"}]}