From 76467ca52850a697bac71c47b2656f7677734e26 Mon Sep 17 00:00:00 2001 From: JavidMinadathAlimohi Date: Tue, 17 Dec 2024 18:04:40 -0500 Subject: [PATCH 01/10] Add support for setting minimum and maximum object size in S3 lifecycle rule --- plugins/modules/s3_lifecycle.py | 47 +++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/plugins/modules/s3_lifecycle.py b/plugins/modules/s3_lifecycle.py index 2f48e06d404..7d0feb2a0ae 100644 --- a/plugins/modules/s3_lifecycle.py +++ b/plugins/modules/s3_lifecycle.py @@ -59,6 +59,16 @@ and replaced with the new transition(s) default: true type: bool + maximum_object_size: + descriptionL + - The maximum object size to which the rule applies. + required: false + type: int + minimum_object_size: + descriptionL + - The minimum object size to which the rule applies. + required: false + type: int noncurrent_version_expiration_days: description: - The number of days after which non-current versions should be deleted. @@ -278,6 +288,8 @@ def build_rule(client, module): expiration_date = parse_date(module.params.get("expiration_date")) expiration_days = module.params.get("expiration_days") expire_object_delete_marker = module.params.get("expire_object_delete_marker") + maximum_object_size = module.params.get("maximum_object_size") + minimum_object_size = module.params.get("minimum_object_size") noncurrent_version_expiration_days = module.params.get("noncurrent_version_expiration_days") noncurrent_version_transition_days = module.params.get("noncurrent_version_transition_days") noncurrent_version_transitions = module.params.get("noncurrent_version_transitions") @@ -292,7 +304,15 @@ def build_rule(client, module): transitions = module.params.get("transitions") purge_transitions = module.params.get("purge_transitions") - rule = dict(Filter=dict(Prefix=prefix), Status=status.title()) + if maximum_object_size is not None or minimum_object_size is not None: + and_dict = dict(Prefix=prefix) + if minimum_object_size is not None: + and_dict["ObjectSizeGreaterThan"] = minimum_object_size + if maximum_object_size is not None: + and_dict["ObjectSizeLessThan"] = maximum_object_size + rule = dict(Filter=dict(And=and_dict), Status=status.title()) + else: + rule = dict(Filter=dict(Prefix=prefix), Status=status.title()) if rule_id is not None: rule["ID"] = rule_id @@ -363,17 +383,26 @@ def compare_and_update_configuration(client, module, current_lifecycle_rules, ru changed = False appended = False + # Helper function to deeply compare filters + def filters_are_equal(filter1, filter2): + if not filter1 or not filter2: + return filter1 == filter2 + return ( + filter1.get("Prefix", "") == filter2.get("Prefix", "") + and filter1.get("ObjectSizeGreaterThan") == filter2.get("ObjectSizeGreaterThan") + and filter1.get("ObjectSizeLessThan") == filter2.get("ObjectSizeLessThan") + and filter1.get("And", {}).get("Prefix", "") == filter2.get("And", {}).get("Prefix", "") + and filter1.get("And", {}).get("ObjectSizeGreaterThan") == filter2.get("And", {}).get("ObjectSizeGreaterThan") + and filter1.get("And", {}).get("ObjectSizeLessThan") == filter2.get("And", {}).get("ObjectSizeLessThan") + ) + # If current_lifecycle_obj is not None then we have rules to compare, otherwise just add the rule if current_lifecycle_rules: # If rule ID exists, use that for comparison otherwise compare based on prefix for existing_rule in current_lifecycle_rules: - if rule.get("ID") == existing_rule.get("ID") and rule["Filter"].get("Prefix", "") != existing_rule.get( - "Filter", {} - ).get("Prefix", ""): + if rule.get("ID") == existing_rule.get("ID") and not filters_are_equal(rule.get("Filter", {}), existing_rule.get("Filter", {})): existing_rule.pop("ID") - elif rule_id is None and rule["Filter"].get("Prefix", "") == existing_rule.get("Filter", {}).get( - "Prefix", "" - ): + elif rule_id is None and filters_are_equal(rule.get("Filter", {}), existing_rule.get("Filter", {})): existing_rule.pop("ID") if rule.get("ID") == existing_rule.get("ID"): changed_, appended_ = update_or_append_rule( @@ -598,6 +627,8 @@ def main(): expiration_days=dict(type="int"), expiration_date=dict(), expire_object_delete_marker=dict(type="bool"), + maximum_object_size=dict(type=int), + minimum_object_size=dict(type=int), noncurrent_version_expiration_days=dict(type="int"), noncurrent_version_keep_newer=dict(type="int"), noncurrent_version_storage_class=dict(default="glacier", type="str", choices=s3_storage_class), @@ -681,4 +712,4 @@ def main(): if __name__ == "__main__": - main() + main() \ No newline at end of file From 9a6f8ffb54a7abe72f96b1fce1be16043d5f80a3 Mon Sep 17 00:00:00 2001 From: JavidMinadathAlimohi Date: Tue, 17 Dec 2024 18:38:03 -0500 Subject: [PATCH 02/10] Updated the integration test case for s3_lifecycle to validate minimum and maximum object size settings --- .../targets/s3_lifecycle/tasks/main.yml | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/tests/integration/targets/s3_lifecycle/tasks/main.yml b/tests/integration/targets/s3_lifecycle/tasks/main.yml index d9f169561af..947c356ef47 100644 --- a/tests/integration/targets/s3_lifecycle/tasks/main.yml +++ b/tests/integration/targets/s3_lifecycle/tasks/main.yml @@ -694,6 +694,95 @@ that: - output is changed + # Check create and delete lifecycle policy with minimum and maximum object size (with prefix) + - name: Create rule with minimum and maximum object size + s3_lifecycle: + name: "{{ bucket_name }}" + rule_id: minimum-object-size-prefix + prefix: /something/ + minimum_object_size: 100 + maximum_object_size: 1000 + state: present + status: enabled + expiration_days: 30 + register: output + - assert: + that: + - output is changed + + - name: Create rule with minimum object size (idempotency) + s3_lifecycle: + name: "{{ bucket_name }}" + rule_id: minimum-object-size-prefix + prefix: /something/ + minimum_object_size: 100 + maximum_object_size: 1000 + state: present + status: enabled + expiration_days: 30 + register: output + - assert: + that: + - output is not changed + + - name: Delete rule with minimum and maximum object size + s3_lifecycle: + name: "{{ bucket_name }}" + rule_id: minimum-object-size-prefix + prefix: /something/ + minimum_object_size: 100 + maximum_object_size: 1000 + state: absent + status: enabled + expiration_days: 30 + register: output + - assert: + that: + - output is changed + + # Check create and delete lifecycle policy with minimum and maximum object size (no prefix) + - name: Create rule with minimum and maximum object size + s3_lifecycle: + name: "{{ bucket_name }}" + rule_id: minimum-object-size-noprefix + minimum_object_size: 100 + maximum_object_size: 1000 + state: present + status: enabled + expiration_days: 30 + register: output + - assert: + that: + - output is changed + + - name: Create rule with minimum object size (idempotency) + s3_lifecycle: + name: "{{ bucket_name }}" + rule_id: minimum-object-size-noprefix + minimum_object_size: 100 + maximum_object_size: 1000 + state: present + status: enabled + expiration_days: 30 + register: output + - assert: + that: + - output is not changed + + - name: Delete rule with minimum and maximum object size + s3_lifecycle: + name: "{{ bucket_name }}" + rule_id: minimum-object-size-noprefix + minimum_object_size: 100 + maximum_object_size: 1000 + state: absent + status: enabled + expiration_days: 30 + register: output + - assert: + that: + - output is changed + # ============================================================ always: - name: Ensure all buckets are deleted From da15b55a53a0d11dfdddd6c051c4a748aba2465c Mon Sep 17 00:00:00 2001 From: JavidMinadathAlimohi Date: Wed, 18 Dec 2024 18:46:08 -0500 Subject: [PATCH 03/10] 1. Fixed the issue on documentation. 2. Updated code based on the feedback from PR review --- plugins/modules/s3_lifecycle.py | 38 +++++++++++++++++---------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/plugins/modules/s3_lifecycle.py b/plugins/modules/s3_lifecycle.py index 7d0feb2a0ae..96e63585e9d 100644 --- a/plugins/modules/s3_lifecycle.py +++ b/plugins/modules/s3_lifecycle.py @@ -60,12 +60,12 @@ default: true type: bool maximum_object_size: - descriptionL + description: - The maximum object size to which the rule applies. required: false type: int minimum_object_size: - descriptionL + description: - The minimum object size to which the rule applies. required: false type: int @@ -281,7 +281,22 @@ def fetch_rules(client, module, name): module.fail_json_aws(e) return current_lifecycle_rules - +# Helper function to deeply compare filters +def filters_are_equal(filter1, filter2): + if filter1 == filter2: + return True + if not filter1 or not filter2: + return False + # Treat empty string as equal to a filter not being set + return ( + filter1.get("Prefix", "") == filter2.get("Prefix", "") + and filter1.get("ObjectSizeGreaterThan") == filter2.get("ObjectSizeGreaterThan") + and filter1.get("ObjectSizeLessThan") == filter2.get("ObjectSizeLessThan") + and filter1.get("And", {}).get("Prefix", "") == filter2.get("And", {}).get("Prefix", "") + and filter1.get("And", {}).get("ObjectSizeGreaterThan") == filter2.get("And", {}).get("ObjectSizeGreaterThan") + and filter1.get("And", {}).get("ObjectSizeLessThan") == filter2.get("And", {}).get("ObjectSizeLessThan") + ) + def build_rule(client, module): name = module.params.get("name") abort_incomplete_multipart_upload_days = module.params.get("abort_incomplete_multipart_upload_days") @@ -382,27 +397,14 @@ def compare_and_update_configuration(client, module, current_lifecycle_rules, ru lifecycle_configuration = dict(Rules=[]) changed = False appended = False - - # Helper function to deeply compare filters - def filters_are_equal(filter1, filter2): - if not filter1 or not filter2: - return filter1 == filter2 - return ( - filter1.get("Prefix", "") == filter2.get("Prefix", "") - and filter1.get("ObjectSizeGreaterThan") == filter2.get("ObjectSizeGreaterThan") - and filter1.get("ObjectSizeLessThan") == filter2.get("ObjectSizeLessThan") - and filter1.get("And", {}).get("Prefix", "") == filter2.get("And", {}).get("Prefix", "") - and filter1.get("And", {}).get("ObjectSizeGreaterThan") == filter2.get("And", {}).get("ObjectSizeGreaterThan") - and filter1.get("And", {}).get("ObjectSizeLessThan") == filter2.get("And", {}).get("ObjectSizeLessThan") - ) # If current_lifecycle_obj is not None then we have rules to compare, otherwise just add the rule if current_lifecycle_rules: # If rule ID exists, use that for comparison otherwise compare based on prefix for existing_rule in current_lifecycle_rules: - if rule.get("ID") == existing_rule.get("ID") and not filters_are_equal(rule.get("Filter", {}), existing_rule.get("Filter", {})): + if rule.get("ID") == existing_rule.get("ID") and not filters_are_equal(rule.get("Filter"), existing_rule.get("Filter")): existing_rule.pop("ID") - elif rule_id is None and filters_are_equal(rule.get("Filter", {}), existing_rule.get("Filter", {})): + elif rule_id is None and filters_are_equal(rule.get("Filter"), existing_rule.get("Filter")): existing_rule.pop("ID") if rule.get("ID") == existing_rule.get("ID"): changed_, appended_ = update_or_append_rule( From 25258af7b16bf4aef1a05648373e9d4a5b51262e Mon Sep 17 00:00:00 2001 From: JavidMinadathAlimohi Date: Mon, 23 Dec 2024 15:22:20 -0500 Subject: [PATCH 04/10] 1. Include unit test for function "filters_are_equal" 2. Included a changelog minor change fragment file. 3. Enclosed types within double quotes. --- .../2205-support-minmax-s3lifecycle.yml | 2 + plugins/modules/s3_lifecycle.py | 4 +- .../test_s3_lifecycle_filters_are_equal.py | 108 ++++++++++++++++++ 3 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 changelogs/fragments/2205-support-minmax-s3lifecycle.yml create mode 100644 tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py diff --git a/changelogs/fragments/2205-support-minmax-s3lifecycle.yml b/changelogs/fragments/2205-support-minmax-s3lifecycle.yml new file mode 100644 index 00000000000..2f192ea4ec6 --- /dev/null +++ b/changelogs/fragments/2205-support-minmax-s3lifecycle.yml @@ -0,0 +1,2 @@ +minor_changes: + - s3_lifecycle - Support for min and max object size when applying the filter rules. \ No newline at end of file diff --git a/plugins/modules/s3_lifecycle.py b/plugins/modules/s3_lifecycle.py index 96e63585e9d..c82290df250 100644 --- a/plugins/modules/s3_lifecycle.py +++ b/plugins/modules/s3_lifecycle.py @@ -629,8 +629,8 @@ def main(): expiration_days=dict(type="int"), expiration_date=dict(), expire_object_delete_marker=dict(type="bool"), - maximum_object_size=dict(type=int), - minimum_object_size=dict(type=int), + maximum_object_size=dict(type="int"), + minimum_object_size=dict(type="int"), noncurrent_version_expiration_days=dict(type="int"), noncurrent_version_keep_newer=dict(type="int"), noncurrent_version_storage_class=dict(default="glacier", type="str", choices=s3_storage_class), diff --git a/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py b/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py new file mode 100644 index 00000000000..8a3f0b4bbb0 --- /dev/null +++ b/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py @@ -0,0 +1,108 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2023, Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +import pytest + +from ansible_collections.community.aws.plugins.modules.s3_lifecycle import filters_are_equal + +def test_filters_are_equal(): + # Test case 1: Both filters are identical + filter1 = { + "And": { + "Prefix": "nested/", + "ObjectSizeGreaterThan": 150, + "ObjectSizeLessThan": 180, + }, + } + filter2 = filter1.copy() + assert filters_are_equal(filter1, filter2) is True + + # Test case 2: One filter is None + filter1 = None + filter2 = { + "Prefix": "test/" + } + assert filters_are_equal(filter1, filter2) is False + + # Test case 3: One filter is empty + filter1 = {} + filter2 = { + "ObjectSizeGreaterThan": 100, + } + assert filters_are_equal(filter1, filter2) is False + + # Test case 4: Filters differ in a single key + filter1 = { + "ObjectSizeGreaterThan": 100, + } + filter2 = { + "ObjectSizeGreaterThan": 200, # Different value + } + assert filters_are_equal(filter1, filter2) is False + + # Test case 5: Filters with missing `And` key in one filter + filter1 = { + "Prefix": "test/", + } + filter2 = { + "And": { + "Prefix": "nested/", + "ObjectSizeGreaterThan": 100, + }, + } + assert filters_are_equal(filter1, filter2) is False + + # Test case 6: Filters with nested `And` keys matching + filter1 = { + "And": { + "Prefix": "nested/", + "ObjectSizeGreaterThan": 150, + }, + } + filter2 = { + "And": { + "Prefix": "nested/", + "ObjectSizeGreaterThan": 150, + }, + } + assert filters_are_equal(filter1, filter2) is True + + # Test case 7: Filters with nested `And` keys differing + filter1 = { + "And": { + "Prefix": "test/", + "ObjectSizeGreaterThan": 150, + }, + } + filter2 = { + "And": { + "Prefix": "nested/", # Different key/value + "ObjectSizeLessThan": 150, + }, + } + assert filters_are_equal(filter1, filter2) is False + + # Test case 8: Both filters are None + filter1 = None + filter2 = None + assert filters_are_equal(filter1, filter2) is False + + # Test case 9: Filters with different `Prefix` values + filter1 = { + "Prefix": "test/" + } + filter2 = { + "Prefix": "different/" + } + assert filters_are_equal(filter1, filter2) is False + + # Test case 10: Filters with empty strings for `Prefix` + filter1 = { + "Prefix": "" + } + filter2 = { + "Prefix": "" + } + assert filters_are_equal(filter1, filter2) is True \ No newline at end of file From 66f1b93e74fe4a2f688f1740d61157222144e8a5 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Sat, 28 Dec 2024 11:43:57 +0100 Subject: [PATCH 05/10] Apply suggestions from code review lint --- changelogs/fragments/2205-support-minmax-s3lifecycle.yml | 2 +- plugins/modules/s3_lifecycle.py | 9 ++++++--- .../modules/test_s3_lifecycle_filters_are_equal.py | 6 +++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/changelogs/fragments/2205-support-minmax-s3lifecycle.yml b/changelogs/fragments/2205-support-minmax-s3lifecycle.yml index 2f192ea4ec6..9eeb11d5f39 100644 --- a/changelogs/fragments/2205-support-minmax-s3lifecycle.yml +++ b/changelogs/fragments/2205-support-minmax-s3lifecycle.yml @@ -1,2 +1,2 @@ minor_changes: - - s3_lifecycle - Support for min and max object size when applying the filter rules. \ No newline at end of file + - s3_lifecycle - Support for min and max object size when applying the filter rules (https://github.com/ansible-collections/community.aws/pull/2205). \ No newline at end of file diff --git a/plugins/modules/s3_lifecycle.py b/plugins/modules/s3_lifecycle.py index c82290df250..993ee948563 100644 --- a/plugins/modules/s3_lifecycle.py +++ b/plugins/modules/s3_lifecycle.py @@ -64,10 +64,12 @@ - The maximum object size to which the rule applies. required: false type: int + version_added: 9.1.0 minimum_object_size: description: - The minimum object size to which the rule applies. required: false + version_added: 9.1.0 type: int noncurrent_version_expiration_days: description: @@ -281,6 +283,7 @@ def fetch_rules(client, module, name): module.fail_json_aws(e) return current_lifecycle_rules + # Helper function to deeply compare filters def filters_are_equal(filter1, filter2): if filter1 == filter2: @@ -296,7 +299,8 @@ def filters_are_equal(filter1, filter2): and filter1.get("And", {}).get("ObjectSizeGreaterThan") == filter2.get("And", {}).get("ObjectSizeGreaterThan") and filter1.get("And", {}).get("ObjectSizeLessThan") == filter2.get("And", {}).get("ObjectSizeLessThan") ) - + + def build_rule(client, module): name = module.params.get("name") abort_incomplete_multipart_upload_days = module.params.get("abort_incomplete_multipart_upload_days") @@ -397,7 +401,6 @@ def compare_and_update_configuration(client, module, current_lifecycle_rules, ru lifecycle_configuration = dict(Rules=[]) changed = False appended = False - # If current_lifecycle_obj is not None then we have rules to compare, otherwise just add the rule if current_lifecycle_rules: # If rule ID exists, use that for comparison otherwise compare based on prefix @@ -714,4 +717,4 @@ def main(): if __name__ == "__main__": - main() \ No newline at end of file + main() diff --git a/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py b/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py index 8a3f0b4bbb0..54cdee2d761 100644 --- a/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py +++ b/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py @@ -78,8 +78,8 @@ def test_filters_are_equal(): } filter2 = { "And": { - "Prefix": "nested/", # Different key/value - "ObjectSizeLessThan": 150, + "Prefix": "nested/", # Different key/value + "ObjectSizeLessThan": 150, }, } assert filters_are_equal(filter1, filter2) is False @@ -105,4 +105,4 @@ def test_filters_are_equal(): filter2 = { "Prefix": "" } - assert filters_are_equal(filter1, filter2) is True \ No newline at end of file + assert filters_are_equal(filter1, filter2) is True From 3f294607d045c9caa869ca60849bfdc137507ccb Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Sat, 28 Dec 2024 11:44:13 +0100 Subject: [PATCH 06/10] lint --- .../unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py b/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py index 54cdee2d761..f6bb5223708 100644 --- a/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py +++ b/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py @@ -7,6 +7,7 @@ from ansible_collections.community.aws.plugins.modules.s3_lifecycle import filters_are_equal + def test_filters_are_equal(): # Test case 1: Both filters are identical filter1 = { From 23dc3ec80410041239f45f2be66da676648fc5d3 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Sat, 28 Dec 2024 14:36:44 +0100 Subject: [PATCH 07/10] Move tests --- .../test_filters_are_equal.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/unit/plugins/modules/{test_s3_lifecycle_filters_are_equal.py => s3_lifecycle/test_filters_are_equal.py} (100%) diff --git a/tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py b/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py similarity index 100% rename from tests/unit/plugins/modules/test_s3_lifecycle_filters_are_equal.py rename to tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py From fa4dd7df736ffeb576c4bdd0c59e5e3361115640 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Sat, 28 Dec 2024 14:38:35 +0100 Subject: [PATCH 08/10] unused import --- .../modules/s3_lifecycle/test_filters_are_equal.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py b/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py index f6bb5223708..17e77653557 100644 --- a/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py +++ b/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py @@ -3,11 +3,13 @@ # Copyright: (c) 2023, Ansible Project # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) -import pytest - from ansible_collections.community.aws.plugins.modules.s3_lifecycle import filters_are_equal - +# @pytest.mark.parametrize( +# "filter1,filter2,result", +# [] +# ) +# def test_filters_are_equal(filter1, filter2, result): def test_filters_are_equal(): # Test case 1: Both filters are identical filter1 = { From d88b6775db499c3c35a6b058df33dbcb24da6e31 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Sat, 28 Dec 2024 17:39:15 +0100 Subject: [PATCH 09/10] Flip unit tests to pytest.mark.parameterized --- plugins/modules/s3_lifecycle.py | 4 +- .../s3_lifecycle/test_filters_are_equal.py | 210 +++++++++--------- 2 files changed, 112 insertions(+), 102 deletions(-) diff --git a/plugins/modules/s3_lifecycle.py b/plugins/modules/s3_lifecycle.py index 993ee948563..64bce4516e1 100644 --- a/plugins/modules/s3_lifecycle.py +++ b/plugins/modules/s3_lifecycle.py @@ -405,7 +405,9 @@ def compare_and_update_configuration(client, module, current_lifecycle_rules, ru if current_lifecycle_rules: # If rule ID exists, use that for comparison otherwise compare based on prefix for existing_rule in current_lifecycle_rules: - if rule.get("ID") == existing_rule.get("ID") and not filters_are_equal(rule.get("Filter"), existing_rule.get("Filter")): + if rule.get("ID") == existing_rule.get("ID") and not filters_are_equal( + rule.get("Filter"), existing_rule.get("Filter") + ): existing_rule.pop("ID") elif rule_id is None and filters_are_equal(rule.get("Filter"), existing_rule.get("Filter")): existing_rule.pop("ID") diff --git a/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py b/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py index 17e77653557..84adfa564b7 100644 --- a/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py +++ b/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py @@ -3,109 +3,117 @@ # Copyright: (c) 2023, Ansible Project # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) -from ansible_collections.community.aws.plugins.modules.s3_lifecycle import filters_are_equal - -# @pytest.mark.parametrize( -# "filter1,filter2,result", -# [] -# ) -# def test_filters_are_equal(filter1, filter2, result): -def test_filters_are_equal(): - # Test case 1: Both filters are identical - filter1 = { - "And": { - "Prefix": "nested/", - "ObjectSizeGreaterThan": 150, - "ObjectSizeLessThan": 180, - }, - } - filter2 = filter1.copy() - assert filters_are_equal(filter1, filter2) is True - - # Test case 2: One filter is None - filter1 = None - filter2 = { - "Prefix": "test/" - } - assert filters_are_equal(filter1, filter2) is False - - # Test case 3: One filter is empty - filter1 = {} - filter2 = { - "ObjectSizeGreaterThan": 100, - } - assert filters_are_equal(filter1, filter2) is False +import pytest - # Test case 4: Filters differ in a single key - filter1 = { - "ObjectSizeGreaterThan": 100, - } - filter2 = { - "ObjectSizeGreaterThan": 200, # Different value - } - assert filters_are_equal(filter1, filter2) is False - - # Test case 5: Filters with missing `And` key in one filter - filter1 = { - "Prefix": "test/", - } - filter2 = { - "And": { - "Prefix": "nested/", - "ObjectSizeGreaterThan": 100, - }, - } - assert filters_are_equal(filter1, filter2) is False - - # Test case 6: Filters with nested `And` keys matching - filter1 = { - "And": { - "Prefix": "nested/", - "ObjectSizeGreaterThan": 150, - }, - } - filter2 = { - "And": { - "Prefix": "nested/", - "ObjectSizeGreaterThan": 150, - }, - } - assert filters_are_equal(filter1, filter2) is True +from ansible_collections.community.aws.plugins.modules.s3_lifecycle import filters_are_equal - # Test case 7: Filters with nested `And` keys differing - filter1 = { - "And": { - "Prefix": "test/", - "ObjectSizeGreaterThan": 150, - }, - } - filter2 = { - "And": { - "Prefix": "nested/", # Different key/value - "ObjectSizeLessThan": 150, - }, - } - assert filters_are_equal(filter1, filter2) is False - # Test case 8: Both filters are None - filter1 = None - filter2 = None - assert filters_are_equal(filter1, filter2) is False +@pytest.mark.parametrize( + "filter1,filter2,result", + [ + [None, None, True], + [{}, {}, True], + # Simple filters equal + [{"Prefix": ""}, {"Prefix": ""}, True], + [{"Prefix": "prefix/"}, {"Prefix": "prefix/"}, True], + [{"ObjectSizeGreaterThan": 100}, {"ObjectSizeGreaterThan": 100}, True], + [{"ObjectSizeLessThan": 100}, {"ObjectSizeLessThan": 100}, True], + # One filter is empty + [{"Prefix": ""}, {}, False], + [{"ObjectSizeGreaterThan": 100}, {}, False], + [{"ObjectSizeLessThan": 100}, {}, False], + # One filter is None + [{"Prefix": ""}, None, False], + [{"ObjectSizeGreaterThan": 100}, None, False], + [{"ObjectSizeLessThan": 100}, None, False], + # Filters differ in a single key + [{"Prefix": "prefix/"}, {"Prefix": "prefix2/"}, False], + [{"ObjectSizeGreaterThan": 100}, {"ObjectSizeGreaterThan": 200}, False], + [{"ObjectSizeLessThan": 100}, {"ObjectSizeLessThan": 200}, False], + # While in theory, these would be equal. We currently don't treat them as such and + # a single key in the "And" dict is technically not valid. + [{"Prefix": "prefix/"}, {"And": {"Prefix": "prefix/"}}, False], + [{"ObjectSizeGreaterThan": 100}, {"And": {"ObjectSizeGreaterThan": 100}}, False], + [{"ObjectSizeLessThan": 100}, {"And": {"ObjectSizeLessThan": 100}}, False], + ], +) +def test_filters_are_equal_simple(filter1, filter2, result): + assert filters_are_equal(filter1, filter2) is result + assert filters_are_equal(filter2, filter1) is result - # Test case 9: Filters with different `Prefix` values - filter1 = { - "Prefix": "test/" - } - filter2 = { - "Prefix": "different/" - } - assert filters_are_equal(filter1, filter2) is False - # Test case 10: Filters with empty strings for `Prefix` - filter1 = { - "Prefix": "" - } - filter2 = { - "Prefix": "" - } - assert filters_are_equal(filter1, filter2) is True +# Could be merged with the ones above, but naming will give a better idea of what's wrong +@pytest.mark.parametrize( + "filter1,filter2,result", + [ + # Equal + [ + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + True, + ], + # Special case of "Prefix" missing == Prefix of "" + [ + {"And": {"Prefix": "", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + {"And": {"ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + True, + ], + # Equal but with 2 of 3 "And" keys + [ + {"And": {"ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + {"And": {"ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + True, + ], + [ + {"And": {"Prefix": "nested/", "ObjectSizeLessThan": 180}}, + {"And": {"Prefix": "nested/", "ObjectSizeLessThan": 180}}, + True, + ], + [ + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150}}, + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150}}, + True, + ], + # One key missing + [ + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + {"And": {"ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + False, + ], + [ + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + {"And": {"Prefix": "nested/", "ObjectSizeLessThan": 180}}, + False, + ], + [ + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150}}, + False, + ], + # One key different + [ + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + {"And": {"Prefix": "another/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + False, + ], + [ + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 42, "ObjectSizeLessThan": 180}}, + False, + ], + [ + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 180}}, + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 90}}, + False, + ], + # Mixed with a non-and + [ + {"Prefix": "test/"}, + {"And": {"Prefix": "nested/", "ObjectSizeGreaterThan": 150, "ObjectSizeLessThan": 90}}, + False, + ], + ], +) +def test_filters_are_equal_and(filter1, filter2, result): + assert filters_are_equal(filter1, filter2) is result + assert filters_are_equal(filter2, filter1) is result From ad956eb611851520a2be80f1f4f27cfb5ba772ee Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Sun, 29 Dec 2024 09:58:00 +0100 Subject: [PATCH 10/10] ignore warning about reversed order --- .../plugins/modules/s3_lifecycle/test_filters_are_equal.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py b/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py index 84adfa564b7..4f295646300 100644 --- a/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py +++ b/tests/unit/plugins/modules/s3_lifecycle/test_filters_are_equal.py @@ -39,7 +39,7 @@ ) def test_filters_are_equal_simple(filter1, filter2, result): assert filters_are_equal(filter1, filter2) is result - assert filters_are_equal(filter2, filter1) is result + assert filters_are_equal(filter2, filter1) is result # pylint: disable=arguments-out-of-order # Could be merged with the ones above, but naming will give a better idea of what's wrong @@ -116,4 +116,4 @@ def test_filters_are_equal_simple(filter1, filter2, result): ) def test_filters_are_equal_and(filter1, filter2, result): assert filters_are_equal(filter1, filter2) is result - assert filters_are_equal(filter2, filter1) is result + assert filters_are_equal(filter2, filter1) is result # pylint: disable=arguments-out-of-order