diff --git a/README.md b/README.md index 9ff8ef5c..86a00960 100644 --- a/README.md +++ b/README.md @@ -5,6 +5,110 @@ This bot is meant to help identify issues with a CPython pull request. +## Which repositories it runs on + +Most of bedevere assumes CPython's workflow: `gh-NNNNN` issue numbers, +`Misc/NEWS.d` entries, `X.Y` maintenance branches and the awaiting-review +labels. So each repository the app is installed on runs only the features +enabled for it in [`bedevere/config.py`](bedevere/config.py). + +### How it works + +Every webhook goes to `dispatch()` in `bedevere/__main__.py`. It looks up the +repository the event came from with `config.features_for()`, then passes the +event to the router of each enabled feature, in the order of +`FEATURE_ROUTERS`. Features that aren't enabled never see the event. + +- `python/cpython` runs every feature. +- A `python` org repository with no entry in `REPO_FEATURES` runs only + `policy`, the AI policy labels. This is a safe default for a repository + added to the installation before anyone has decided its features; it is + not a limit, and an entry can turn on any of the features below. +- A repository outside the `python` org runs nothing unless it has an entry. + Bedevere is meant for python org repositories only. +- Bedevere logs the first event it gets from a repository with no entry. +- Repository names are matched without regard to case. + +| Feature | What it does | +| ----------- | ---------------------------------------------------------------------------- | +| `backport` | `needs backport to X.Y` labels, the `[X.Y]` title status, copies labels to backports | +| `close_pr` | Closes PRs that merge a maintenance branch into `main` | +| `filepaths` | `docs` and `tests` labels based on the files changed | +| `gh_issue` | The `bedevere/issue-number` status for `gh-NNNNN` in the title | +| `news` | The `bedevere/news` status for `Misc/NEWS.d` entries | +| `policy` | Comments on issues and PRs labeled `AI-needs-explanation` or `AI-guidelines-not-met`; the second label also closes the item and removes the first | +| `stage` | The awaiting-review stage labels and comments | + +`filepaths` also runs the NEWS check when a PR is opened or updated. It skips +that check when `news` is off, so a repository can have `filepaths` without +`news`. + +### Which features fit a repository + +Features are turned on or off per repository. None of them have per-repository +settings: a feature behaves the same on every repository that runs it. + +| Feature | Fits other python org repositories? | +| ----------- | ----------------------------------- | +| `policy` | Yes. | +| `stage` | Yes, if PRs are reviewed by members of the python org's **Python core** team. A review from anyone else counts as a non-core review, so a repository whose reviewers aren't in that team would leave PRs at `awaiting core review`. Its comments say "A Python core developer". | +| `filepaths` | Partly. It goes by file name, so it works on any repository: when a PR is opened, one that changes only docs files (`.rst`, `.md`, `.nitignore`) gets `docs`, and one that changes only test files (`test_*`, `_test*`), or tests mixed with docs, gets `tests`. But a docs-only PR with no `Misc/NEWS.d` entry also gets CPython's `skip news` label. | +| `close_pr` | Only if `X.Y` branches are maintenance branches that must never be merged into `main`. Its comment links to CPython. | +| `gh_issue`, `news`, `backport` | No. They expect CPython's `gh-NNNNN` titles, `Misc/NEWS.d` entries and backport workflow, and post failing statuses or wrong labels elsewhere. | + +### Setting up a repository + +1. **Agree with the repository's maintainers** that they want bedevere, and + which features from the table above. +2. **If they want more than `policy`, add the repository to `REPO_FEATURES`** + in `bedevere/config.py`, with every feature it should run: + + ```python + REPO_FEATURES = { + "python/cpython": ALL_FEATURES, + "python/devguide": frozenset({"policy", "stage"}), + } + ``` + + Use the lowercase `owner/name`. An entry replaces the default, so list + `policy` too unless the repository shouldn't have the AI policy labels. + Open a pull request; the features take effect once it's merged and + deployed. A repository that wants `policy` only needs no entry. +3. **Create the labels the features use** in the repository (Issues, Labels, + New label). Copy each description from + [CPython's labels](https://github.com/python/cpython/labels), since GitHub + shows it in the label picker and it tells triagers what applying the label + does. The colour doesn't matter to bedevere; copying it only keeps the + labels looking the same across repositories. + - `policy`: `AI-needs-explanation` (bedevere asks the author to explain + the contribution in their own words) and `AI-guidelines-not-met` + (bedevere comments, closes the issue or PR, and removes + `AI-needs-explanation` if it's there). Either label does nothing on a + closed item, and adding `AI-needs-explanation` to an item that already + has `AI-guidelines-not-met` does nothing. People apply these, and + bedevere acts only when the name matches exactly, including case. Until + they exist bedevere does nothing, and a different spelling + (`AI needs explanation`) is ignored without any error. + - `stage`: `awaiting review`, `awaiting core review`, `awaiting changes`, + `awaiting change review` and `awaiting merge`. Bedevere applies these + itself; if one is missing, GitHub creates it with a default colour and + no description. +4. **Add the repository to the installation.** A python org admin opens the + org's settings, GitHub Apps, bedevere, Configure, and adds the repository + under Repository access. If step 2 applies, do this after the deploy, or + the repository runs `policy` only until then. +5. **Check that it works.** Open a test issue, apply `AI-needs-explanation`, + and check that bedevere comments within a minute or so, then delete or + close the issue. With `stage`, open a test PR and check it gets an + `awaiting` label. If nothing happens, check the label names and the app's + Recent Deliveries in its settings. +6. **Tell the repository's triagers** which features are on and what each + label does. + +To change a repository's features later, edit its entry and open a pull +request. To turn bedevere off on a repository, remove the repository from the +installation. + ## What the bot does: - ### Identifies missing GitHub issue numbers in the title If no GitHub issue number is found the status fails and the diff --git a/bedevere/__main__.py b/bedevere/__main__.py index ecf65129..bd545ebb 100644 --- a/bedevere/__main__.py +++ b/bedevere/__main__.py @@ -12,21 +12,39 @@ from gidgethub import apps, routing, sansio from . import backport, close_pr, filepaths, gh_issue, news, policy, stage +from .config import features_for -router = routing.Router( - backport.router, - gh_issue.router, - close_pr.router, - filepaths.router, - news.router, - policy.router, - stage.router, -) +# App-level handlers that run for every event, whatever the repository. +router = routing.Router() + +# Feature routers, in dispatch order. Each repository runs only the features +# that features_for() enables for it. +FEATURE_ROUTERS = { + "backport": backport.router, + "gh_issue": gh_issue.router, + "close_pr": close_pr.router, + "filepaths": filepaths.router, + "news": news.router, + "policy": policy.router, + "stage": stage.router, +} cache = cachetools.LRUCache(maxsize=500) sentry_sdk.init(os.environ.get("SENTRY_DSN")) +async def dispatch(event, gh, **kwargs): + """Dispatch an event to the app-level router and the repo's features.""" + await router.dispatch(event, gh, **kwargs) + repo = event.data.get("repository") + if repo is None: + return + enabled = features_for(repo["full_name"]) + for name, feature_router in FEATURE_ROUTERS.items(): + if name in enabled: + await feature_router.dispatch(event, gh, features=enabled, **kwargs) + + async def main(request): try: body = await request.read() @@ -51,7 +69,7 @@ async def main(request): # Give GitHub some time to reach internal consistency. await asyncio.sleep(1) - await router.dispatch(event, gh, session=session) + await dispatch(event, gh, session=session) try: print("GH requests remaining:", gh.rate_limit.remaining) except AttributeError: diff --git a/bedevere/config.py b/bedevere/config.py new file mode 100644 index 00000000..c138c43b --- /dev/null +++ b/bedevere/config.py @@ -0,0 +1,38 @@ +"""Which bedevere features run on each repository.""" + +import sys + +ALL_FEATURES = frozenset( + {"backport", "close_pr", "filepaths", "gh_issue", "news", "policy", "stage"} +) +DEFAULT_FEATURES = frozenset({"policy"}) +DEFAULT_OWNER = "python" + +# Keys are lowercase "owner/name". Unlisted repos owned by DEFAULT_OWNER get +# DEFAULT_FEATURES, and unlisted repos owned by anyone else get no features. +REPO_FEATURES = { + "python/cpython": ALL_FEATURES, +} + +_logged_unknown_repos = set() + + +def features_for(repo_full_name): + """Return the features enabled for a repository. + + Repositories without an entry are logged the first time they are seen. + """ + repo = repo_full_name.lower() + try: + return REPO_FEATURES[repo] + except KeyError: + owner = repo.partition("/")[0] + features = DEFAULT_FEATURES if owner == DEFAULT_OWNER else frozenset() + if repo not in _logged_unknown_repos: + _logged_unknown_repos.add(repo) + print( + f"{repo_full_name} has no feature config, running only: " + f"{', '.join(sorted(features)) or 'nothing'}", + file=sys.stderr, + ) + return features diff --git a/bedevere/filepaths.py b/bedevere/filepaths.py index 72df27d9..6ee94162 100644 --- a/bedevere/filepaths.py +++ b/bedevere/filepaths.py @@ -10,13 +10,15 @@ @router.register("pull_request", action="opened") @router.register("pull_request", action="synchronize") @router.register("pull_request", action="reopened") -async def check_file_paths(event, gh, *args, **kwargs): +async def check_file_paths(event, gh, *args, features=None, **kwargs): + # Callers that don't pass features (such as tests) get the NEWS check. + run_news = features is None or "news" in features pull_request = event.data["pull_request"] files = await util.files_for_PR(gh, pull_request) filenames = [file["file_name"] for file in files] if event.data["action"] == "opened": labels = await prtype.classify_by_filepaths(gh, pull_request, filenames) - if prtype.Labels.skip_news not in labels: + if run_news and prtype.Labels.skip_news not in labels: await news.check_news(gh, pull_request, files) - else: + elif run_news: await news.check_news(gh, pull_request, files) diff --git a/tests/test___main__.py b/tests/test___main__.py index e9e628fa..4b3c6400 100644 --- a/tests/test___main__.py +++ b/tests/test___main__.py @@ -4,6 +4,7 @@ from gidgethub import sansio from bedevere import __main__ as main +from bedevere import config app_installation_payload = { "installation": { @@ -92,3 +93,60 @@ async def test_repo_installation_added(capfd): f"App installed by {event.data['installation']['account']['login']}, installation_id: {event.data['installation']['id']}" in out ) + + +def pr_opened_event(repo): + return sansio.Event( + { + "action": "opened", + "number": 1, + "pull_request": { + "url": f"https://api.github.com/repos/{repo}/pulls/1", + "statuses_url": f"https://api.github.com/repos/{repo}/statuses/abc", + "issue_url": f"https://api.github.com/repos/{repo}/issues/1", + "title": "Fix a typo", + "body": "", + "user": {"login": "contributor"}, + "author_association": "NONE", + "draft": False, + "base": {"ref": "main"}, + "head": {"ref": "fix-typo", "label": "contributor:fix-typo"}, + }, + "repository": {"full_name": repo}, + }, + event="pull_request", + delivery_id="1", + ) + + +async def test_unconfigured_repo_runs_no_cpython_features(): + # FakeGH has no API methods, so any status, label or comment call fails. + await main.dispatch(pr_opened_event("python/devguide"), FakeGH()) + + +async def test_configured_repo_runs_its_features_in_order(): + dispatched = [] + + def spy(name): + class SpyRouter: + async def dispatch(self, event, gh, **kwargs): + dispatched.append((name, kwargs["features"])) + + return SpyRouter() + + spies = {name: spy(name) for name in main.FEATURE_ROUTERS} + with mock.patch.dict(main.FEATURE_ROUTERS, spies): + await main.dispatch(pr_opened_event("python/cpython"), FakeGH()) + + assert [name for name, _ in dispatched] == list(main.FEATURE_ROUTERS) + assert all(features == config.ALL_FEATURES for _, features in dispatched) + + +async def test_event_without_repository_runs_no_features(): + event_data = {"action": "created"} + event_data.update(app_installation_payload) + event = sansio.Event(event_data, event="installation", delivery_id="1") + with mock.patch.dict(main.FEATURE_ROUTERS, {}, clear=True): + main.FEATURE_ROUTERS["policy"] = mock.AsyncMock() + await main.dispatch(event, FakeGH()) + main.FEATURE_ROUTERS["policy"].dispatch.assert_not_called() diff --git a/tests/test_config.py b/tests/test_config.py new file mode 100644 index 00000000..3eac66a2 --- /dev/null +++ b/tests/test_config.py @@ -0,0 +1,39 @@ +import pytest + +from bedevere import config + + +@pytest.fixture(autouse=True) +def reset_logged_repos(monkeypatch): + monkeypatch.setattr(config, "_logged_unknown_repos", set()) + + +def test_cpython_gets_every_feature(): + assert config.features_for("python/cpython") == config.ALL_FEATURES + + +def test_unknown_python_repo_gets_policy_only(): + assert config.features_for("python/devguide") == {"policy"} + + +@pytest.mark.parametrize("repo", ["mariatta/devguide", "pythonista/cpython"]) +def test_repo_outside_python_org_gets_nothing(repo): + assert config.features_for(repo) == frozenset() + + +def test_lookup_ignores_case(): + assert config.features_for("Python/CPython") == config.ALL_FEATURES + assert config.features_for("Python/DevGuide") == {"policy"} + + +def test_unknown_repo_logged_once(capfd): + config.features_for("python/devguide") + config.features_for("python/devguide") + out, err = capfd.readouterr() + assert err.count("python/devguide has no feature config") == 1 + + +def test_repo_outside_python_org_logged_as_running_nothing(capfd): + config.features_for("mariatta/devguide") + out, err = capfd.readouterr() + assert "mariatta/devguide has no feature config, running only: nothing" in err diff --git a/tests/test_filepaths.py b/tests/test_filepaths.py index 761ce90b..52555839 100644 --- a/tests/test_filepaths.py +++ b/tests/test_filepaths.py @@ -261,3 +261,34 @@ async def test_synchronize(): assert len(gh.post_url) == 1 assert gh.post_url[0] == "https://api.github.com/some/status" assert gh.post_data[0]["state"] == "success" + + +@pytest.mark.parametrize("action", ["opened", "synchronize"]) +async def test_no_news_check_when_news_disabled(action): + filenames = [ + { + "filename": "test_docs2.py", + "patch": "@@ -31,3 +31,7 @@ # Licensed to PSF under a Contributor Agreement.", + }, + ] + issue = {"labels": [], "labels_url": "https://api.github.com/some/label"} + gh = FakeGH(getiter=filenames, getitem=issue) + event_data = { + "action": action, + "number": 1234, + "pull_request": { + "url": "https://api.github.com/repos/cpython/python/pulls/1234", + "statuses_url": "https://api.github.com/some/status", + "issue_url": "https://api.github.com/repos/cpython/python/issue/1234", + "issue_comment_url": "https://api.github.com/repos/cpython/python/issue/1234/comments", + "author_association": "NONE", + }, + } + event = sansio.Event(event_data, event="pull_request", delivery_id="1") + await filepaths.router.dispatch(event, gh, features=frozenset({"filepaths"})) + if action == "opened": + # The type label is still applied, but there's no NEWS status or help. + assert gh.post_url == ["https://api.github.com/some/label"] + assert gh.post_data == [[Labels.tests.value]] + else: + assert gh.post_url == [] diff --git a/tests/test_policy.py b/tests/test_policy.py index a789cef6..b47ee44c 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -92,11 +92,13 @@ def app_id(monkeypatch): ("pull_request", policy.CLOSE_LABEL, policy.PR_CLOSE_MESSAGE), ], ) -async def test_policy_action_through_main_router(kind, label, message): +@pytest.mark.parametrize("repo", ["python/cpython", "python/devguide"]) +async def test_policy_action_through_main_dispatch(repo, kind, label, message): event = make_event(kind, label) + event.data["repository"] = {"full_name": repo} gh = FakeGH(label) - await main.router.dispatch(event, gh) + await main.dispatch(event, gh) assert gh.calls[0] == ("get", ISSUE_URL) assert gh.calls[1] == ("getiter", COMMENTS_URL) @@ -113,6 +115,16 @@ async def test_policy_action_through_main_router(kind, label, message): assert gh.issue["state"] == "open" +async def test_no_policy_action_outside_python_org(): + event = make_event("issues", policy.CLOSE_LABEL) + event.data["repository"] = {"full_name": "mariatta/devguide"} + gh = FakeGH(policy.CLOSE_LABEL) + + await main.dispatch(event, gh) + + assert gh.calls == [] + + async def test_unrelated_label(): gh = FakeGH("type-bug") await policy.router.dispatch(make_event("issues", "type-bug"), gh)