)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":6737,"name":"Edward Hope-Morley","email":"edward.hope-morley@canonical.com","username":"hopem"},"change_message_id":"c232b46d7154cf4564b6dc2d985cd36f4d094e65","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"230266af_d529ee13","updated":"2023-09-22 12:45:02.000000000","message":"For an app called \"nova-compute-special\" you are doing:\n\nceph_joined():\n  send_application_name(app_name\u003d\u0027nova-compute-special\u0027)\n\nceph_changed():\n  ensure_ceph_keyring(service\u003d\u0027nova-compute-special\u0027)\n  create_libvirt_secret(CEPH_OLD_SECRET_UUID)\n  if _handle_ceph_request():\n    send_application_name(app_name\u003d\u0027nova-compute\u0027)\n\nceph_changed():\n  ensure_ceph_keyring(service\u003d\u0027nova-compute\u0027)\n  create_libvirt_secret(CEPH_SECRET_UUID)\n\n\nSo basically you are gating whether or not to create the new \u0027nova-compute\u0027 keyring on whether or not broker requests succeeded but those requests are orthogonal to your keyring setup so I dont see why we need to gate on it. You could therefore do:\n\nceph_joined():\n  send_application_name(app_name\u003d\u0027nova-compute-special\u0027)\n\nceph_changed():\n  ensure_ceph_keyring(service\u003d\u0027nova-compute-special\u0027)\n  create_libvirt_secret(CEPH_OLD_SECRET_UUID)\n  send_application_name(app_name\u003d\u0027nova-compute\u0027)\n\nceph_changed():\n  ensure_ceph_keyring(service\u003d\u0027nova-compute\u0027)\n  create_libvirt_secret(CEPH_SECRET_UUID)","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"},{"author":{"_account_id":14567,"name":"Rodrigo Barbieri","email":"rodrigo.barbieri2010@gmail.com","username":"ganso"},"change_message_id":"bf28f9fde0ec70e4932aa61939a8b878a720e51e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"3a2f96ae_70563332","in_reply_to":"230266af_d529ee13","updated":"2023-09-22 12:54:24.000000000","message":"oh I see what you mean now. I thought you wanted to get rid of the entire if/else that chooses between the old and new path (therefore also choosing between which secret to use). Sure we can move the send_application_name(new) to unlink it from the ceph broker","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"},{"author":{"_account_id":6737,"name":"Edward Hope-Morley","email":"edward.hope-morley@canonical.com","username":"hopem"},"change_message_id":"d01e96cf0cc723be4047a4c78d410934d3a6b561","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"c1f8ba24_73a51f19","updated":"2023-09-25 12:38:06.000000000","message":"I think this looks good now. Lets put it through some decent testing. I see that your commit message says pending func tests so hopefully we can update those to include a scenario where we have e.g. both a \"nova-compute\" app and a \"nova-compute-alt\" app.","commit_id":"1128e26643e6cfdabf0df3c278f5f514956f4a6f"}],"hooks/nova_compute_context.py":[{"author":{"_account_id":6737,"name":"Edward Hope-Morley","email":"edward.hope-morley@canonical.com","username":"hopem"},"change_message_id":"d01e96cf0cc723be4047a4c78d410934d3a6b561","unresolved":true,"context_lines":[{"line_number":65,"context_line":"# This is just a label and it must be consistent across"},{"line_number":66,"context_line":"# nova-compute nodes to support live migration."},{"line_number":67,"context_line":"CEPH_SECRET_UUID \u003d \u0027c91ce26f-403d-4058-9c38-6b56e1c428e0\u0027"},{"line_number":68,"context_line":"CEPH_OLD_SECRET_UUID \u003d \u0027514c9fca-8cbe-11e2-9c52-3bc8c7819472\u0027"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"OVS_BRIDGE \u003d \u0027br-int\u0027"},{"line_number":71,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"00344d89_3f41f783","line":68,"range":{"start_line":68,"start_character":0,"end_line":68,"end_character":61},"updated":"2023-09-25 12:38:06.000000000","message":"maybe add a short comment to explain why we are maintaining an \"old\" secret. even if it like \"# See LP 2037003 for reason why we have this\"","commit_id":"1128e26643e6cfdabf0df3c278f5f514956f4a6f"}],"hooks/nova_compute_hooks.py":[{"author":{"_account_id":6737,"name":"Edward Hope-Morley","email":"edward.hope-morley@canonical.com","username":"hopem"},"change_message_id":"6ce5518d4d8f645fd7b9a1bc155e8ca35dfe159d","unresolved":true,"context_lines":[{"line_number":570,"context_line":"                      rid\u003drid, unit\u003dunit)"},{"line_number":571,"context_line":"    else:"},{"line_number":572,"context_line":"        if _ceph_changed(service_name(), CEPH_OLD_SECRET_UUID,"},{"line_number":573,"context_line":"                         rid\u003drid, unit\u003dunit) is True:"},{"line_number":574,"context_line":"            log(\u0027Sending application name for new ceph credentials after \u0027"},{"line_number":575,"context_line":"                \u0027setting up the old credentials\u0027)"},{"line_number":576,"context_line":"            send_application_name(app_name\u003d\u0027nova-compute\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"91e0762a_da73e4b6","line":573,"range":{"start_line":573,"start_character":0,"end_line":573,"end_character":53},"updated":"2023-09-22 12:19:13.000000000","message":"i understand why you are making this conditional but I think it is unnecessary i.e. lets do both calls regardless and if _handle_ceph_request() fails there will be a log msg to check. I left a comment below to bump the log message to WARNING. Also, we could do a status_set() to say something failed.","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"},{"author":{"_account_id":14567,"name":"Rodrigo Barbieri","email":"rodrigo.barbieri2010@gmail.com","username":"ganso"},"change_message_id":"bfce838f0d20fd1327abb10dd30a4222f134f792","unresolved":true,"context_lines":[{"line_number":570,"context_line":"                      rid\u003drid, unit\u003dunit)"},{"line_number":571,"context_line":"    else:"},{"line_number":572,"context_line":"        if _ceph_changed(service_name(), CEPH_OLD_SECRET_UUID,"},{"line_number":573,"context_line":"                         rid\u003drid, unit\u003dunit) is True:"},{"line_number":574,"context_line":"            log(\u0027Sending application name for new ceph credentials after \u0027"},{"line_number":575,"context_line":"                \u0027setting up the old credentials\u0027)"},{"line_number":576,"context_line":"            send_application_name(app_name\u003d\u0027nova-compute\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9c328402_37fa45fb","line":573,"range":{"start_line":573,"start_character":0,"end_line":573,"end_character":53},"in_reply_to":"91e0762a_da73e4b6","updated":"2023-09-22 12:24:37.000000000","message":"we cannot do both at once. Only one can be done depending on what is in the relation-data.","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"},{"author":{"_account_id":6737,"name":"Edward Hope-Morley","email":"edward.hope-morley@canonical.com","username":"hopem"},"change_message_id":"6ce5518d4d8f645fd7b9a1bc155e8ca35dfe159d","unresolved":true,"context_lines":[{"line_number":579,"context_line":"def _ceph_changed(svc_name, secret_uuid, rid\u003dNone, unit\u003dNone):"},{"line_number":580,"context_line":"    if \u0027ceph\u0027 not in CONFIGS.complete_contexts():"},{"line_number":581,"context_line":"        log(\u0027ceph relation incomplete. Peer not ready?\u0027)"},{"line_number":582,"context_line":"        return"},{"line_number":583,"context_line":"    if not ensure_ceph_keyring(service\u003dsvc_name, user\u003d\u0027nova\u0027, group\u003d\u0027nova\u0027):"},{"line_number":584,"context_line":"        log(\u0027Could not create ceph keyring \u0027"},{"line_number":585,"context_line":"            \u0027for {}: peer not ready?\u0027.format(svc_name))"}],"source_content_type":"text/x-python","patch_set":1,"id":"0b2ebfec_fe3838e5","line":582,"range":{"start_line":582,"start_character":0,"end_line":582,"end_character":14},"updated":"2023-09-22 12:19:13.000000000","message":"nit: keep the original newline here","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"},{"author":{"_account_id":14567,"name":"Rodrigo Barbieri","email":"rodrigo.barbieri2010@gmail.com","username":"ganso"},"change_message_id":"bfce838f0d20fd1327abb10dd30a4222f134f792","unresolved":true,"context_lines":[{"line_number":579,"context_line":"def _ceph_changed(svc_name, secret_uuid, rid\u003dNone, unit\u003dNone):"},{"line_number":580,"context_line":"    if \u0027ceph\u0027 not in CONFIGS.complete_contexts():"},{"line_number":581,"context_line":"        log(\u0027ceph relation incomplete. Peer not ready?\u0027)"},{"line_number":582,"context_line":"        return"},{"line_number":583,"context_line":"    if not ensure_ceph_keyring(service\u003dsvc_name, user\u003d\u0027nova\u0027, group\u003d\u0027nova\u0027):"},{"line_number":584,"context_line":"        log(\u0027Could not create ceph keyring \u0027"},{"line_number":585,"context_line":"            \u0027for {}: peer not ready?\u0027.format(svc_name))"}],"source_content_type":"text/x-python","patch_set":1,"id":"b73aff85_aa498770","line":582,"range":{"start_line":582,"start_character":0,"end_line":582,"end_character":14},"in_reply_to":"0b2ebfec_fe3838e5","updated":"2023-09-22 12:24:37.000000000","message":"I am not sure I see what\u0027s wrong. I deleted the whitespace and hit enter again, let\u0027s see if it addresses it.","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"},{"author":{"_account_id":6737,"name":"Edward Hope-Morley","email":"edward.hope-morley@canonical.com","username":"hopem"},"change_message_id":"6ce5518d4d8f645fd7b9a1bc155e8ca35dfe159d","unresolved":true,"context_lines":[{"line_number":598,"context_line":""},{"line_number":599,"context_line":"    try:"},{"line_number":600,"context_line":"        _handle_ceph_request()"},{"line_number":601,"context_line":"        return True"},{"line_number":602,"context_line":"    except ValueError as e:"},{"line_number":603,"context_line":"        # The end user has most likely provided a invalid value for a"},{"line_number":604,"context_line":"        # configuration option. Just log the traceback here, the end"}],"source_content_type":"text/x-python","patch_set":1,"id":"8bfd0e77_28ff5649","line":601,"range":{"start_line":601,"start_character":0,"end_line":601,"end_character":19},"updated":"2023-09-22 12:19:13.000000000","message":"for the reason stated above i think this is unnecessary.","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"},{"author":{"_account_id":14567,"name":"Rodrigo Barbieri","email":"rodrigo.barbieri2010@gmail.com","username":"ganso"},"change_message_id":"bfce838f0d20fd1327abb10dd30a4222f134f792","unresolved":true,"context_lines":[{"line_number":598,"context_line":""},{"line_number":599,"context_line":"    try:"},{"line_number":600,"context_line":"        _handle_ceph_request()"},{"line_number":601,"context_line":"        return True"},{"line_number":602,"context_line":"    except ValueError as e:"},{"line_number":603,"context_line":"        # The end user has most likely provided a invalid value for a"},{"line_number":604,"context_line":"        # configuration option. Just log the traceback here, the end"}],"source_content_type":"text/x-python","patch_set":1,"id":"f3c3e7f5_5dfd391d","line":601,"range":{"start_line":601,"start_character":0,"end_line":601,"end_character":19},"in_reply_to":"8bfd0e77_28ff5649","updated":"2023-09-22 12:24:37.000000000","message":"right, but we cannot do both at once.","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"},{"author":{"_account_id":6737,"name":"Edward Hope-Morley","email":"edward.hope-morley@canonical.com","username":"hopem"},"change_message_id":"6ce5518d4d8f645fd7b9a1bc155e8ca35dfe159d","unresolved":true,"context_lines":[{"line_number":606,"context_line":"        # the hook execution."},{"line_number":607,"context_line":"        log(\u0027Caught ValueError, invalid value provided for \u0027"},{"line_number":608,"context_line":"            \u0027configuration?: \"{}\"\u0027.format(str(e)),"},{"line_number":609,"context_line":"            level\u003dDEBUG)"},{"line_number":610,"context_line":""},{"line_number":611,"context_line":""},{"line_number":612,"context_line":"# TODO: Refactor this method moving part of this logic to charmhelpers,"}],"source_content_type":"text/x-python","patch_set":1,"id":"06740026_39299a95","line":609,"range":{"start_line":609,"start_character":0,"end_line":609,"end_character":24},"updated":"2023-09-22 12:19:13.000000000","message":"this should be level\u003dWARNING","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"},{"author":{"_account_id":14567,"name":"Rodrigo Barbieri","email":"rodrigo.barbieri2010@gmail.com","username":"ganso"},"change_message_id":"bfce838f0d20fd1327abb10dd30a4222f134f792","unresolved":true,"context_lines":[{"line_number":606,"context_line":"        # the hook execution."},{"line_number":607,"context_line":"        log(\u0027Caught ValueError, invalid value provided for \u0027"},{"line_number":608,"context_line":"            \u0027configuration?: \"{}\"\u0027.format(str(e)),"},{"line_number":609,"context_line":"            level\u003dDEBUG)"},{"line_number":610,"context_line":""},{"line_number":611,"context_line":""},{"line_number":612,"context_line":"# TODO: Refactor this method moving part of this logic to charmhelpers,"}],"source_content_type":"text/x-python","patch_set":1,"id":"916e0bae_b7fc2aa5","line":609,"range":{"start_line":609,"start_character":0,"end_line":609,"end_character":24},"in_reply_to":"06740026_39299a95","updated":"2023-09-22 12:24:37.000000000","message":"unrelated to the change in question, but sure","commit_id":"1d99b70be787438e293c60769b867bb50ac31845"}]}