)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"55f502faf2d94091226d223edcbbeeb8bde464dd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"c160b32e_5c6989aa","updated":"2024-11-04 15:10:03.000000000","message":"soft -1 for a few reasons.\n\n1.) this is fixing a bug but we have not filed a launchpad bug for this and linked it in the commit message with Closes-Bug: #xyz\n\n\n2.) there is no test change to cover the old or new behaviour.\nnow the plugin has very little test coverage in general but it seams like\nit should be possible to test calling format_global_efficacy.\n\nhttps://github.com/openstack/watcher-dashboard/blob/master/watcher_dashboard/test/test_formset_table.py\n\nmight be an example of how to write such a test.\n\n\nif the existing cores are happy to merge this as is i think the code change makes sense.\n\nif i was a core fo this repo i would at least like to have a conversation about the expected minimum set of testing for this repo.\n\ni do not have experiance reviewing horizon plugins so this might be the norm in such projects in which case i can update my review to +1. as a core reviewer of other service/lib projects i would hope for at least a bug link to track the issue.\n\ni considered asking for a release not but whiel the infra exist i see it not really used so that while nice to have is not required i think for merging.","commit_id":"ae7746aae283a33157005c7df4ca124860b2a7d9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a265c1ca9fd8bf976d69e893c9a8565bf11e7818","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"5e90cff8_b0bea6c6","updated":"2024-11-04 15:59:02.000000000","message":"Based on my manual testing of this patch i can confirm the orginal bug and that this corrects the problem. https://imgur.com/a/hUlruos\n\ngiven the commit has also been updated with the bug fix link i am upgrding my review to +1","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"3aec63a9847fc0ed4e19639c7c6796c2fd93597d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"ff64d329_580d2f35","updated":"2024-12-11 11:28:31.000000000","message":"LGTM","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"80d3c3a4d42430fcd536a0c8ff981f7fb8999a27","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"4981731d_8506121f","updated":"2024-11-06 08:03:19.000000000","message":"i am not sure about it Alfredo and I don\u0027t want to block on that. perhaps we can merge and re-visit? maybe add relevant comment in the code?","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5904e408588cf9115510fa2d43850ad7a67dc344","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"3fae0b38_83761fbe","updated":"2024-12-03 01:09:48.000000000","message":"upgrading to +2","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"}],"watcher_dashboard/api/watcher.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"55f502faf2d94091226d223edcbbeeb8bde464dd","unresolved":true,"context_lines":[{"line_number":517,"context_line":"        self.value \u003d indicator.get(\u0027value\u0027, None)"},{"line_number":518,"context_line":"        self.name \u003d indicator.get(\u0027name\u0027, None)"},{"line_number":519,"context_line":"        self.description \u003d indicator.get(\u0027description\u0027, None)"},{"line_number":520,"context_line":"        self.unit \u003d indicator.get(\u0027unit\u0027, None)"}],"source_content_type":"text/x-python","patch_set":2,"id":"885fb4ae_853a765b","line":520,"updated":"2024-11-04 15:10:03.000000000","message":"ack so this is the revert to using the correct accesor on indicator.\n\nas this is not a keyword paramter we woudl expect the indication to alwasy be not equal to None","commit_id":"ae7746aae283a33157005c7df4ca124860b2a7d9"}],"watcher_dashboard/content/action_plans/tables.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"55f502faf2d94091226d223edcbbeeb8bde464dd","unresolved":true,"context_lines":[{"line_number":130,"context_line":"    if len(action_plan.global_efficacy) \u003e 0:"},{"line_number":131,"context_line":"        global_efficacy_dict \u003d action_plan.global_efficacy[0]"},{"line_number":132,"context_line":"    else:"},{"line_number":133,"context_line":"        global_efficacy_dict \u003d {}"},{"line_number":134,"context_line":"    global_efficacy \u003d watcher.EfficacyIndicator(global_efficacy_dict)"},{"line_number":135,"context_line":"    if global_efficacy.value is not None and global_efficacy.unit:"},{"line_number":136,"context_line":"        formatted_global_efficacy \u003d \"%(value)s %(unit)s\" % dict("}],"source_content_type":"text/x-python","patch_set":2,"id":"fc1cca5f_06a2b670","line":133,"updated":"2024-11-04 15:10:03.000000000","message":"and this fixes the orginal bug that\nhttps://review.opendev.org/c/openstack/watcher-dashboard/+/587032\nwas tryign to adress by correctly defaulting it to {} when not set.","commit_id":"ae7746aae283a33157005c7df4ca124860b2a7d9"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"2b297ab0977fe58e1d7ee2de3f34ce8f370d9a4f","unresolved":true,"context_lines":[{"line_number":133,"context_line":"        global_efficacy_dict \u003d {}"},{"line_number":134,"context_line":"    global_efficacy \u003d watcher.EfficacyIndicator(global_efficacy_dict)"},{"line_number":135,"context_line":"    if global_efficacy.value is not None and global_efficacy.unit:"},{"line_number":136,"context_line":"        formatted_global_efficacy \u003d \"%(value)s %(unit)s\" % dict("},{"line_number":137,"context_line":"            unit\u003dglobal_efficacy.unit,"},{"line_number":138,"context_line":"            value\u003dglobal_efficacy.value)"},{"line_number":139,"context_line":"    elif global_efficacy.value is not None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7a2176ab_87f0d56f","line":136,"updated":"2024-11-05 07:42:09.000000000","message":"do we need to worry about/guard the case of action_plan.global_efficacy having more than one dict?  i.e. global_efficacy[1] or more ?","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"f9083fc4df0b1815edcec5dbfee14d502a27461f","unresolved":false,"context_lines":[{"line_number":133,"context_line":"        global_efficacy_dict \u003d {}"},{"line_number":134,"context_line":"    global_efficacy \u003d watcher.EfficacyIndicator(global_efficacy_dict)"},{"line_number":135,"context_line":"    if global_efficacy.value is not None and global_efficacy.unit:"},{"line_number":136,"context_line":"        formatted_global_efficacy \u003d \"%(value)s %(unit)s\" % dict("},{"line_number":137,"context_line":"            unit\u003dglobal_efficacy.unit,"},{"line_number":138,"context_line":"            value\u003dglobal_efficacy.value)"},{"line_number":139,"context_line":"    elif global_efficacy.value is not None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"acc78eb5_a432e500","line":136,"in_reply_to":"7a2176ab_87f0d56f","updated":"2024-11-05 09:29:51.000000000","message":"My understanding of the API is that that global_efficacy is always a single value while efficacy indicators can be more that one. https://docs.openstack.org/watcher/latest/glossary.html#solution","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"e46ae2485fbd04ca2f0665a990dac38953a90301","unresolved":true,"context_lines":[{"line_number":133,"context_line":"        global_efficacy_dict \u003d {}"},{"line_number":134,"context_line":"    global_efficacy \u003d watcher.EfficacyIndicator(global_efficacy_dict)"},{"line_number":135,"context_line":"    if global_efficacy.value is not None and global_efficacy.unit:"},{"line_number":136,"context_line":"        formatted_global_efficacy \u003d \"%(value)s %(unit)s\" % dict("},{"line_number":137,"context_line":"            unit\u003dglobal_efficacy.unit,"},{"line_number":138,"context_line":"            value\u003dglobal_efficacy.value)"},{"line_number":139,"context_line":"    elif global_efficacy.value is not None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"a22b93ff_fd461d69","line":136,"in_reply_to":"80c40017_20d6fa8e","updated":"2024-11-13 17:53:05.000000000","message":"Thanks! I think you are correct, looking at existing code, apparently there is an existing goal, HardwareMaintenance which is already returning a of indicators as global_efficacy_indicator:\n\nhttps://github.com/openstack/watcher/blob/master/watcher/decision_engine/goal/efficacy/specs.py#L98-L164\n\nI have a solution to manage multivalued global_efficiency but I\u0027d say it\u0027s better to send it as a patch on top of this one as it seems as a different fix/feature, imo.","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"18db475d6091c45c8bbd40cc1929ad61f745b4c3","unresolved":false,"context_lines":[{"line_number":133,"context_line":"        global_efficacy_dict \u003d {}"},{"line_number":134,"context_line":"    global_efficacy \u003d watcher.EfficacyIndicator(global_efficacy_dict)"},{"line_number":135,"context_line":"    if global_efficacy.value is not None and global_efficacy.unit:"},{"line_number":136,"context_line":"        formatted_global_efficacy \u003d \"%(value)s %(unit)s\" % dict("},{"line_number":137,"context_line":"            unit\u003dglobal_efficacy.unit,"},{"line_number":138,"context_line":"            value\u003dglobal_efficacy.value)"},{"line_number":139,"context_line":"    elif global_efficacy.value is not None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"64ce6e5b_0697a033","line":136,"in_reply_to":"84a37791_842ea202","updated":"2024-11-18 07:01:34.000000000","message":"ACK thanks marking this comment resolved then","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"d83f1f462a7bf27f81d79ae3682f7ca9a724bd02","unresolved":true,"context_lines":[{"line_number":133,"context_line":"        global_efficacy_dict \u003d {}"},{"line_number":134,"context_line":"    global_efficacy \u003d watcher.EfficacyIndicator(global_efficacy_dict)"},{"line_number":135,"context_line":"    if global_efficacy.value is not None and global_efficacy.unit:"},{"line_number":136,"context_line":"        formatted_global_efficacy \u003d \"%(value)s %(unit)s\" % dict("},{"line_number":137,"context_line":"            unit\u003dglobal_efficacy.unit,"},{"line_number":138,"context_line":"            value\u003dglobal_efficacy.value)"},{"line_number":139,"context_line":"    elif global_efficacy.value is not None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"84a37791_842ea202","line":136,"in_reply_to":"a22b93ff_fd461d69","updated":"2024-11-14 08:31:18.000000000","message":"https://review.opendev.org/c/openstack/watcher-dashboard/+/935043","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"80d3c3a4d42430fcd536a0c8ff981f7fb8999a27","unresolved":false,"context_lines":[{"line_number":133,"context_line":"        global_efficacy_dict \u003d {}"},{"line_number":134,"context_line":"    global_efficacy \u003d watcher.EfficacyIndicator(global_efficacy_dict)"},{"line_number":135,"context_line":"    if global_efficacy.value is not None and global_efficacy.unit:"},{"line_number":136,"context_line":"        formatted_global_efficacy \u003d \"%(value)s %(unit)s\" % dict("},{"line_number":137,"context_line":"            unit\u003dglobal_efficacy.unit,"},{"line_number":138,"context_line":"            value\u003dglobal_efficacy.value)"},{"line_number":139,"context_line":"    elif global_efficacy.value is not None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"80c40017_20d6fa8e","line":136,"in_reply_to":"acc78eb5_a432e500","updated":"2024-11-06 08:03:19.000000000","message":"i am not sure but I couldn\u0027t tell from scanning the doc \n\ni did some digging and it really does suggest there could be a list of indicators in there...\n\n\nfor example here https://github.com/openstack/watcher/blob/b5e45b43b92f4239ade849328e0ec47eef1d8b22/watcher/decision_engine/solution/efficacy.py#L93-L100\n\n```\n                indicators.append(\n                    Indicator(\n                        name\u003drelated_indicator_spec.name,\n                        description\u003drelated_indicator_spec.description,\n                        unit\u003drelated_indicator_spec.unit,\n                        value\u003dvalue))\n\n\n            self.indicators \u003d indicators\n\n```\n\n\nalso grepping through tests, it looks like they are accessing different elements of the global_efficacy:\n\n\n./watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py:349:        global_efficacy_value \u003d solution.global_efficacy[0].get(\u0027value\u0027, 0)\n\n\n./watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py:399:        global_efficacy_value \u003d solution.global_efficacy[2].get(\u0027value\u0027, 0)\n\n\n./watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py:324:        global_efficacy_value \u003d solution.global_efficacy[3].get(\u0027value\u0027, 0)","commit_id":"42d8914dcfc404f76e8e6e67dfb918b845ab9316"}]}