)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":34150,"name":"Simon Hensel","email":"simon.hensel@inovex.de","username":"shensel"},"change_message_id":"eaf8c9741443e6a3bfc507278da42d9e2ecf4527","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"f3a1b89c_eba0b270","updated":"2024-09-23 11:12:34.000000000","message":"Any chance to get another review on this patch?","commit_id":"b9ae35acde47b18f79dd33816d9331b51a1cc960"},{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"7535214f0ac4160914b173e13eb9ff5ab18f7b8d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"eea12c7b_6bcfafa5","updated":"2024-10-09 20:53:21.000000000","message":"@simon.hensel@inovex.de\nPlease add test for this functionality in ci/roles/subnet_pool/ ci/roles/subnet/","commit_id":"65795d6a0f7e9c5c1ec7d9e598f6c9cbf3b03f9a"},{"author":{"_account_id":34150,"name":"Simon Hensel","email":"simon.hensel@inovex.de","username":"shensel"},"change_message_id":"42270217fd3a1f9f0a5035fef685c57bd6acf75b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"1d0c5358_d078f325","updated":"2024-10-15 12:14:28.000000000","message":"I did some testing locally and got mixed results, this looks like a race condition to me.\n\nThe issue comes from the task that creates the (exact same) subnet a second time, which should always return \"changed: false\", but it only sometimes does.\nWhen comparing the returned dictionaries for both subnet creation tasks, all values are the same, except \"revision_number\" and \"updated_at\".\n\nWhen adding a short delay between both tasks with ansible.builtin.pause, the test always completes correctly.\nIt seems as if the first subnet creation is not fully completed before it gets created a second time, hence the return value contains \"changed: true\".\n\nSo could adding a pause to the test be a solution to this problem?","commit_id":"dccdafd110b93596f123afc12b6628da0c0f8667"},{"author":{"_account_id":28619,"name":"Dmitriy Rabotyagov","email":"noonedeadpunk@gmail.com","username":"noonedeadpunk"},"change_message_id":"b5b843fba0e6e99b5551f0efb82fa5d7ebcb58f4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"4f213bcd_5a6beb7b","updated":"2024-10-11 07:21:53.000000000","message":"Seems like a valid failure here, related to the change:\n```\n\nTASK [subnet : Verify Subnet Allocation Pools Exist] ***************************\ntask path: /home/zuul/src/opendev.org/openstack/ansible-collections-openstack/ci/roles/subnet/tasks/subnet-allocation.yml:122\nfatal: [localhost]: FAILED! \u003d\u003e {\n    \"assertion\": \"idem2 is not changed\",\n    \"changed\": false,\n    \"evaluated_to\": false,\n    \"msg\": \"Assertion failed\"\n}\n```","commit_id":"dccdafd110b93596f123afc12b6628da0c0f8667"},{"author":{"_account_id":34150,"name":"Simon Hensel","email":"simon.hensel@inovex.de","username":"shensel"},"change_message_id":"2a10c87c0ea3edc781f7a0353b9192f321236e49","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"8d8f44d8_33417faa","updated":"2024-10-10 12:36:49.000000000","message":"recheck","commit_id":"dccdafd110b93596f123afc12b6628da0c0f8667"},{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"50e5a31d3cdb92ed373044eee7c881f8940dfdc6","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"b1d8bd05_2837d091","in_reply_to":"1d0c5358_d078f325","updated":"2024-10-17 15:14:49.000000000","message":"Yeah, you can add \"pause\"","commit_id":"dccdafd110b93596f123afc12b6628da0c0f8667"},{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"b10a545d37317daa9b63809ef68e1d2c068f0e7d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"da57295d_6f6f48d2","updated":"2024-11-05 10:50:11.000000000","message":"Thanks!","commit_id":"28541ee158deb3b11e2bfd06277bf0e5451b34d2"}],"ci/roles/subnet/tasks/subnet-allocation.yml":[{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"aca79fb7ce2cb4fa4cf582f2b38bc78d54b4a6ed","unresolved":true,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"- name: Sort Subnet Allocation Pools"},{"line_number":123,"context_line":"  ansible.builtin.set_fact:"},{"line_number":124,"context_line":"    allocation_pools: \"{{ subnet_result.subnets[0].allocation_pools | sort(attribute\u003d\u0027start\u0027, reverse\u003dTrue) }}\""},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"- name: Verify Subnet Allocation Pools Exist"},{"line_number":127,"context_line":"  assert:"},{"line_number":128,"context_line":"    that:"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"d2b7db46_01a035bd","line":125,"range":{"start_line":124,"start_character":3,"end_line":125,"end_character":1},"updated":"2024-10-22 12:39:48.000000000","message":"Reviewed yet before this change, but it seems like random here, it should sort without reverse\u003dtrue. I\u0027d go with \"OR\"s instead of relying on non-working sorting.","commit_id":"d994a40e5fce2ff5b055d046d304f45f239e0e73"},{"author":{"_account_id":34150,"name":"Simon Hensel","email":"simon.hensel@inovex.de","username":"shensel"},"change_message_id":"0c173553cc19e4394ec80f521377afb33df4298a","unresolved":false,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"- name: Sort Subnet Allocation Pools"},{"line_number":123,"context_line":"  ansible.builtin.set_fact:"},{"line_number":124,"context_line":"    allocation_pools: \"{{ subnet_result.subnets[0].allocation_pools | sort(attribute\u003d\u0027start\u0027, reverse\u003dTrue) }}\""},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"- name: Verify Subnet Allocation Pools Exist"},{"line_number":127,"context_line":"  assert:"},{"line_number":128,"context_line":"    that:"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"7c3740cc_23512a18","line":125,"range":{"start_line":124,"start_character":3,"end_line":125,"end_character":1},"in_reply_to":"0be55ea5_abe2ede0","updated":"2024-11-05 09:18:26.000000000","message":"The assertions are now updated to use OR instead of sorting.\nThanks for reviewing and helping fix the issue!","commit_id":"d994a40e5fce2ff5b055d046d304f45f239e0e73"},{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"8cffd1331ad7376bd6189325282bd712ce3876ed","unresolved":true,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"- name: Sort Subnet Allocation Pools"},{"line_number":123,"context_line":"  ansible.builtin.set_fact:"},{"line_number":124,"context_line":"    allocation_pools: \"{{ subnet_result.subnets[0].allocation_pools | sort(attribute\u003d\u0027start\u0027, reverse\u003dTrue) }}\""},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"- name: Verify Subnet Allocation Pools Exist"},{"line_number":127,"context_line":"  assert:"},{"line_number":128,"context_line":"    that:"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"0be55ea5_abe2ede0","line":125,"range":{"start_line":124,"start_character":3,"end_line":125,"end_character":1},"in_reply_to":"ac18589a_1b2e93bd","updated":"2024-10-31 15:03:03.000000000","message":"Ack, missed this. Thanks for clarifying. I\u0027d prefer to have OR here anyway, I don\u0027t want to let any option to make this test flaky, It\u0027s already fragile CI here. Can you do it please?","commit_id":"d994a40e5fce2ff5b055d046d304f45f239e0e73"},{"author":{"_account_id":34150,"name":"Simon Hensel","email":"simon.hensel@inovex.de","username":"shensel"},"change_message_id":"2eef0cd218aa4541a56ffe0bafbfe89a0b41c12f","unresolved":true,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"- name: Sort Subnet Allocation Pools"},{"line_number":123,"context_line":"  ansible.builtin.set_fact:"},{"line_number":124,"context_line":"    allocation_pools: \"{{ subnet_result.subnets[0].allocation_pools | sort(attribute\u003d\u0027start\u0027, reverse\u003dTrue) }}\""},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"- name: Verify Subnet Allocation Pools Exist"},{"line_number":127,"context_line":"  assert:"},{"line_number":128,"context_line":"    that:"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"ac18589a_1b2e93bd","line":125,"range":{"start_line":124,"start_character":3,"end_line":125,"end_character":1},"in_reply_to":"d2b7db46_01a035bd","updated":"2024-10-22 12:54:00.000000000","message":"When sorting without reverse\u003dTrue, Python sorts the IP addresses lexicographic order, which means 192.168.0.10 is sorted before 192.168.0.2, which in itself is not an issue, but then the test assertions need to be adjusted. Or we don\u0027t sort at all and use OR, as you proposed.","commit_id":"d994a40e5fce2ff5b055d046d304f45f239e0e73"}],"ci/roles/subnet/tasks/subnet-pool.yaml":[{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"9e7b9e0cc874ceb98cca829b9b9af3130606deec","unresolved":true,"context_lines":[{"line_number":127,"context_line":""},{"line_number":128,"context_line":"- name: Sort Subnet Allocation Pools"},{"line_number":129,"context_line":"  ansible.builtin.set_fact:"},{"line_number":130,"context_line":"    allocation_pools: \"{{ subnet_result.subnets[0].allocation_pools | sort(attribute\u003d\u0027start\u0027) }}\""},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"- name: Verify Subnet Allocation Pools Exist"},{"line_number":133,"context_line":"  assert:"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"2629aeff_9bcb90fb","line":130,"updated":"2024-10-22 12:36:42.000000000","message":"Didn\u0027t sort it?\n\nhttps://074901964fd52c807088-0be6f2faeb5dd053eec85051536d7f3c.ssl.cf2.rackcdn.com/912775/5/check/ansible-collections-openstack-functional-devstack/5cc90c1/job-output.txt\n\n10-22 10:54:52.749072 | controller | TASK [subnet : Sort Subnet Allocation Pools] ***********************************\n2024-10-22 10:54:52.749117 | controller | task path: /home/zuul/src/opendev.org/openstack/ansible-collections-openstack/ci/roles/subnet/tasks/subnet-pool.yaml:128\n2024-10-22 10:54:52.749139 | controller | ok: [localhost] \u003d\u003e {\n2024-10-22 10:54:52.819787 | controller |     \"ansible_facts\": {\n2024-10-22 10:54:52.819833 | controller |         \"allocation_pools\": [\n2024-10-22 10:54:52.819843 | controller |             {\n2024-10-22 10:54:52.819851 | controller |                 \"end\": \"192.168.42.8\",\n2024-10-22 10:54:52.819859 | controller |                 \"start\": \"192.168.42.6\"\n2024-10-22 10:54:52.819867 | controller |             },\n2024-10-22 10:54:52.819874 | controller |             {\n2024-10-22 10:54:52.819881 | controller |                 \"end\": \"192.168.42.4\",\n2024-10-22 10:54:52.819889 | controller |                 \"start\": \"192.168.42.2\"\n2024-10-22 10:54:52.819896 | controller |             }\n2024-10-22 10:54:52.819903 | controller |         ]\n2024-10-22 10:54:52.819911 | controller |     },\n2024-10-22 10:54:52.819918 | controller |     \"changed\": false\n2024-10-22 10:54:52.819925 | controller | }","commit_id":"d994a40e5fce2ff5b055d046d304f45f239e0e73"},{"author":{"_account_id":34150,"name":"Simon Hensel","email":"simon.hensel@inovex.de","username":"shensel"},"change_message_id":"2eef0cd218aa4541a56ffe0bafbfe89a0b41c12f","unresolved":true,"context_lines":[{"line_number":127,"context_line":""},{"line_number":128,"context_line":"- name: Sort Subnet Allocation Pools"},{"line_number":129,"context_line":"  ansible.builtin.set_fact:"},{"line_number":130,"context_line":"    allocation_pools: \"{{ subnet_result.subnets[0].allocation_pools | sort(attribute\u003d\u0027start\u0027) }}\""},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"- name: Verify Subnet Allocation Pools Exist"},{"line_number":133,"context_line":"  assert:"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"555e51d0_64c6e574","line":130,"in_reply_to":"2629aeff_9bcb90fb","updated":"2024-10-22 12:54:00.000000000","message":"The sorting works (and is necessary since the Openstack SDK returns allocation pools in random order), in this case reverse sorting it not necessary, whereas the subnet-allocation test needs to be sorted to match the assertion.","commit_id":"d994a40e5fce2ff5b055d046d304f45f239e0e73"},{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"9e7b9e0cc874ceb98cca829b9b9af3130606deec","unresolved":true,"context_lines":[{"line_number":141,"context_line":"- name: Verify Subnet Allocation Pools"},{"line_number":142,"context_line":"  assert:"},{"line_number":143,"context_line":"    that:"},{"line_number":144,"context_line":"      - allocation_pools.0.start \u003d\u003d \u0027192.168.42.2\u0027"},{"line_number":145,"context_line":"      - allocation_pools.0.end \u003d\u003d \u0027192.168.42.4\u0027"},{"line_number":146,"context_line":"      - allocation_pools.1.start \u003d\u003d \u0027192.168.42.6\u0027"},{"line_number":147,"context_line":"      - allocation_pools.1.end \u003d\u003d \u0027192.168.42.8\u0027"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"- name: Delete subnet {{ subnet_name }}"},{"line_number":150,"context_line":"  openstack.cloud.subnet:"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"fcd3731e_85e12a9f","line":147,"range":{"start_line":144,"start_character":8,"end_line":147,"end_character":48},"updated":"2024-10-22 12:36:42.000000000","message":"Instead of sorting that didn\u0027t work (no idea why), it\u0027s possible to do like:\n\n- (allocation_pools.0.start \u003d\u003d \u0027192.168.42.2\u0027 and allocation_pools.0.end \u003d\u003d \u0027192.168.42.4\u0027) or (allocation_pools.0.start \u003d\u003d \u0027192.168.42.6\u0027 and allocation_pools.0.end \u003d\u003d \u0027192.168.42.8\u0027)\n- (allocation_pools.1.start \u003d\u003d \u0027192.168.42.2\u0027 and allocation_pools.1.end \u003d\u003d \u0027192.168.42.4\u0027) or (allocation_pools.1.start \u003d\u003d \u0027192.168.42.6\u0027 and allocation_pools.1.end \u003d\u003d \u0027192.168.42.8\u0027)","commit_id":"d994a40e5fce2ff5b055d046d304f45f239e0e73"}],"plugins/modules/subnet.py":[{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"d5c52593a40f33c681f1c3e7181a6429c8aa097c","unresolved":true,"context_lines":[{"line_number":297,"context_line":"        disable_gateway_ip\u003ddict("},{"line_number":298,"context_line":"            type\u003d\u0027bool\u0027, default\u003dFalse, aliases\u003d[\u0027no_gateway_ip\u0027]),"},{"line_number":299,"context_line":"        dns_nameservers\u003ddict(type\u003d\u0027list\u0027, elements\u003d\u0027str\u0027),"},{"line_number":300,"context_line":"        allocation_pool_start\u003ddict(),"},{"line_number":301,"context_line":"        allocation_pool_end\u003ddict(),"},{"line_number":302,"context_line":"        host_routes\u003ddict(type\u003d\u0027list\u0027, elements\u003d\u0027dict\u0027),"},{"line_number":303,"context_line":"        ipv6_ra_mode\u003ddict(choices\u003dipv6_mode_choices),"},{"line_number":304,"context_line":"        ipv6_address_mode\u003ddict(choices\u003dipv6_mode_choices),"}],"source_content_type":"text/x-python","patch_set":1,"id":"14e29e84_b7f86fe2","side":"PARENT","line":301,"range":{"start_line":300,"start_character":8,"end_line":301,"end_character":35},"updated":"2024-06-27 18:35:20.000000000","message":"I think this change is breaking backward compatibility and all people using allocation pools. Why not to add an option `allocation_pools` and keep this one for using as well?\nJust add this to `allocation_pools` later in code.","commit_id":"93d51498e9e489f54836504856068ebf8b29696b"},{"author":{"_account_id":34150,"name":"Simon Hensel","email":"simon.hensel@inovex.de","username":"shensel"},"change_message_id":"f49bacbf8908f0c73c72124c51e04191253d815a","unresolved":true,"context_lines":[{"line_number":297,"context_line":"        disable_gateway_ip\u003ddict("},{"line_number":298,"context_line":"            type\u003d\u0027bool\u0027, default\u003dFalse, aliases\u003d[\u0027no_gateway_ip\u0027]),"},{"line_number":299,"context_line":"        dns_nameservers\u003ddict(type\u003d\u0027list\u0027, elements\u003d\u0027str\u0027),"},{"line_number":300,"context_line":"        allocation_pool_start\u003ddict(),"},{"line_number":301,"context_line":"        allocation_pool_end\u003ddict(),"},{"line_number":302,"context_line":"        host_routes\u003ddict(type\u003d\u0027list\u0027, elements\u003d\u0027dict\u0027),"},{"line_number":303,"context_line":"        ipv6_ra_mode\u003ddict(choices\u003dipv6_mode_choices),"},{"line_number":304,"context_line":"        ipv6_address_mode\u003ddict(choices\u003dipv6_mode_choices),"}],"source_content_type":"text/x-python","patch_set":1,"id":"7298ab74_f527e497","side":"PARENT","line":301,"range":{"start_line":300,"start_character":8,"end_line":301,"end_character":35},"in_reply_to":"14e29e84_b7f86fe2","updated":"2024-07-01 14:20:13.000000000","message":"Fair point, I did not think of that. The patch is now updated, please review again.","commit_id":"93d51498e9e489f54836504856068ebf8b29696b"}]}