)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"80b5679b2a675893ae783d570be1d53f7cc9744d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"7fff61d5_c10fc8f1","updated":"2024-10-31 09:30:39.000000000","message":"Thanks for the fix, just a small suggestion around compatibility with documented cluster label behaviour.","commit_id":"8dbf54c47a614129fee578e49a9c5c795e4f3bd1"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"e2ee1825f8b3b12afd1b5c5377e8e7387a6146ef","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"e2eaf07b_9f92aab5","updated":"2025-05-15 00:27:27.000000000","message":"FYI: I have a duplicate change in https://review.opendev.org/c/openstack/magnum-capi-helm/+/946857 - you can incorporate the test case from there and expand/adjust, or I can update this patchset to move this forward.","commit_id":"bfb82e1f6a796845e5cdfb2cdf95b5f0b19a7d00"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"dc0e883f7ae2739bd252c5cad17984af473f6151","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"ff91809c_9fbddcb9","updated":"2025-06-17 01:47:33.000000000","message":"Hi Piotr and reviewers,\n\nI have updated I48fda8cdb6e9de915528e47de40a771614c092d1 with changes from this patchset and made changes I think are necessary.\n\nPlease review there, and hopefully it replaces this (with co-author).","commit_id":"bfb82e1f6a796845e5cdfb2cdf95b5f0b19a7d00"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"568c3f40b0259b4da369bce5a6a752eba2a62556","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"11e8950b_ae64b4c3","updated":"2025-05-27 20:58:18.000000000","message":"Hi Piotr, I\u0027m keen to see this progress and merge.\n\nI\u0027ll look to update my patchset in a week or two with the details here and add you as a co-author, but equally happy to review your changes/response here if you will have the time soon.","commit_id":"bfb82e1f6a796845e5cdfb2cdf95b5f0b19a7d00"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"1181edc2f839518812cfc765526a9f4f54cc57a6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"3bf9d116_1f2b4a90","updated":"2025-05-14 23:51:30.000000000","message":"Label doesn\u0027t match documentation. Needs unit test added for negative case.","commit_id":"bfb82e1f6a796845e5cdfb2cdf95b5f0b19a7d00"},{"author":{"_account_id":22629,"name":"Michal Nasiadka","email":"mnasiadka@gmail.com","username":"mnasiadka"},"change_message_id":"a6da4fe28310db93d5d5b4b5a674b290fa567b26","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"d30f5ddb_94faf9b4","updated":"2024-12-12 13:56:19.000000000","message":"looks better, but I think some more unit tests would be nice to make sure it works (for both cases - floating ip label set or not)","commit_id":"bfb82e1f6a796845e5cdfb2cdf95b5f0b19a7d00"}],"magnum_capi_helm/driver.py":[{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"80b5679b2a675893ae783d570be1d53f7cc9744d","unresolved":true,"context_lines":[{"line_number":847,"context_line":"            \"etcd\": self._get_etcd_config(cluster),"},{"line_number":848,"context_line":"            \"apiServer\": {"},{"line_number":849,"context_line":"                \"associateFloatingIP\":"},{"line_number":850,"context_line":"                cluster.cluster_template.floating_ip_enabled,  # fmt: skip"},{"line_number":851,"context_line":"                \"enableLoadBalancer\": True,"},{"line_number":852,"context_line":"                \"loadBalancerProvider\": self._get_octavia_provider(cluster),"},{"line_number":853,"context_line":"            },"}],"source_content_type":"text/x-python","patch_set":7,"id":"d6b2dabb_dfc28d20","line":850,"updated":"2024-10-31 09:30:39.000000000","message":"I think we should allow this to be overwritten as false by the relevant cluster label even when the template value is true as described in [1], ideally using a separate helper function in the same way that loadBalancerProvider key is implemented below.\n\n[1] https://docs.openstack.org/magnum/latest/user/#master-lb-floating-ip-enabled","commit_id":"8dbf54c47a614129fee578e49a9c5c795e4f3bd1"},{"author":{"_account_id":32657,"name":"Piotr Parczewski","email":"piotr@stackhpc.com","username":"piotrp"},"change_message_id":"3fc4cd92b31781768fa9d6732955075ad1b57757","unresolved":true,"context_lines":[{"line_number":847,"context_line":"            \"etcd\": self._get_etcd_config(cluster),"},{"line_number":848,"context_line":"            \"apiServer\": {"},{"line_number":849,"context_line":"                \"associateFloatingIP\":"},{"line_number":850,"context_line":"                cluster.cluster_template.floating_ip_enabled,  # fmt: skip"},{"line_number":851,"context_line":"                \"enableLoadBalancer\": True,"},{"line_number":852,"context_line":"                \"loadBalancerProvider\": self._get_octavia_provider(cluster),"},{"line_number":853,"context_line":"            },"}],"source_content_type":"text/x-python","patch_set":7,"id":"e1f72f40_54640cd1","line":850,"in_reply_to":"d6b2dabb_dfc28d20","updated":"2024-11-05 07:44:45.000000000","message":"Ack, fair point","commit_id":"8dbf54c47a614129fee578e49a9c5c795e4f3bd1"},{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"208544bdab7739cfc966ec54c94a607df22bffe0","unresolved":false,"context_lines":[{"line_number":847,"context_line":"            \"etcd\": self._get_etcd_config(cluster),"},{"line_number":848,"context_line":"            \"apiServer\": {"},{"line_number":849,"context_line":"                \"associateFloatingIP\":"},{"line_number":850,"context_line":"                cluster.cluster_template.floating_ip_enabled,  # fmt: skip"},{"line_number":851,"context_line":"                \"enableLoadBalancer\": True,"},{"line_number":852,"context_line":"                \"loadBalancerProvider\": self._get_octavia_provider(cluster),"},{"line_number":853,"context_line":"            },"}],"source_content_type":"text/x-python","patch_set":7,"id":"384496d5_47fc1bb3","line":850,"in_reply_to":"e1f72f40_54640cd1","updated":"2025-01-06 09:28:40.000000000","message":"Done","commit_id":"8dbf54c47a614129fee578e49a9c5c795e4f3bd1"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"e284fe8ef426ca9da23bdd30c26aa06db96d15ba","unresolved":true,"context_lines":[{"line_number":738,"context_line":"            CONF.capi_helm.csi_cinder_allow_volume_expansion,"},{"line_number":739,"context_line":"        )"},{"line_number":740,"context_line":""},{"line_number":741,"context_line":"    def _get_floating_ip_enabled(self, cluster):"},{"line_number":742,"context_line":"        return ("},{"line_number":743,"context_line":"            self._get_label_bool(cluster, \"master_lb_floating_enabled\", None)"},{"line_number":744,"context_line":"            or cluster.cluster_template.floating_ip_enabled"}],"source_content_type":"text/x-python","patch_set":9,"id":"05a968c8_8f62503b","line":741,"updated":"2025-05-14 23:57:58.000000000","message":"This function name could be more specific. There are two \"floating ip enabled\" settings, even if not implemented in this driver.\n\n1) `cluster_template.floating_ip_enabled` is for adding a floating ip to all control plane and worker nodes. This is not implemented in CAPI Helm, and I don\u0027t see a use case for it. However, that is the documentation and design.\n2) Label `master_lb_floating_ip_enabled` is whether to add a floating IP to the API Loadbalancer. This is documented as defaulting to the same value as the above. (I think this should change, but that is beyond this patchset).\n\nReference: https://review.opendev.org/c/openstack/magnum/+/641547","commit_id":"bfb82e1f6a796845e5cdfb2cdf95b5f0b19a7d00"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"1181edc2f839518812cfc765526a9f4f54cc57a6","unresolved":true,"context_lines":[{"line_number":740,"context_line":""},{"line_number":741,"context_line":"    def _get_floating_ip_enabled(self, cluster):"},{"line_number":742,"context_line":"        return ("},{"line_number":743,"context_line":"            self._get_label_bool(cluster, \"master_lb_floating_enabled\", None)"},{"line_number":744,"context_line":"            or cluster.cluster_template.floating_ip_enabled"},{"line_number":745,"context_line":"        )"},{"line_number":746,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"ae4363e5_c31b368e","line":743,"updated":"2025-05-14 23:51:30.000000000","message":"this label should be `master_lb_floating_ip_enabled`, not `master_lb_floating_enabled`\n\nSee https://docs.openstack.org/magnum/latest/user/#master-lb-floating-ip-enabled","commit_id":"bfb82e1f6a796845e5cdfb2cdf95b5f0b19a7d00"}]}