diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 750306c..49d0dc6 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -14,12 +14,14 @@ on: paths: - 'scripts/**' - 'tests/**' + - 'cla/**' - '.github/workflows/tests.yml' push: branches: [main] paths: - 'scripts/**' - 'tests/**' + - 'cla/**' - '.github/workflows/tests.yml' permissions: diff --git a/cla/allowlist.yml b/cla/allowlist.yml index 49a0bc3..11a1b0e 100644 --- a/cla/allowlist.yml +++ b/cla/allowlist.yml @@ -1,48 +1,36 @@ # .github/cla/allowlist.yml -# Purpose: (A) workflow overrides for CLA/DCO, (B) license overrides for requires_cla.py +# +# This file does NOT contain a per-user or per-team bypass list. It has two +# effects, both on WHICH DOCUMENT a contributor must sign, never on whether +# someone is checked at all: +# * license_overrides.require_cla / allow_dco — read by requires_cla.py, +# override a licence's permissive/non-permissive classification. +# * license_overrides.repos..require_cla — read by +# policy_selector.fetch_shared_config(), and `false` downgrades that whole +# repo from CLA to DCO. This is live: vmware/.github is on DCO because of +# the entry below. +# +# Two further top-level keys are honoured but undocumented elsewhere, so they +# are recorded here rather than left to be rediscovered: a top-level `repos:` +# block (used only when license_overrides.repos is absent or empty), and a +# top-level `repositories:` list whose entries are appended straight to the +# DCO-only set. Prefer license_overrides.repos; the other two exist for +# backwards compatibility. +# +# Who skips the gate entirely is decided in policy_selector.process_single_pr(), +# not here. Three paths return early, in order: an existing successful +# "Check CLA/DCO" status on the head SHA, a bot (BOT_ALLOWLIST or any login +# ending "[bot]"), and an org member (live is_org_member() API call). +# +# Both production workflows fetch this file from the default branch with no +# pinned ref, so any edit is live across every gated repo on the next sweep. # ----------------------------- -# A) WORKFLOW OVERRIDES (used by reusable-cla-check.yml) -# ----------------------------- - -# If true, any org member is treated as allowed (no CLA/DCO gate). -org_members: true - -# Run DCO on permissive repos? -# off -> never run assistant on permissive repos -# external_only -> run assistant (DCO mode) for non-members only -# all -> run assistant (DCO mode) for everyone -dco_on_permissive: external_only - -# GitHub logins that always skip the gate (case-insensitive) -users: - - normansden -# - bob-consultant - -# Bot accounts that always skip the gate -bots: - - dependabot[bot] - - github-actions[bot] - -# Teams whose members always skip the gate. -# Accepts "org/team" or bare "team" (interpreted as /team). -# teams: -# - myorg/maintainers -# - release-engineering - -# Optional global exemption window (everyone is allowed during this period). -# ISO 8601 timestamps; end is inclusive to the minute. -#temporary_exemptions: -# start: 2025-12-30T00:00:00Z -# end: 2025-12-31T23:59:59Z -# reason: "Partner pilot" - -# ----------------------------- -# B) LICENSE POLICY OVERRIDES (used by requires_cla.py) +# LICENSE POLICY OVERRIDES # ----------------------------- # Base truth for permissive/non-permissive comes from: -# data/permissive.json + data/licenses_all.json +# data/permissive_names.json + data/licenses_all.json # The keys below *override* that base decision. license_overrides: @@ -54,18 +42,20 @@ license_overrides: - LicenseRef-Broadcom* # (add more as needed) - # (Optional) Treat specific IDs as permissive (force allow), even if base says otherwise. - # Only use this if you intend to *downgrade* certain licenses to permissive. -# permissive: + # (Optional) The inverse of require_cla: force these IDs to DCO even if the + # base tables call them non-permissive. The key is allow_dco — an earlier + # version of this file documented it as "permissive", which nothing reads. +# allow_dco: - # (Optional) Per-repo overrides. Keys can be "/" or just "". - # Useful when the detector result needs correction or you want to force policy. + # (Optional) Per-repo overrides. Keys MUST be "/" — process_single_pr + # compares against the full name, so a bare "" key is collected but never + # matches anything. repos: vmware/.github: - require_cla: false # force enforcement for this repo + require_cla: false # false = downgrade this repo to DCO-only # force_spdx: "MIT" # pretend this is the detected license (rarely needed) -# docs-site: -# require_cla: false # force permissive for this repo +# vmware/docs-site: +# require_cla: false # false = downgrade that repo to DCO-only # (Optional) Normalize odd/legacy license IDs to canonical SPDX before evaluation. # The left side is what the detector finds; the right side is the canonical ID. diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 1e695de..48ef37c 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -29,6 +29,8 @@ import json import os import sys +import contextlib +import io import tempfile import unittest @@ -55,6 +57,15 @@ import policy_selector # noqa: E402 +# policy_selector wraps this import in try/except and substitutes a stub, so a +# missing optional dep (aiohttp, rapidfuzz) degrades rather than breaking. Match +# that: an unguarded import here turns one absent dep into a collection error +# that erases the whole suite instead of skipping the tests that need it. +try: + import requires_cla # noqa: E402 +except Exception: # pragma: no cover - exercised only on a degraded runner + requires_cla = None + SIGNED_SUFFIX = "for this and all future contributions" @@ -686,5 +697,110 @@ def test_dedup_markers_are_present(self): self.assertIn("Sign via Comment", msg) +@unittest.skipIf(requires_cla is None, "requires_cla unavailable (optional dep missing)") +class TestAllowlistFile(unittest.TestCase): + """Parse the REAL cla/allowlist.yml. + + Every other test in this file monkeypatches `requires_cla.requires_CLA` + away in setUp, so none of them exercise the shipped file — the suite + passes whether it is valid, emptied, or absent. That gap let a change to + this file reach a PR with the full suite green and nothing reading it. + + (Note the seam is the monkeypatch, not `allowlist_data={}`: `_load_allowlist` + gates on a truthiness check, so an empty dict falls through to the disk + read and would pick up the real file anyway.) + + Both production workflows fetch this file from `ref: main` with no pinned + ref, so a malformed version is live org-wide the moment it merges. + """ + + # The only top-level keys any live code reads: `license_overrides` via + # requires_cla, and `repos`/`repositories` via + # policy_selector.fetch_shared_config. Asserting a subset rather than + # denying a list of known-dead names catches keys nobody has thought of — + # including `temp_exemptions`, the spelling the retired workflow actually + # read, which an earlier denylist here missed while blocking the inert + # `temporary_exemptions`. + READ_BY_LIVE_CODE = {"license_overrides", "repos", "repositories"} + + @classmethod + def setUpClass(cls): + import yaml + # Reuse production's own path constant so moving the file fails loudly + # here instead of leaving the test reading a stale location. + cls.path = requires_cla._ALLOWLIST_PATH + # encoding= matters: _load_allowlist opens utf-8 explicitly, and this + # file contains non-ASCII. Without it a non-UTF-8 locale raises in + # setUpClass and silently erases all of these tests from the report. + cls.data = yaml.safe_load(cls.path.read_text(encoding="utf-8")) + + def mapping(self): + """Fail fast with a message naming the file, rather than letting a + non-mapping surface as AttributeError/TypeError in four places.""" + self.assertIsInstance( + self.data, dict, + f"{self.path} must parse to a mapping; got {type(self.data).__name__}", + ) + return self.data + + def test_file_parses_to_a_mapping(self): + self.mapping() + + def test_only_keys_live_code_reads(self): + extra = set(self.mapping()) - self.READ_BY_LIVE_CODE + self.assertEqual( + extra, set(), + f"{sorted(extra)} is read by no live code. Enforcement bypasses live in " + "policy_selector.process_single_pr(); a key here that looks like one " + "but does nothing is how a departed employee stayed apparently " + "allowlisted after the workflow honouring it was retired.", + ) + + def test_license_overrides_shaped_as_requires_cla_expects(self): + overrides = self.mapping().get("license_overrides") + self.assertIsInstance(overrides, dict) + self.assertIsInstance(overrides.get("require_cla"), list) + + def test_repo_overrides_shaped_as_policy_selector_expects(self): + """`license_overrides.repos` decides CLA-vs-DCO for a whole repo, and + fetch_shared_config swallows a malformed shape into a debug log.""" + repos = (self.mapping().get("license_overrides") or {}).get("repos") + self.assertIsInstance(repos, dict, "repos must be a mapping, not a list or null") + for name, cfg in repos.items(): + self.assertIsInstance(cfg, dict, f"repos[{name}] must be a mapping, not null") + self.assertIsInstance(cfg.get("require_cla"), bool, + f"repos[{name}].require_cla must be a bool") + self.assertIn("/", name, + f"repos[{name}] must be '/' — process_single_pr " + "compares against the full name, so a bare repo never matches") + + def test_dotgithub_stays_on_dco(self): + """Pins a deliberate policy decision rather than the file's shape. + + Deleting the repos block, emptying it, dropping this entry, or flipping + it to true all leave the previous shape-only assertions green while + silently moving this repo from DCO back to CLA. Changing that is a + legitimate decision — it just has to be a deliberate one, so it fails + here first. + """ + repos = (self.mapping().get("license_overrides") or {}).get("repos") or {} + self.assertIs( + repos.get("vmware/.github", {}).get("require_cla"), False, + "vmware/.github is intentionally on DCO; if that changed on purpose, " + "update this test in the same commit", + ) + + def test_broadcom_source_available_still_forces_cla(self): + """Broadcom Source Available is listed as permissive in the base + tables, so without this override it would fall through to DCO. Feed + the raw ID through production's own normaliser rather than a + hand-normalised literal, so a change to that normaliser fails here.""" + norm = requires_cla._norm_license_name("LicenseRef-Broadcom_Source_Available") + buf = io.StringIO() + with contextlib.redirect_stdout(buf): # keeps ::warning:: out of CI annotations + decision = requires_cla._override_requires_cla(norm, self.mapping()) + self.assertIs(decision, True) + + if __name__ == "__main__": unittest.main()