pre-upload: support options for default enabled flags [reland]
We pull out hook options when the hook is explicitly enabled, but not
when it's implicitly enabled by default. We haven't had any hooks as
those take options before so no one noticed.
The previous version had a logic error and enabled all non-common
hooks by default. This adds more unittests to verify that doesn't
happen, and then fixes the logic to only auto-enable common hooks.
BUG=b:162982485
TEST=unittests pass
Change-Id: Ic3aac6f8469579cdd9cb47aa058f97c3b0389e27
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/repohooks/+/2368044
Reviewed-by: Bernie Thompson <bhthompson@chromium.org>
Commit-Queue: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
diff --git a/pre-upload.py b/pre-upload.py
index bdacbec..e06b808 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -2063,23 +2063,34 @@
valid_keys = set(_HOOK_FLAGS.keys())
hooks = _HOOK_FLAGS.copy()
- options = config.options(SECTION) if config.has_section(SECTION) else []
+ hook_overrides = set(
+ config.options(SECTION) if config.has_section(SECTION) else [])
+
+ unknown_keys = hook_overrides - valid_keys
+ if unknown_keys:
+ raise ValueError(f'{_CONFIG_FILE}: [{SECTION}]: unknown keys: '
+ f'{unknown_keys}')
enable_flags = []
disable_flags = []
- for flag in options:
- if flag not in valid_keys:
- raise ValueError('Error: unknown key "%s" in hook section of "%s"' %
- (flag, _CONFIG_FILE))
+ for flag in valid_keys:
+ if flag in hook_overrides:
+ try:
+ enabled = config.getboolean(SECTION, flag)
+ except ValueError as e:
+ raise ValueError('Error: parsing flag "%s" in "%s" failed: %s' %
+ (flag, _CONFIG_FILE, e))
+ elif hooks[flag] in _COMMON_HOOKS:
+ # Enable common hooks by default so we process custom options below.
+ enabled = True
+ else:
+ # All other hooks we left as a tristate. We use this below for a few
+ # hooks to control default behavior.
+ enabled = None
- try:
- enabled = config.getboolean(SECTION, flag)
- except ValueError as e:
- raise ValueError('Error: parsing flag "%s" in "%s" failed: %s' %
- (flag, _CONFIG_FILE, e))
if enabled:
enable_flags.append(flag)
- else:
+ elif enabled is not None:
disable_flags.append(flag)
# See if this hook has custom options.
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index 7613973..0702f54 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -8,6 +8,7 @@
from __future__ import print_function
+import configparser
import datetime
import os
import sys
@@ -1862,5 +1863,112 @@
self.assertTrue(called)
+class OverrideHooksProcessing(PreUploadTestCase):
+ """Verify _get_override_hooks processing."""
+
+ @staticmethod
+ def parse(data):
+ """Helper to create a config & parse it."""
+ cfg = configparser.ConfigParser()
+ cfg.read_string(data)
+ return pre_upload._get_override_hooks(cfg)
+
+ def testHooks(self):
+ """Verify we reject unknown hook names (e.g. typos)."""
+ with self.assertRaises(ValueError) as e:
+ self.parse("""
+[Hook Overrides]
+foobar: true
+""")
+ self.assertIn('foobar', str(e.exception))
+
+ def testImplicitDisable(self):
+ """Verify non-common hooks aren't enabled by default."""
+ enabled, _ = self.parse('')
+ self.assertNotIn(pre_upload._run_checkpatch, enabled)
+
+ def testExplicitDisable(self):
+ """Verify hooks disabled are disabled."""
+ _, disabled = self.parse("""
+[Hook Overrides]
+tab_check: false
+""")
+ self.assertIn(pre_upload._check_no_tabs, disabled)
+
+ def testExplicitEnable(self):
+ """Verify hooks enabled are enabled."""
+ enabled, _ = self.parse("""
+[Hook Overrides]
+tab_check: true
+""")
+ self.assertIn(pre_upload._check_no_tabs, enabled)
+
+ def testOptions(self):
+ """Verify hook options are loaded."""
+ enabled, _ = self.parse("""
+[Hook Overrides Options]
+keyword_check: --kw
+""")
+ for func in enabled:
+ if func.__name__ == 'keyword_check':
+ self.assertIn('options', func.keywords)
+ self.assertEqual(func.keywords['options'], ['--kw'])
+ break
+ else:
+ self.fail('could not find "keyword_check" enabled hook')
+
+ def testSignOffField(self):
+ """Verify signoff field handling."""
+ # Enforce no s-o-b by default.
+ enabled, disabled = self.parse('')
+ self.assertIn(pre_upload._check_change_has_no_signoff_field, enabled)
+ self.assertNotIn(pre_upload._check_change_has_signoff_field, enabled)
+ self.assertNotIn(pre_upload._check_change_has_signoff_field, disabled)
+
+ # If disabled, don't enforce either policy.
+ enabled, disabled = self.parse("""
+[Hook Overrides]
+signoff_check: false
+""")
+ self.assertNotIn(pre_upload._check_change_has_no_signoff_field, enabled)
+ self.assertNotIn(pre_upload._check_change_has_signoff_field, enabled)
+ self.assertIn(pre_upload._check_change_has_signoff_field, disabled)
+
+ # If enabled, require s-o-b.
+ enabled, disabled = self.parse("""
+[Hook Overrides]
+signoff_check: true
+""")
+ self.assertNotIn(pre_upload._check_change_has_no_signoff_field, enabled)
+ self.assertIn(pre_upload._check_change_has_signoff_field, enabled)
+ self.assertNotIn(pre_upload._check_change_has_signoff_field, disabled)
+
+ def testBranchField(self):
+ """Verify branch field enabling."""
+ # Should be disabled by default.
+ enabled, disabled = self.parse('')
+ self.assertIn(pre_upload._check_change_has_no_branch_field, enabled)
+ self.assertNotIn(pre_upload._check_change_has_branch_field, enabled)
+ self.assertNotIn(pre_upload._check_change_has_branch_field, disabled)
+
+ # Should be disabled if requested.
+ enabled, disabled = self.parse("""
+[Hook Overrides]
+branch_check: false
+""")
+ self.assertIn(pre_upload._check_change_has_no_branch_field, enabled)
+ self.assertNotIn(pre_upload._check_change_has_branch_field, enabled)
+ self.assertIn(pre_upload._check_change_has_branch_field, disabled)
+
+ # Should be enabled if requested.
+ enabled, disabled = self.parse("""
+[Hook Overrides]
+branch_check: true
+""")
+ self.assertNotIn(pre_upload._check_change_has_no_branch_field, enabled)
+ self.assertIn(pre_upload._check_change_has_branch_field, enabled)
+ self.assertNotIn(pre_upload._check_change_has_branch_field, disabled)
+
+
if __name__ == '__main__':
cros_test_lib.main(module=__name__)
