[bct] Add more flexibility to suppression mechanism (#36226)

* wip suppression logic update

* Improve suppression logic

* add mgmt plane ignores

* reword

* switch loop order, match, deepcopy (#36249)

* add named tuple for suppressions

* update tests

* update wording

* clean up imports

---------

Co-authored-by: Catalina Peralta <[email protected]>
Co-authored-by: Krista Pratico <[email protected]>

Author: 3 people

scripts/breaking_changes_checker/breaking_changes_allowlist.py

@@ -11,4 +11,24 @@
 
 # See Readme for ignore format
 
-IGNORE_BREAKING_CHANGES = {}
+IGNORE_BREAKING_CHANGES = {
+    "azure-mgmt-": [
+        # Changes due to latest dpg design + need to support overloads in this tool
+        ("ChangedParameterOrdering", "*", "*", "__init__"),
+        # Changes due to latest dpg design
+        ("ChangedParameterKind", "*", "*", "*", "top"),
+        ("ChangedParameterKind", "*", "*", "*", "filter"),
+        ("ChangedParameterKind", "*", "*", "*", "skip"),
+        ("RemovedOrRenamedPositionalParam", "*", "*", "*", "maxpagesize"),
+        # Changes due to not using msrest anymore
+        ("RemovedOrRenamedClassMethod", "*", "*", "as_dict"),
+        ("RemovedOrRenamedClassMethod", "*", "*", "deserialize"),
+        ("RemovedOrRenamedClassMethod", "*", "*", "enable_additional_properties_sending"),
+        ("RemovedOrRenamedClassMethod", "*", "*", "from_dict"),
+        ("RemovedOrRenamedClassMethod", "*", "*", "is_xml_model"),
+        ("RemovedOrRenamedClassMethod", "*", "*", "serialize"),
+        ("RemovedOrRenamedPositionalParam", "*", "*", "as_dict", "key_transformer"),
+        ("RemovedOrRenamedPositionalParam", "*", "*", "as_dict"),
+        ("RemovedFunctionKwargs", "*", "*", "as_dict"),
+    ]
+}

scripts/breaking_changes_checker/breaking_changes_tracker.py

@@ -5,12 +5,21 @@
 # Licensed under the MIT License. See License.txt in the project root for license information.
 # --------------------------------------------------------------------------------------------
 
-from enum import Enum
-from typing import Any, Dict, List, Union
 import jsondiff
+import re
+from enum import Enum
+from typing import Any, Dict, List, Union, Optional, NamedTuple
+from copy import deepcopy
 from breaking_changes_allowlist import IGNORE_BREAKING_CHANGES
 
 
+class Suppression(NamedTuple):
+    change_type: str
+    module: str
+    class_name: Optional[str] = None
+    function_name: Optional[str] = None
+    parameter_or_property_name: Optional[str] = None
+
 class BreakingChangeType(str, Enum):
     REMOVED_OR_RENAMED_CLIENT = "RemovedOrRenamedClient"
     REMOVED_OR_RENAMED_CLIENT_METHOD = "RemovedOrRenamedClientMethod"
@@ -42,11 +51,11 @@ class BreakingChangesTracker:
     REMOVED_OR_RENAMED_MODULE_LEVEL_FUNCTION_MSG = \
         "The publicly exposed function '{}.{}' was deleted or renamed in the current version"
     REMOVED_OR_RENAMED_POSITIONAL_PARAM_OF_METHOD_MSG = \
-        "The '{}.{} method '{}' had its '{}' parameter '{}' deleted or renamed in the current version"
+        "The '{}.{}' method '{}' had its parameter '{}' of kind '{}' deleted or renamed in the current version"
     REMOVED_OR_RENAMED_POSITIONAL_PARAM_OF_FUNCTION_MSG = \
-        "The function '{}.{}' had its '{}' parameter '{}' deleted or renamed in the current version"
+        "The function '{}.{}' had its parameter '{}' of kind '{}' deleted or renamed in the current version"
     ADDED_POSITIONAL_PARAM_TO_METHOD_MSG = \
-        "The '{}.{} method '{}' had a '{}' parameter '{}' inserted in the current version"
+        "The '{}.{}' method '{}' had a '{}' parameter '{}' inserted in the current version"
     ADDED_POSITIONAL_PARAM_TO_FUNCTION_MSG = \
         "The function '{}.{}' had a '{}' parameter '{}' inserted in the current version"
     REMOVED_OR_RENAMED_INSTANCE_ATTRIBUTE_FROM_CLIENT_MSG = \
@@ -444,15 +453,15 @@ def check_positional_parameter_removed_or_renamed(
                         (
                             self.REMOVED_OR_RENAMED_POSITIONAL_PARAM_OF_METHOD_MSG,
                             BreakingChangeType.REMOVED_OR_RENAMED_POSITIONAL_PARAM,
-                            self.module_name, self.class_name, self.function_name, param_type, deleted
+                            self.module_name, self.class_name, self.function_name, deleted, param_type
                         )
                     )
                 else:
                     self.breaking_changes.append(
                         (
                             self.REMOVED_OR_RENAMED_POSITIONAL_PARAM_OF_FUNCTION_MSG,
                             BreakingChangeType.REMOVED_OR_RENAMED_POSITIONAL_PARAM,
-                            self.module_name, self.function_name, param_type, deleted
+                            self.module_name, self.function_name, deleted, param_type
                         )
                     )
 
@@ -559,25 +568,46 @@ def check_module_level_function_removed_or_renamed(self, function_components: Di
                 )
             return True
 
-    # ----------------------------------- Report methods -----------------------------------
+    def match(self, bc, ignored):
+        if bc == ignored:
+            return True
+        for b, i in zip(bc, ignored):
+            if i == "*":
+                continue
+            if b != i:
+                return False
+        return True
+
     def get_reportable_breaking_changes(self, ignore_changes: Dict) -> List:
-        reportable_changes = []
-        ignored = ignore_changes[self.package_name]
-        for bc in self.breaking_changes:
-            msg, bc_type, module_name, *args = bc
+        ignored = []
+        # Match all ignore rules that should apply to this package
+        for ignored_package, ignore_rules in ignore_changes.items():
+            if re.findall(ignored_package, self.package_name):
+                ignored.extend(ignore_rules)
+
+        # Remove ignored breaking changes from list of reportable changes
+        bc_copy = deepcopy(self.breaking_changes)
+        for bc in bc_copy:
+            _, bc_type, module_name, *args = bc
             class_name = args[0] if args else None
             function_name = args[1] if len(args) > 1 else None
-            if (bc_type, module_name) in ignored or \
-                    (bc_type, module_name, class_name) in ignored or \
-                    (bc_type, module_name, class_name, function_name) in ignored:
-                continue
-            reportable_changes.append(bc)
-        return reportable_changes
+            parameter_name = args[2] if len(args) > 2 else None
+
+            for rule in ignored:
+                suppression = Suppression(*rule)
+
+                if suppression.parameter_or_property_name is not None:
+                    # If the ignore rule is for a property or parameter, we should check up to that level on the original breaking change
+                    if self.match((bc_type, module_name, class_name, function_name, parameter_name), suppression):
+                        self.breaking_changes.remove(bc)
+                        break
+                elif self.match((bc_type, module_name, class_name, function_name), suppression):
+                    self.breaking_changes.remove(bc)
+                    break
 
     def report_changes(self) -> None:
         ignore_changes = self.ignore if self.ignore else IGNORE_BREAKING_CHANGES
-        if self.package_name in ignore_changes:
-            self.breaking_changes = self.get_reportable_breaking_changes(ignore_changes)
+        self.get_reportable_breaking_changes(ignore_changes)
 
         # If there are no breaking changes after the ignore check, return early
         if not self.breaking_changes:

scripts/breaking_changes_checker/tests/test_breaking_changes.py

@@ -24,8 +24,8 @@ def format_breaking_changes(breaking_changes):
     "(RemovedOrRenamedInstanceAttribute): The model or publicly exposed class 'azure.storage.queue.Metrics' had its instance variable 'retention_policy' deleted or renamed in the current version",
     "(RemovedOrRenamedInstanceAttribute): The client 'azure.storage.queue.QueueClient' had its instance variable 'queue_name' deleted or renamed in the current version",
     "(ChangedParameterKind): The class 'azure.storage.queue.QueueClient' method 'from_queue_url' had its parameter 'credential' changed from 'positional_or_keyword' to 'keyword_only' in the current version",
-    "(AddedPositionalParam): The 'azure.storage.queue.QueueClient method 'get_queue_access_policy' had a 'positional_or_keyword' parameter 'queue_name' inserted in the current version",
-    "(RemovedOrRenamedPositionalParam): The 'azure.storage.queue.QueueClient method 'set_queue_access_policy' had its 'positional_or_keyword' parameter 'signed_identifiers' deleted or renamed in the current version",
+    "(AddedPositionalParam): The 'azure.storage.queue.QueueClient' method 'get_queue_access_policy' had a 'positional_or_keyword' parameter 'queue_name' inserted in the current version",
+    "(RemovedOrRenamedPositionalParam): The 'azure.storage.queue.QueueClient' method 'set_queue_access_policy' had its parameter 'signed_identifiers' of kind 'positional_or_keyword' deleted or renamed in the current version",
     "(ChangedParameterDefaultValue): The class 'azure.storage.queue.QueueClient' method 'set_queue_metadata' had its parameter 'metadata' default value changed from 'None' to ''",
     "(RemovedOrRenamedClassMethod): The 'azure.storage.queue.QueueSasPermissions' method 'from_string' was deleted or renamed in the current version",
     "(RemovedFunctionKwargs): The class 'azure.storage.queue.QueueServiceClient' method 'set_service_properties' changed from accepting keyword arguments to not accepting them in the current version",
@@ -35,13 +35,13 @@ def format_breaking_changes(breaking_changes):
     "(ChangedParameterDefaultValue): The publicly exposed function 'azure.storage.queue.generate_queue_sas' had its parameter 'permission' default value changed from 'None' to ''",
     "(ChangedParameterKind): The function 'azure.storage.queue.generate_queue_sas' had its parameter 'ip' changed from 'positional_or_keyword' to 'keyword_only' in the current version",
     "(AddedPositionalParam): The function 'azure.storage.queue.generate_queue_sas' had a 'positional_or_keyword' parameter 'conn_str' inserted in the current version",
-    "(RemovedOrRenamedPositionalParam): The function 'azure.storage.queue.generate_queue_sas' had its 'positional_or_keyword' parameter 'account_name' deleted or renamed in the current version",
+    "(RemovedOrRenamedPositionalParam): The function 'azure.storage.queue.generate_queue_sas' had its parameter 'account_name' of kind 'positional_or_keyword' deleted or renamed in the current version",
     "(RemovedOrRenamedModuleLevelFunction): The publicly exposed function 'azure.storage.queue.generate_account_sas' was deleted or renamed in the current version",
     "(ChangedParameterKind): The class 'azure.storage.queue.aio.QueueClient' method 'from_queue_url' had its parameter 'credential' changed from 'positional_or_keyword' to 'keyword_only' in the current version",
     "(RemovedParameterDefaultValue): The class 'azure.storage.queue.aio.QueueClient' method 'update_message' had default value 'None' removed from its parameter 'pop_receipt' in the current version",
     "(ChangedFunctionKind): The class 'azure.storage.queue.aio.QueueServiceClient' method 'get_service_stats' changed from 'asynchronous' to 'synchronous' in the current version.",
+    "(ChangedParameterOrdering): The class 'azure.storage.queue.QueueClient' method 'from_connection_string' had its parameters re-ordered from '['conn_str', 'queue_name', 'credential', 'kwargs']' to '['queue_name', 'conn_str', 'credential', 'kwargs']' in the current version",
     "(ChangedParameterOrdering): The class 'azure.storage.queue.aio.QueueClient' method 'from_connection_string' had its parameters re-ordered from '['conn_str', 'queue_name', 'credential', 'kwargs']' to '['queue_name', 'conn_str', 'credential', 'kwargs']' in the current version",
-    "(ChangedParameterOrdering): The class 'azure.storage.queue.QueueClient' method 'from_connection_string' had its parameters re-ordered from '['conn_str', 'queue_name', 'credential', 'kwargs']' to '['queue_name', 'conn_str', 'credential', 'kwargs']' in the current version"
 ]
 
 
@@ -86,6 +86,30 @@ def test_ignore_checks():
     assert changes == expected_msg
 
 
+def test_ignore_with_wildcard_checks():
+    with open(os.path.join(os.path.dirname(__file__), "test_stable.json"), "r") as fd:
+        stable = json.load(fd)
+    with open(os.path.join(os.path.dirname(__file__), "test_current.json"), "r") as fd:
+        current = json.load(fd)
+    diff = jsondiff.diff(stable, current)
+
+    ignore = {
+        "azure-storage-queue": [
+            ("ChangedParameterOrdering", "*", "*", "from_connection_string"),
+            ("ChangedFunctionKind", "azure.storage.queue.aio", "QueueServiceClient", "get_service_stats"),
+        ]
+    }
+
+    bc = BreakingChangesTracker(stable, current, diff, "azure-storage-queue", ignore=ignore)
+    bc.run_checks()
+
+    changes = bc.report_changes()
+
+    expected_msg = format_breaking_changes(EXPECTED[:-3])
+    assert len(bc.breaking_changes)+3 == len(EXPECTED)
+    assert changes == expected_msg
+
+
 def test_replace_all_params():
     current = {
         "azure.ai.formrecognizer": {
@@ -157,10 +181,10 @@ def test_replace_all_params():
     }
 
     EXPECTED = [
-        "(RemovedOrRenamedPositionalParam): The 'azure.ai.formrecognizer.class_name method 'one' had its 'positional_or_keyword' parameter 'testing' deleted or renamed in the current version",
-        "(RemovedOrRenamedPositionalParam): The 'azure.ai.formrecognizer.class_name method 'two' had its 'positional_or_keyword' parameter 'testing2' deleted or renamed in the current version",
-        "(RemovedOrRenamedPositionalParam): The function 'azure.ai.formrecognizer.my_function_name' had its 'positional_or_keyword' parameter 'testing' deleted or renamed in the current version",
-        "(RemovedOrRenamedPositionalParam): The function 'azure.ai.formrecognizer.my_function_name' had its 'positional_or_keyword' parameter 'testing2' deleted or renamed in the current version"
+        "(RemovedOrRenamedPositionalParam): The 'azure.ai.formrecognizer.class_name' method 'one' had its parameter 'testing' of kind 'positional_or_keyword' deleted or renamed in the current version",
+        "(RemovedOrRenamedPositionalParam): The 'azure.ai.formrecognizer.class_name' method 'two' had its parameter 'testing2' of kind 'positional_or_keyword' deleted or renamed in the current version",
+        "(RemovedOrRenamedPositionalParam): The function 'azure.ai.formrecognizer.my_function_name' had its parameter 'testing' of kind 'positional_or_keyword' deleted or renamed in the current version",
+        "(RemovedOrRenamedPositionalParam): The function 'azure.ai.formrecognizer.my_function_name' had its parameter 'testing2' of kind 'positional_or_keyword' deleted or renamed in the current version"
     ]
 
     diff = jsondiff.diff(stable, current)