From c1fd5bdad014caf258143c8d8eda23097e36a756 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 3 Mar 2026 13:04:29 +0200 Subject: [PATCH] feat: wildcard conventional-title and conditional merge requirements - Support '*' wildcard for conventional-title to accept any type while enforcing the Conventional Commits format - Make merge requirements in welcome comment conditional: - LGTM count shown only when minimum_lgtm > 0 - Verified shown only when verified_job is enabled - Numbering adjusts dynamically - Use constants (WIP_STR, HOLD_LABEL_STR, HAS_CONFLICTS_LABEL_STR) in merge requirements instead of hardcoded strings - Conditional failure message in wildcard mode Closes #1013 --- examples/config.yaml | 3 + webhook_server/config/schema.yaml | 3 +- .../libs/handlers/pull_request_handler.py | 38 ++++--- .../libs/handlers/runner_handler.py | 32 ++++-- .../tests/test_pull_request_handler.py | 99 ++++++++++++++----- webhook_server/tests/test_runner_handler.py | 48 +++++++++ 6 files changed, 175 insertions(+), 48 deletions(-) diff --git a/examples/config.yaml b/examples/config.yaml index 238d48bf..dcd88a7b 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -198,6 +198,9 @@ repositories: # - my-title: custom type example # - hotfix(api): resolve production issue # + # Use "*" to accept any type while enforcing the format + # conventional-title: "*" + # # Resources: https://www.conventionalcommits.org/en/v1.0.0/ conventional-title: "feat,fix,build,chore,ci,docs,style,refactor,perf,test,revert" branch-protection: diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index 9f70bcc3..6a36b0fc 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -358,6 +358,7 @@ properties: type: string description: | Comma-separated list of allowed commit types for Conventional Commits validation. + Use "*" to accept any type while still enforcing the format ([optional scope]: ). Enforces Conventional Commits v1.0.0 specification: [optional scope]: @@ -370,7 +371,7 @@ properties: Examples: my-title, hotfix, release, custom Format requirements: - - Type from configured list + - Type from configured list (or any type if wildcard "*" is used) - Optional scope in parentheses: (scope) - Optional breaking change indicator: ! - Mandatory colon + space: ": " diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index c181d8e7..391d39c4 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -315,11 +315,7 @@ def _prepare_welcome_comment(self) -> str: This PR will be automatically approved when the following conditions are met: -1. **Approval**: `/approve` from at least one approver -2. **LGTM Count**: Minimum {self.github_webhook.minimum_lgtm} `/lgtm` from reviewers -3. **Status Checks**: All required status checks must pass -{self._prepare_no_blockers_requirement} -5. **Verified**: PR must be marked as verified (if verification is enabled) +{self._prepare_merge_requirements} ### 📊 Review Process @@ -496,23 +492,35 @@ def _prepare_tips_section(self) -> str: return "\n".join(tips) @property - def _prepare_no_blockers_requirement(self) -> str: - """Prepare the No Blockers merge requirement line. + def _prepare_merge_requirements(self) -> str: + """Prepare the merge requirements section for the welcome comment. - Only mentions labels that are enabled. + Dynamically builds numbered list based on enabled features. """ - blockers: list[str] = [] + requirements: list[str] = [] - if self.labels_handler.is_label_enabled(WIP_STR): - blockers.append("WIP") + requirements.append("**Approval**: `/approve` from at least one approver") + + if self.github_webhook.minimum_lgtm > 0: + requirements.append(f"**LGTM Count**: Minimum {self.github_webhook.minimum_lgtm} `/lgtm` from reviewers") + requirements.append("**Status Checks**: All required status checks must pass") + + # No blockers (WIP, hold, conflict labels) + blockers: list[str] = [] + if self.labels_handler.is_label_enabled(WIP_STR): + blockers.append(WIP_STR) if self.labels_handler.is_label_enabled(HOLD_LABEL_STR): - blockers.append("hold") + blockers.append(HOLD_LABEL_STR) + if self.labels_handler.is_label_enabled(HAS_CONFLICTS_LABEL_STR): + blockers.append(HAS_CONFLICTS_LABEL_STR) + blockers_text = f"No {', '.join(blockers)} labels" if blockers else "No blocker labels" + requirements.append(f"**No Blockers**: {blockers_text} and PR must be mergeable (no conflicts)") - # Conflict labels (has-conflicts) are always shown since they're fundamental - blockers.append("conflict") + if self.github_webhook.verified_job: + requirements.append("**Verified**: PR must be marked as verified") - return f"4. **No Blockers**: No {', '.join(blockers)} labels" + return "\n".join(f"{i}. {req}" for i, req in enumerate(requirements, 1)) @property def _prepare_automerge_command_line(self) -> str: diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index ddb04bff..8fbc3f23 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -455,14 +455,34 @@ async def run_conventional_title_check(self, pull_request: PullRequest) -> None: self.logger.info(f"{self.log_prefix} Check run is in progress, re-running {CONVENTIONAL_TITLE_STR}.") await self.check_run_handler.set_check_in_progress(name=CONVENTIONAL_TITLE_STR) - allowed_names = [name.strip() for name in self.github_webhook.conventional_title.split(",") if name.strip()] title = pull_request.title + is_wildcard = self.github_webhook.conventional_title.strip() == "*" - self.logger.debug(f"{self.log_prefix} Conventional title check for title: {title}, allowed: {allowed_names}") - # Use generator expression instead of list comprehension inside any() for efficiency - if any(re.match(rf"^{re.escape(_name)}(\([^)]+\))?!?: .+", title) for _name in allowed_names): + if is_wildcard: + allowed_names: list[str] = [] + title_valid = bool(re.match(r"^[\w-]+(\([^)]+\))?!?: .+", title)) + self.logger.debug(f"{self.log_prefix} Conventional title check (wildcard) for title: {title}") + else: + allowed_names = [name.strip() for name in self.github_webhook.conventional_title.split(",") if name.strip()] + title_valid = any(re.match(rf"^{re.escape(_name)}(\([^)]+\))?!?: .+", title) for _name in allowed_names) + self.logger.debug( + f"{self.log_prefix} Conventional title check for title: {title}, allowed: {allowed_names}" + ) + + if title_valid: await self.check_run_handler.set_check_success(name=CONVENTIONAL_TITLE_STR, output=output) else: + if is_wildcard: + types_display = "any valid type (wildcard `*` configured)" + else: + types_display = ", ".join(f"`{t}`" for t in allowed_names) + + type_rule = ( + "Type can be any valid token (wildcard `*` configured)" + if is_wildcard + else "Type must be one of the configured types" + ) + output["title"] = "❌ Conventional Title" output["summary"] = "Conventional Commit Format Violation" output["text"] = f"""## Conventional Commits Validation Failed @@ -476,7 +496,7 @@ async def run_conventional_title_check(self, pull_request: PullRequest) -> None: ``` **Configured types for this repository:** -{", ".join(f"`{t}`" for t in allowed_names)} +{types_display} **Valid examples:** - `feat: add user authentication` @@ -486,7 +506,7 @@ async def run_conventional_title_check(self, pull_request: PullRequest) -> None: - `docs: update installation guide` **Format rules:** -- Type must be one of the configured types +- {type_rule} - Optional scope in parentheses: `(scope)` - Optional breaking change indicator: `!` - **Mandatory**: colon followed by space `: ` diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 50678600..eff91f1e 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -478,44 +478,91 @@ def test_prepare_retest_welcome_comment(self, pull_request_handler: PullRequestH assert TOX_STR in result assert "pre-commit" in result - def test_prepare_no_blockers_requirement_all_enabled(self, pull_request_handler: PullRequestHandler) -> None: - """Test no blockers requirement when all labels are enabled.""" - # Default: enabled_labels is None, so all are enabled - # Mock is_label_enabled to return True for all labels + def test_prepare_merge_requirements_all_enabled(self, pull_request_handler: PullRequestHandler) -> None: + """Test merge requirements when all features are enabled.""" pull_request_handler.labels_handler.is_label_enabled = Mock(return_value=True) - result = pull_request_handler._prepare_no_blockers_requirement - assert "No WIP, hold, conflict labels" in result + pull_request_handler.github_webhook.minimum_lgtm = 2 + pull_request_handler.github_webhook.verified_job = True + result = pull_request_handler._prepare_merge_requirements + assert "1. **Approval**" in result + assert "2. **LGTM Count**: Minimum 2" in result + assert "3. **Status Checks**" in result + assert ( + "4. **No Blockers**: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)" in result + ) + assert "5. **Verified**" in result - def test_prepare_no_blockers_requirement_wip_disabled(self, pull_request_handler: PullRequestHandler) -> None: - """Test no blockers requirement when wip is disabled.""" - # Mock is_label_enabled: wip disabled, hold enabled + def test_prepare_merge_requirements_lgtm_zero(self, pull_request_handler: PullRequestHandler) -> None: + """Test merge requirements when minimum_lgtm is 0 (LGTM line omitted).""" + pull_request_handler.labels_handler.is_label_enabled = Mock(return_value=True) + pull_request_handler.github_webhook.minimum_lgtm = 0 + pull_request_handler.github_webhook.verified_job = True + result = pull_request_handler._prepare_merge_requirements + assert "LGTM Count" not in result + assert "1. **Approval**" in result + assert "2. **Status Checks**" in result + assert "3. **No Blockers**" in result + assert "4. **Verified**" in result + + def test_prepare_merge_requirements_verified_disabled(self, pull_request_handler: PullRequestHandler) -> None: + """Test merge requirements when verified_job is disabled.""" + pull_request_handler.labels_handler.is_label_enabled = Mock(return_value=True) + pull_request_handler.github_webhook.minimum_lgtm = 1 + pull_request_handler.github_webhook.verified_job = False + result = pull_request_handler._prepare_merge_requirements + assert "Verified" not in result + assert "1. **Approval**" in result + assert "2. **LGTM Count**" in result + assert "3. **Status Checks**" in result + assert "4. **No Blockers**" in result + + def test_prepare_merge_requirements_wip_disabled(self, pull_request_handler: PullRequestHandler) -> None: + """Test merge requirements blockers when wip is disabled.""" pull_request_handler.labels_handler.is_label_enabled = Mock(side_effect=lambda label: label != WIP_STR) - result = pull_request_handler._prepare_no_blockers_requirement - assert "WIP" not in result + pull_request_handler.github_webhook.minimum_lgtm = 1 + pull_request_handler.github_webhook.verified_job = False + result = pull_request_handler._prepare_merge_requirements + assert "wip" not in result assert "hold" in result - assert "conflict" in result + assert "has-conflicts" in result - def test_prepare_no_blockers_requirement_hold_disabled(self, pull_request_handler: PullRequestHandler) -> None: - """Test no blockers requirement when hold is disabled.""" - # Mock is_label_enabled: hold disabled, wip enabled + def test_prepare_merge_requirements_hold_disabled(self, pull_request_handler: PullRequestHandler) -> None: + """Test merge requirements blockers when hold is disabled.""" pull_request_handler.labels_handler.is_label_enabled = Mock(side_effect=lambda label: label != HOLD_LABEL_STR) - result = pull_request_handler._prepare_no_blockers_requirement - assert "WIP" in result + pull_request_handler.github_webhook.minimum_lgtm = 1 + pull_request_handler.github_webhook.verified_job = False + result = pull_request_handler._prepare_merge_requirements + assert "wip" in result assert "hold" not in result - assert "conflict" in result + assert "has-conflicts" in result - def test_prepare_no_blockers_requirement_both_disabled(self, pull_request_handler: PullRequestHandler) -> None: - """Test no blockers requirement when both wip and hold are disabled.""" - # Mock is_label_enabled: both wip and hold disabled + def test_prepare_merge_requirements_both_blockers_disabled(self, pull_request_handler: PullRequestHandler) -> None: + """Test merge requirements blockers when both wip and hold are disabled.""" pull_request_handler.labels_handler.is_label_enabled = Mock( side_effect=lambda label: label not in (WIP_STR, HOLD_LABEL_STR) ) - result = pull_request_handler._prepare_no_blockers_requirement - assert "WIP" not in result + pull_request_handler.github_webhook.minimum_lgtm = 0 + pull_request_handler.github_webhook.verified_job = False + result = pull_request_handler._prepare_merge_requirements + assert "wip" not in result assert "hold" not in result - assert "conflict" in result - # Only conflict should be present - assert "No conflict labels" in result + assert "has-conflicts" in result + assert "No has-conflicts labels and PR must be mergeable (no conflicts)" in result + + def test_prepare_merge_requirements_minimal(self, pull_request_handler: PullRequestHandler) -> None: + """Test merge requirements with minimum features (no LGTM, no verified).""" + pull_request_handler.labels_handler.is_label_enabled = Mock( + side_effect=lambda label: label not in (WIP_STR, HOLD_LABEL_STR) + ) + pull_request_handler.github_webhook.minimum_lgtm = 0 + pull_request_handler.github_webhook.verified_job = False + result = pull_request_handler._prepare_merge_requirements + assert "LGTM Count" not in result + assert "Verified" not in result + # Should have exactly 3 items numbered 1-3 + assert "1. **Approval**" in result + assert "2. **Status Checks**" in result + assert "3. **No Blockers**" in result @pytest.mark.asyncio async def test_label_all_opened_pull_requests_merge_state_after_merged( diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 0c3e66f3..fc8739b2 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -647,6 +647,54 @@ async def test_run_conventional_title_check_in_progress( mock_set_progress.assert_awaited_once_with(name=CONVENTIONAL_TITLE_STR) mock_set_success.assert_awaited_once() + @pytest.mark.asyncio + @pytest.mark.parametrize( + "title,should_pass,reason", + [ + ("feat: add feature", True, "standard type with wildcard"), + ("custom-type: something new", True, "custom type accepted by wildcard"), + ("anything: goes here", True, "any word accepted as type"), + ("fix(scope): with scope", True, "scoped type with wildcard"), + ("type!: breaking change", True, "breaking change with wildcard"), + ("no-colon-space here", False, "missing colon-space separator"), + (": empty type", False, "empty type name"), + ("feat:", False, "missing description"), + ], + ) + async def test_conventional_title_wildcard( + self, runner_handler: RunnerHandler, mock_pull_request: Mock, title: str, should_pass: bool, reason: str + ) -> None: + """Test Conventional Commits validation with wildcard '*' accepting any type.""" + runner_handler.github_webhook.conventional_title = "*" + mock_pull_request.title = title + + with patch.object( + runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) + ): + with patch.object( + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() + ) as mock_set_progress: + with patch.object( + runner_handler.check_run_handler, "set_check_success", new=AsyncMock() + ) as mock_set_success: + with patch.object( + runner_handler.check_run_handler, "set_check_failure", new=AsyncMock() + ) as mock_set_failure: + await runner_handler.run_conventional_title_check(mock_pull_request) + + mock_set_progress.assert_awaited_once_with(name=CONVENTIONAL_TITLE_STR) + + if should_pass: + assert mock_set_success.await_count == 1, ( + f"Expected '{title}' to pass wildcard validation ({reason}), but it failed" + ) + mock_set_failure.assert_not_awaited() + else: + assert mock_set_failure.await_count == 1, ( + f"Expected '{title}' to fail wildcard validation ({reason}), but it passed" + ) + mock_set_success.assert_not_awaited() + @pytest.mark.asyncio async def test_is_branch_exists(self, runner_handler: RunnerHandler) -> None: """Test is_branch_exists."""