)]}'
{"nova/notifications/base.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"22ee4c0699f6636004a8b18eb65f60a350003ea7","unresolved":false,"context_lines":[{"line_number":391,"context_line":""},{"line_number":392,"context_line":""},{"line_number":393,"context_line":"def null_safe_int(s):"},{"line_number":394,"context_line":"    return int(s) if s else 0"},{"line_number":395,"context_line":""},{"line_number":396,"context_line":""},{"line_number":397,"context_line":"def null_safe_isotime(s):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff0f0b1f_f36f99f9","line":394,"updated":"2017-05-23 03:13:42.000000000","message":"So, the problem I have with this is that 0 is a valid integer. What precisely is null_safe_int() actually being called on? Let\u0027s say there is a field called net_rx_bytes that is defined in a notification object type as a nullable integer.\n\nNow let\u0027s suppose that there is no information about the amount of RX bytes received on a particular network interface, and we output the notification object after calling null_safe_int(None) for this net_rx_bytes field. By doing so, we\u0027ve now essentially told the listener of that notification message that there have been no network traffic for some resource when in fact, the real situation is that we don\u0027t *know* what the network traffic is. The two things are very different (definitely from a billing perspective!).\n\nThis is why I\u0027m -1 on this change until I see evidence that null_safe_int() isn\u0027t used on fields like what I describe above.","commit_id":"8bbffc66ef2ace102d6e0189fb3f54836f5bbf24"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"97b40197eae346d32e4595776a4cf2b1599ffb4f","unresolved":false,"context_lines":[{"line_number":391,"context_line":""},{"line_number":392,"context_line":""},{"line_number":393,"context_line":"def null_safe_int(s):"},{"line_number":394,"context_line":"    return int(s) if s else 0"},{"line_number":395,"context_line":""},{"line_number":396,"context_line":""},{"line_number":397,"context_line":"def null_safe_isotime(s):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f231b9d_0971dd0c","line":394,"in_reply_to":"7f231b9d_09455d43","updated":"2017-06-14 14:19:28.000000000","message":"After discussing on IRC, IMO we need to retain backward compatibility in the notification payload, which means sending \u0027\u0027, not 0.\n\nMy suggestion is to make null_safe_int explicit about what it does when it gets a null.\n\n def null_safe_int(s, default):\n    try:\n        return int(s)\n    except (TypeError, ValueError):\n        return default\n\n...and then call it as\n\n null_safe_int(instance.progress, \u0027\u0027)\n\n...with a comment that this is to retain backward compatibility.","commit_id":"8bbffc66ef2ace102d6e0189fb3f54836f5bbf24"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5fb7c37a4e745bb2bbf93bac22382c3c1472840c","unresolved":false,"context_lines":[{"line_number":391,"context_line":""},{"line_number":392,"context_line":""},{"line_number":393,"context_line":"def null_safe_int(s):"},{"line_number":394,"context_line":"    return int(s) if s else 0"},{"line_number":395,"context_line":""},{"line_number":396,"context_line":""},{"line_number":397,"context_line":"def null_safe_isotime(s):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f231b9d_09a67d54","line":394,"in_reply_to":"7f231b9d_0971dd0c","updated":"2017-06-14 14:23:57.000000000","message":"I was thinking along similar lines as Eric about doing a try/except on TypeError and ValueError when the cast fails.\n\nOtherwise I\u0027d make it match the REST API response:\n\nhttps://github.com/openstack/nova/blob/9dde4fe0eacc01c135401da3313c493caa99a30c/nova/api/openstack/compute/views/servers.py#L153\n\nBut ultimately, why do we even care about changing this right now? If this is only used for legacy notifications, which we want to be more or less frozen, why are we even bothering worrying about this right now?","commit_id":"8bbffc66ef2ace102d6e0189fb3f54836f5bbf24"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"205979f5111c395f67f70e854f5f2ee5f0dbef72","unresolved":false,"context_lines":[{"line_number":391,"context_line":""},{"line_number":392,"context_line":""},{"line_number":393,"context_line":"def null_safe_int(s):"},{"line_number":394,"context_line":"    return int(s) if s else 0"},{"line_number":395,"context_line":""},{"line_number":396,"context_line":""},{"line_number":397,"context_line":"def null_safe_isotime(s):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f231b9d_2c3387f8","line":394,"in_reply_to":"7f231b9d_09a67d54","updated":"2017-06-14 14:32:49.000000000","message":"I think it\u0027s because we moved these null_safe_* methods out of info_from_instance [1], so they could hypothetically be used by \"other stuff\", which makes a method that defaults an int to 0 subject to higher scrutiny.\n\n[1] https://review.openstack.org/#/c/463977/1/nova/notifications/base.py","commit_id":"8bbffc66ef2ace102d6e0189fb3f54836f5bbf24"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"b2dac9ce2d542ea05725254a80b59d52516d54d2","unresolved":false,"context_lines":[{"line_number":391,"context_line":""},{"line_number":392,"context_line":""},{"line_number":393,"context_line":"def null_safe_int(s):"},{"line_number":394,"context_line":"    return int(s) if s else 0"},{"line_number":395,"context_line":""},{"line_number":396,"context_line":""},{"line_number":397,"context_line":"def null_safe_isotime(s):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f231b9d_8c861b13","line":394,"in_reply_to":"7f231b9d_2c3387f8","updated":"2017-06-14 14:39:05.000000000","message":"So given that it\u0027s only used in the one spot, we could sweep it back under the rug by putting this method back inside info_from_instance - or better yet, inlining it on L477: \n\n progress\u003dint(instance.progress) if instance.progress else \u0027\u0027,","commit_id":"8bbffc66ef2ace102d6e0189fb3f54836f5bbf24"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"d89d5e4836c23ee95a6602e3219bfc05c7dda747","unresolved":false,"context_lines":[{"line_number":391,"context_line":""},{"line_number":392,"context_line":""},{"line_number":393,"context_line":"def null_safe_int(s):"},{"line_number":394,"context_line":"    return int(s) if s else 0"},{"line_number":395,"context_line":""},{"line_number":396,"context_line":""},{"line_number":397,"context_line":"def null_safe_isotime(s):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f231b9d_cab3ba97","line":394,"in_reply_to":"7f231b9d_8c861b13","updated":"2017-06-15 09:25:22.000000000","message":"Yeah, at least let\u0027s inline this to avoid encouraging others to use such a conversion","commit_id":"8bbffc66ef2ace102d6e0189fb3f54836f5bbf24"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"25629c3663574fdc2a2b73838f937defc11466a2","unresolved":false,"context_lines":[{"line_number":391,"context_line":""},{"line_number":392,"context_line":""},{"line_number":393,"context_line":"def null_safe_int(s):"},{"line_number":394,"context_line":"    return int(s) if s else 0"},{"line_number":395,"context_line":""},{"line_number":396,"context_line":""},{"line_number":397,"context_line":"def null_safe_isotime(s):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f231b9d_493fd5b3","line":394,"in_reply_to":"7f231b9d_f098cb1b","updated":"2017-06-14 13:57:59.000000000","message":"I dug into this a bit.  I can confirm that this method is only used on Instance.progress as gibi says.\n\nInstance.progress is used as a percentage, e.g. in a migration op.  If we\u0027re being finicky, we could argue that None means \"there\u0027s no operation in progress\" whereas 0 means \"there\u0027s an operation, but it hasn\u0027t gotten anywhere yet\".  While that\u0027s potentially a useful distinction, it isn\u0027t actually used that way in practice.\n\nFirst of all, I couldn\u0027t find anywhere the \"progress\" member of the result of get_instance_info is accessed.  (I\u0027ll admit I don\u0027t know how \"notifications\" are used - if the result is sent across the wire to an API consumer, that would explain why I couldn\u0027t find usages in nova.)\n\nThat said, the only other place I could find where instance.progress is read [1] does indeed default it to zero if it\u0027s None.\n\nThe \"progress\" notification element was introduced by johnthetubaguy three years ago [2].  There\u0027s no explanation as to why he thought it necessary to make this \"null safe\".\n\nDiscussions on IRC ongoing [3].\n\n[1] https://github.com/openstack/nova/blob/9dde4fe0eacc01c135401da3313c493caa99a30c/nova/api/openstack/compute/views/servers.py#L153\n[2] https://review.openstack.org/#/c/120053/\n[3] http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2017-06-14.log.html#t2017-06-14T13:44:50","commit_id":"8bbffc66ef2ace102d6e0189fb3f54836f5bbf24"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e6a44aa9642706c8fa7d638718ec71bae218bcf0","unresolved":false,"context_lines":[{"line_number":391,"context_line":""},{"line_number":392,"context_line":""},{"line_number":393,"context_line":"def null_safe_int(s):"},{"line_number":394,"context_line":"    return int(s) if s else 0"},{"line_number":395,"context_line":""},{"line_number":396,"context_line":""},{"line_number":397,"context_line":"def null_safe_isotime(s):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f231b9d_09455d43","line":394,"in_reply_to":"7f231b9d_f098cb1b","updated":"2017-06-14 13:58:05.000000000","message":"My suggestion is to go with b) :)","commit_id":"8bbffc66ef2ace102d6e0189fb3f54836f5bbf24"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"013fc7d303583ff03b1ab21d39a676c249a99693","unresolved":false,"context_lines":[{"line_number":391,"context_line":""},{"line_number":392,"context_line":""},{"line_number":393,"context_line":"def null_safe_int(s):"},{"line_number":394,"context_line":"    return int(s) if s else 0"},{"line_number":395,"context_line":""},{"line_number":396,"context_line":""},{"line_number":397,"context_line":"def null_safe_isotime(s):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f231b9d_f098cb1b","line":394,"in_reply_to":"ff0f0b1f_f36f99f9","updated":"2017-06-14 11:28:06.000000000","message":"I agree with your view. \n\nAt the moment null_safe_int is only used to convert Instance.progress, that is nullable, to the progress field of the legacy instance notification payload[1]. I have no information about why the progress field in the legacy payload is not allowed to contain null. \nInstance.progress is null when there is no task running on the instance.\n\nAlso the subsequent patch does not depend on the null_safe_int just on the null_safe_iso_time so technically I could move that away from this or even drop this change totally.\n\nLet\u0027s see our options:\na) Keep everything as is. This means null_safe_int returns empty string which is not a valid int seems confusing.\nb) Do not use null_safe_int() at all. This means that the legacy payload will contain null. I don\u0027t know if it breaks any consumer but potentially it can.\nc) return 0, -1 or 42 from null_safe_int(None). Unfortunately there is no good integer value to return that will be meaningful in every case.\n\n@Jay: What do you suggest, which way we should go with this?\n\n[1] https://github.com/openstack/nova/blob/9dde4fe0eacc01c135401da3313c493caa99a30c/nova/notifications/base.py#L478","commit_id":"8bbffc66ef2ace102d6e0189fb3f54836f5bbf24"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"7e4e795e066460bf2c29cfb06acfde1f94a07822","unresolved":false,"context_lines":[{"line_number":471,"context_line":"        # Status properties"},{"line_number":472,"context_line":"        state\u003dinstance.vm_state,"},{"line_number":473,"context_line":"        state_description\u003dnull_safe_str(instance.task_state),"},{"line_number":474,"context_line":"        # NOTE(gibi): It might seems wrong to default the progress to an empty"},{"line_number":475,"context_line":"        # string but this is how legacy work and this code only used by the"},{"line_number":476,"context_line":"        # legacy notification so try to keep the compatibility here but also"},{"line_number":477,"context_line":"        # keep it contained."}],"source_content_type":"text/x-python","patch_set":6,"id":"7f231b9d_b97a17b7","line":474,"range":{"start_line":474,"start_character":31,"end_line":474,"end_character":36},"updated":"2017-06-15 13:48:28.000000000","message":"seem","commit_id":"45ad7f3a2889b0234e8215cf0b2507a51542c00a"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"7e4e795e066460bf2c29cfb06acfde1f94a07822","unresolved":false,"context_lines":[{"line_number":472,"context_line":"        state\u003dinstance.vm_state,"},{"line_number":473,"context_line":"        state_description\u003dnull_safe_str(instance.task_state),"},{"line_number":474,"context_line":"        # NOTE(gibi): It might seems wrong to default the progress to an empty"},{"line_number":475,"context_line":"        # string but this is how legacy work and this code only used by the"},{"line_number":476,"context_line":"        # legacy notification so try to keep the compatibility here but also"},{"line_number":477,"context_line":"        # keep it contained."},{"line_number":478,"context_line":"        progress\u003dint(instance.progress) if instance.progress else \u0027\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"7f231b9d_79523f3d","line":475,"range":{"start_line":475,"start_character":59,"end_line":475,"end_character":63},"updated":"2017-06-15 13:48:28.000000000","message":"is only","commit_id":"45ad7f3a2889b0234e8215cf0b2507a51542c00a"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"7e4e795e066460bf2c29cfb06acfde1f94a07822","unresolved":false,"context_lines":[{"line_number":472,"context_line":"        state\u003dinstance.vm_state,"},{"line_number":473,"context_line":"        state_description\u003dnull_safe_str(instance.task_state),"},{"line_number":474,"context_line":"        # NOTE(gibi): It might seems wrong to default the progress to an empty"},{"line_number":475,"context_line":"        # string but this is how legacy work and this code only used by the"},{"line_number":476,"context_line":"        # legacy notification so try to keep the compatibility here but also"},{"line_number":477,"context_line":"        # keep it contained."},{"line_number":478,"context_line":"        progress\u003dint(instance.progress) if instance.progress else \u0027\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"7f231b9d_994d1b9c","line":475,"range":{"start_line":475,"start_character":40,"end_line":475,"end_character":44},"updated":"2017-06-15 13:48:28.000000000","message":"works","commit_id":"45ad7f3a2889b0234e8215cf0b2507a51542c00a"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"7e4e795e066460bf2c29cfb06acfde1f94a07822","unresolved":false,"context_lines":[{"line_number":473,"context_line":"        state_description\u003dnull_safe_str(instance.task_state),"},{"line_number":474,"context_line":"        # NOTE(gibi): It might seems wrong to default the progress to an empty"},{"line_number":475,"context_line":"        # string but this is how legacy work and this code only used by the"},{"line_number":476,"context_line":"        # legacy notification so try to keep the compatibility here but also"},{"line_number":477,"context_line":"        # keep it contained."},{"line_number":478,"context_line":"        progress\u003dint(instance.progress) if instance.progress else \u0027\u0027,"},{"line_number":479,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"7f231b9d_19610b06","line":476,"range":{"start_line":476,"start_character":45,"end_line":476,"end_character":48},"updated":"2017-06-15 13:48:28.000000000","message":"strike","commit_id":"45ad7f3a2889b0234e8215cf0b2507a51542c00a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"774c9bcf148336d0d680be8d03f5204ad14523ba","unresolved":false,"context_lines":[{"line_number":471,"context_line":"        # Status properties"},{"line_number":472,"context_line":"        state\u003dinstance.vm_state,"},{"line_number":473,"context_line":"        state_description\u003dnull_safe_str(instance.task_state),"},{"line_number":474,"context_line":"        # NOTE(gibi): It might seems wrong to default the progress to an empty"},{"line_number":475,"context_line":"        # string but this is how legacy work and this code only used by the"},{"line_number":476,"context_line":"        # legacy notification so try to keep the compatibility here but also"},{"line_number":477,"context_line":"        # keep it contained."},{"line_number":478,"context_line":"        progress\u003dint(instance.progress) if instance.progress else \u0027\u0027,"},{"line_number":479,"context_line":""},{"line_number":480,"context_line":"        # accessIPs"}],"source_content_type":"text/x-python","patch_set":6,"id":"7f231b9d_ef5cef4b","line":477,"range":{"start_line":474,"start_character":22,"end_line":477,"end_character":28},"updated":"2017-06-19 13:04:24.000000000","message":"Or:\n\n  We return \u0027\u0027 here for legacy reasons.\n\n? :)","commit_id":"45ad7f3a2889b0234e8215cf0b2507a51542c00a"}]}