Auto merge of #14051 - birryree:tidy-check-buildbot-steps, r=aneeshusa

Adding linting checks for buildbot_steps.yml

This pull request adds some tidy checks around YAML files, and specifically `buildbot_steps.yml`.

Tidy checks added:

* YAML files are checked for well-formedness/parse-ability
* Whether a YAML file has duplicate keys
* Whether a `buildbot_steps.yml` file contains only mappings to list-of-strings.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [x] These changes fix #13838 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

…ing checking for correct mappings and duplicate YAML keys. Added unit tests to test_tidy.py.

<!-- Reviewable:start -->

---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14051)
<!-- Reviewable:end -->

1 files changed, 58 insertions, 4 deletions

diff --git a/python/tidy/servo_tidy/tidy.py b/python/tidy/servo_tidy/tidy.py
index 682100f6e01..b40bdd67194 100644
--- a/python/tidy/servo_tidy/tidy.py
+++ b/python/tidy/servo_tidy/tidy.py
@@ -19,6 +19,7 @@ import subprocess
 import sys
 import colorama
 import toml
+import yaml
 
 from licenseck import MPL, APACHE, COPYRIGHT, licenses_toml, licenses_dep_toml
 
@@ -47,7 +48,8 @@ COMMENTS = ["// ", "# ", " *", "/* "]
 # File patterns to include in the non-WPT tidy check.
 FILE_PATTERNS_TO_CHECK = ["*.rs", "*.rc", "*.cpp", "*.c",
                           "*.h", "Cargo.lock", "*.py", "*.sh",
-                          "*.toml", "*.webidl", "*.json", "*.html"]
+                          "*.toml", "*.webidl", "*.json", "*.html",
+                          "*.yml"]
 
 # File patterns that are ignored for all tidy and lint checks.
 FILE_PATTERNS_TO_IGNORE = ["*.#*", "*.pyc", "fake-ld.sh"]
@@ -178,7 +180,7 @@ def is_apache_licensed(header):
 
 
 def check_license(file_name, lines):
-    if any(file_name.endswith(ext) for ext in (".toml", ".lock", ".json", ".html")) or \
+    if any(file_name.endswith(ext) for ext in (".yml", ".toml", ".lock", ".json", ".html")) or \
        config["skip-check-licenses"]:
         raise StopIteration
 
@@ -216,7 +218,7 @@ def check_modeline(file_name, lines):
 
 
 def check_length(file_name, idx, line):
-    if any(file_name.endswith(ext) for ext in (".lock", ".json", ".html", ".toml")) or \
+    if any(file_name.endswith(ext) for ext in (".yml", ".lock", ".json", ".html", ".toml")) or \
        config["skip-check-length"]:
         raise StopIteration
 
@@ -675,6 +677,58 @@ def check_webidl_spec(file_name, contents):
     yield (0, "No specification link found.")
 
 
+def duplicate_key_yaml_constructor(loader, node, deep=False):
+    mapping = {}
+    for key_node, value_node in node.value:
+        key = loader.construct_object(key_node, deep=deep)
+        if key in mapping:
+            raise KeyError(key)
+        value = loader.construct_object(value_node, deep=deep)
+        mapping[key] = value
+    return loader.construct_mapping(node, deep)
+
+
+def lint_buildbot_steps_yaml(mapping):
+    # Check for well-formedness of contents
+    # A well-formed buildbot_steps.yml should be a map to list of strings
+    for k in mapping.keys():
+        if not isinstance(mapping[k], list):
+            raise ValueError("Key '{}' maps to type '{}', but list expected".format(k, type(mapping[k]).__name__))
+
+        # check if value is a list of strings
+        for item in itertools.ifilter(lambda i: not isinstance(i, str), mapping[k]):
+            raise ValueError("List mapped to '{}' contains non-string element".format(k))
+
+
+class SafeYamlLoader(yaml.SafeLoader):
+    """Subclass of yaml.SafeLoader to avoid mutating the global SafeLoader."""
+    pass
+
+
+def check_yaml(file_name, contents):
+    if not file_name.endswith("buildbot_steps.yml"):
+        raise StopIteration
+
+    # YAML specification doesn't explicitly disallow
+    # duplicate keys, but they shouldn't be allowed in
+    # buildbot_steps.yml as it could lead to confusion
+    SafeYamlLoader.add_constructor(
+        yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG,
+        duplicate_key_yaml_constructor
+    )
+
+    try:
+        contents = yaml.load(contents, Loader=SafeYamlLoader)
+        lint_buildbot_steps_yaml(contents)
+    except yaml.YAMLError as e:
+        line = e.problem_mark.line + 1 if hasattr(e, 'problem_mark') else None
+        yield (line, e)
+    except KeyError as e:
+        yield (None, "Duplicated Key ({})".format(e.message))
+    except ValueError as e:
+        yield (None, e.message)
+
+
 def check_for_possible_duplicate_json_keys(key_value_pairs):
     keys = [x[0] for x in key_value_pairs]
     seen_keys = set()
@@ -964,7 +1018,7 @@ def scan(only_changed_files=False, progress=True):
     directory_errors = check_directory_files(config['check_ext'])
     # standard checks
     files_to_check = filter_files('.', only_changed_files, progress)
-    checking_functions = (check_flake8, check_lock, check_webidl_spec, check_json)
+    checking_functions = (check_flake8, check_lock, check_webidl_spec, check_json, check_yaml)
     line_checking_functions = (check_license, check_by_line, check_toml, check_shell,
                                check_rust, check_spec, check_modeline)
     file_errors = collect_errors_for_files(files_to_check, checking_functions, line_checking_functions)