From ac09448592162f1101f08b869bef4ca236f1c8c6 Mon Sep 17 00:00:00 2001 From: Baggio Wong Date: Wed, 7 Oct 2026 18:19:46 +0000 Subject: [PATCH 1/2] feat: Add a template propagation check for pull requests to main A new minor's template is a one-time copy of the previous minor's. Nothing carries later changes forward, so a change merged to an older minor's template after the copy skips the newer minor unless someone makes it there too. 4.6.0 missed #1343 this way. CONTRIBUTING.md already describes a check for this, but it was never implemented. The "Check template propagation" workflow runs on every pull request to main and in the merge queue. It fails when: - a file in an older minor's template changes without the same file changing in the newest minor's template, unless the description contains `template-propagation: scoped`; - a newly added minor template differs from the previous minor's, apart from paths declared as `template-divergence: `; - a change edits the old per-major paths (template/v4/dirs/), which the build no longer reads. It uses only the standard library, so it runs without the conda environment. It is meant to be required through a ruleset with no bypass list, after a few days running advisory. --- .../workflows/check-template-propagation.yml | 46 +++++ CONTRIBUTING.md | 6 +- src/template_tools.py | 186 ++++++++++++++++++ test/test_template_tools.py | 137 +++++++++++++ 4 files changed, 374 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/check-template-propagation.yml create mode 100644 src/template_tools.py create mode 100644 test/test_template_tools.py diff --git a/.github/workflows/check-template-propagation.yml b/.github/workflows/check-template-propagation.yml new file mode 100644 index 000000000..d6badadf4 --- /dev/null +++ b/.github/workflows/check-template-propagation.yml @@ -0,0 +1,46 @@ +name: Check template propagation +# Meant to be a required status check on main, set by a ruleset with no bypass list. See the +# "Modifying the Dockerfile or dirs/" section of CONTRIBUTING.md and src/template_tools.py. +on: + # No paths filter: a required check that never reports leaves the pull request stuck as pending. + # The check exits straight away when a pull request changes no template files. + pull_request: + branches: [main] + # `edited` reruns the check when someone adds an opt-out line to the description. + types: [opened, synchronize, reopened, edited] + merge_group: +jobs: + check: + name: Check template propagation + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + filter: blob:none + - name: Save the pull request description + env: + GH_TOKEN: ${{ github.token }} + EVENT_NAME: ${{ github.event_name }} + # Passed through the environment, never interpolated into the script. + PR_BODY: ${{ github.event.pull_request.body }} + QUEUE_REF: ${{ github.event.merge_group.head_ref }} + run: | + set -euo pipefail + if [ "$EVENT_NAME" = "merge_group" ]; then + # Queue refs look like refs/heads/gh-readonly-queue/main/pr-1363-. + pr="$(sed -nE 's#.*/pr-([0-9]+)-[0-9a-f]+$#\1#p' <<< "$QUEUE_REF")" + gh pr view "$pr" --repo "$GITHUB_REPOSITORY" --json body --jq .body > "$RUNNER_TEMP/pr-body.txt" + else + printf '%s' "$PR_BODY" > "$RUNNER_TEMP/pr-body.txt" + fi + - name: Check + env: + # For pull_request, github.sha is the pull request merged into main, so the check sees the latest + # main as of this run. Merge queue (or "require branches to be up to date") keeps it current. + BASE: ${{ github.event.merge_group.base_sha || github.event.pull_request.base.sha }} + HEAD: ${{ github.event.merge_group.head_sha || github.sha }} + run: python3 src/template_tools.py check --base "$BASE" --head "$HEAD" --pr-body-file "$RUNNER_TEMP/pr-body.txt" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f5c756375..4536588da 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -172,7 +172,11 @@ Include every template directory you created or edited in your PR, and open the > **Why this rule exists:** templates are copied forward only **once** — at the moment a new minor version is first created (see "How new minor version templates are auto-created at build time" below). After that, each minor's template is an independent, frozen snapshot. So editing an *already-released* minor's template does not reach any newer minor. A common past mistake was editing v4.5 while v4.6 was already in flight: the change landed only in the v4.5 line and never reached v4.6+. Following the rule above avoids this — you always land the change on the newest minor template, creating it first if the current newest is already released. -A CI check (`check_template_propagation`) backs this up: if your PR edits an older minor's template file while a newer minor template exists, it fails unless the same file is also changed in the newest minor template. For a change that is intentionally scoped to older minor lines only (e.g. a targeted backport), add the line `template-propagation: scoped` to your PR description to opt out. +The "Check template propagation" check (`src/template_tools.py check`) backs this up on every pull request to `main`: + +- Changing a file in an older minor's template fails unless the same file also changes in the newest minor's template. For a change that is intentionally scoped to older minor lines only (e.g. a targeted backport), add the line `template-propagation: scoped` to your PR description to opt out. +- Adding a new minor template fails unless it is an exact copy of the previous minor's template, apart from paths listed in the description as `template-divergence: ` lines. +- Editing the old per-major paths, such as `template/v4/dirs/`, fails, because the build only reads `template/v4/v4./`. #### Applying a security fix or infrastructure change (applies to all supported minor versions) diff --git a/src/template_tools.py b/src/template_tools.py new file mode 100644 index 000000000..e9a9e3944 --- /dev/null +++ b/src/template_tools.py @@ -0,0 +1,186 @@ +"""Helpers for the per-minor image templates under template/v{major}/v{major}.{minor}/. + +This module only uses the standard library, so workflows can run it with a plain Python +interpreter instead of the sagemaker-distribution conda environment. + +Commands: + + python src/template_tools.py check --base --head [--pr-body-file ] + Check a pull request's template changes. See find_problems() for the rules. + +Why the check exists: a new minor's template is a one-time copy of the previous minor's template. +Nothing carries later template changes forward, so a change merged to an older minor's template +after the copy skips the newer minor unless it is made there too (4.6.0 missed #1343 this way). +""" + +import argparse +import os +import re +import subprocess +import sys + +_MINOR_TEMPLATE_PATH = re.compile(r"^template/v(\d+)/v(\d+)\.(\d+)/(.+)$") +_LEGACY_TEMPLATE_PATH = re.compile(r"^template/v(\d+)/(.+)$") +_SCOPED_OPT_OUT = re.compile(r"^\s*template-propagation:\s*scoped\s*$", re.MULTILINE | re.IGNORECASE) +_DECLARED_DIVERGENCE = re.compile(r"^\s*template-divergence:\s*(\S+)\s*$", re.MULTILINE) + + +def minor_template_dir(major, minor): + return f"template/v{major}/v{major}.{minor}" + + +def parse_minor_template_path(path): + """Return (major, minor, path inside the template) for a per-minor template file, or None.""" + match = _MINOR_TEMPLATE_PATH.match(path) + if not match or match.group(1) != match.group(2): + return None + return int(match.group(2)), int(match.group(3)), match.group(4) + + +def minors_by_major(paths): + """Map each major version to the set of minors that have a template directory in `paths`.""" + minors = {} + for path in paths: + parsed = parse_minor_template_path(path) + if parsed: + minors.setdefault(parsed[0], set()).add(parsed[1]) + return minors + + +def find_propagation_problems(changed_paths, head_paths, scoped): + """Rule 1: changing an older minor's template file must change the same file in the newest minor too. + + The newest minor template is a copy taken once, so nothing else carries the change forward. + `scoped` (from `template-propagation: scoped` in the PR description) waives this for deliberate + backports. + """ + if scoped: + return [] + newest = {major: max(minors) for major, minors in minors_by_major(head_paths).items()} + changed = set(changed_paths) + problems = [] + for path in sorted(changed): + parsed = parse_minor_template_path(path) + if not parsed: + continue + major, minor, inner = parsed + if minor >= newest.get(major, minor): + continue + counterpart = f"{minor_template_dir(major, newest[major])}/{inner}" + if counterpart not in changed: + problems.append(f"{path} changed, but {counterpart} did not.") + return problems + + +def find_copy_problems(base_paths, head_tree, declared): + """Rule 2: a newly added minor template must match the previous minor's template file for file. + + `head_tree` maps path -> (mode, blob id). Files that are meant to differ in the new minor must be + listed in the PR description as `template-divergence: `. Run against the PR merged with the + latest main, this also catches a change to the previous minor that landed after the copy was made. + """ + added = minors_by_major(head_tree) + existing = minors_by_major(base_paths) + problems = [] + for major in sorted(added): + for minor in sorted(added[major] - existing.get(major, set())): + new_dir, previous_dir = minor_template_dir(major, minor), minor_template_dir(major, minor - 1) + if minor == 0: + continue + if minor - 1 not in added[major]: + problems.append(f"{new_dir}/ was added, but there is no {previous_dir}/ to copy it from.") + continue + new_files = {p[len(new_dir) + 1 :]: v for p, v in head_tree.items() if p.startswith(new_dir + "/")} + old_files = { + p[len(previous_dir) + 1 :]: v for p, v in head_tree.items() if p.startswith(previous_dir + "/") + } + for inner in sorted(set(new_files) | set(old_files)): + if f"{new_dir}/{inner}" in declared: + continue + if inner not in new_files: + problems.append(f"{previous_dir}/{inner} is missing from {new_dir}/.") + elif inner not in old_files: + problems.append(f"{new_dir}/{inner} is not in {previous_dir}/ and is not declared.") + elif new_files[inner] != old_files[inner]: + problems.append(f"{new_dir}/{inner} differs from {previous_dir}/{inner} and is not declared.") + return problems + + +def find_legacy_path_problems(changed_paths, head_paths): + """Rule 3: for majors that have per-minor templates, the build no longer reads template/vX/.""" + majors = set(minors_by_major(head_paths)) + problems = [] + for path in sorted(set(changed_paths)): + match = _LEGACY_TEMPLATE_PATH.match(path) + if not match or parse_minor_template_path(path) or int(match.group(1)) not in majors: + continue + major = match.group(1) + problems.append( + f"{path} is not in a per-minor template; the build only reads template/v{major}/v{major}./." + ) + return problems + + +def find_problems(changed_paths, base_paths, head_tree, pr_body): + """Apply all three rules. `head_tree` maps path -> (mode, blob id) for the tree being merged.""" + pr_body = pr_body or "" + scoped = bool(_SCOPED_OPT_OUT.search(pr_body)) + declared = set(_DECLARED_DIVERGENCE.findall(pr_body)) + return ( + find_propagation_problems(changed_paths, head_tree, scoped) + + find_copy_problems(base_paths, head_tree, declared) + + find_legacy_path_problems(changed_paths, head_tree) + ) + + +def _git(*args): + return subprocess.run(["git", *args], check=True, capture_output=True, text=True).stdout + + +def _template_tree(ref): + """Map path -> (mode, blob id) for every file under template/ at `ref`.""" + tree = {} + for line in _git("ls-tree", "-r", ref, "--", "template").splitlines(): + meta, path = line.split("\t", 1) + mode, _, blob = meta.split() + tree[path] = (mode, blob) + return tree + + +def run_check(base, head, pr_body): + changed = [p for p in _git("diff", "--name-only", f"{base}...{head}").splitlines() if p.startswith("template/")] + if not changed: + print("No template changes.") + return 0 + problems = find_problems(changed, set(_template_tree(base)), _template_tree(head), pr_body) + if not problems: + print(f"Template changes look right ({len(changed)} files).") + return 0 + for problem in problems: + print(f"::error::{problem}") + print( + "\nSee CONTRIBUTING.md, 'Modifying the Dockerfile or dirs/'. A change meant only for older minors can add " + "`template-propagation: scoped` to the PR description. A new minor's template can list intended " + "differences as `template-divergence: ` lines." + ) + return 1 + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + commands = parser.add_subparsers(dest="command", required=True) + check = commands.add_parser("check", help="Check a pull request's template changes.") + check.add_argument("--base", required=True, help="Commit the pull request merges into.") + check.add_argument("--head", required=True, help="The pull request merged with --base.") + check.add_argument("--pr-body-file", help="File containing the pull request description.") + args = parser.parse_args(argv) + + pr_body = "" + if args.pr_body_file: + with open(args.pr_body_file) as f: + pr_body = f.read() + return run_check(args.base, args.head, pr_body) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/test/test_template_tools.py b/test/test_template_tools.py new file mode 100644 index 000000000..8e0e3e529 --- /dev/null +++ b/test/test_template_tools.py @@ -0,0 +1,137 @@ +import subprocess + +import pytest + +pytestmark = pytest.mark.unit + +from template_tools import ( + find_copy_problems, + find_legacy_path_problems, + find_problems, + find_propagation_problems, + parse_minor_template_path, + run_check, +) + +HEAD_PATHS = { + "template/v3/v3.9/Dockerfile", + "template/v4/v4.5/Dockerfile", + "template/v4/v4.5/dirs/a.sh", + "template/v4/v4.6/Dockerfile", + "template/v4/v4.6/dirs/a.sh", +} + + +def _tree(files): + """path -> blob id => path -> (mode, blob id), as git ls-tree reports it.""" + return {path: ("100644", blob) for path, blob in files.items()} + + +def test_parse_minor_template_path(): + assert parse_minor_template_path("template/v4/v4.6/dirs/etc/x.sh") == (4, 6, "dirs/etc/x.sh") + assert parse_minor_template_path("template/v4/v3.9/Dockerfile") is None + assert parse_minor_template_path("template/v4/dirs/etc/x.sh") is None + assert parse_minor_template_path("build_artifacts/v4/v4.6/v4.6.0/Dockerfile") is None + + +def test_older_minor_change_must_reach_newest_minor(): + assert find_propagation_problems(["template/v4/v4.5/dirs/a.sh"], HEAD_PATHS, scoped=False) == [ + "template/v4/v4.5/dirs/a.sh changed, but template/v4/v4.6/dirs/a.sh did not." + ] + both = ["template/v4/v4.5/dirs/a.sh", "template/v4/v4.6/dirs/a.sh"] + assert find_propagation_problems(both, HEAD_PATHS, scoped=False) == [] + + +def test_newest_minor_and_other_majors_need_nothing_else(): + assert find_propagation_problems(["template/v4/v4.6/dirs/a.sh"], HEAD_PATHS, scoped=False) == [] + assert find_propagation_problems(["template/v3/v3.9/Dockerfile"], HEAD_PATHS, scoped=False) == [] + + +def test_scoped_backport_is_allowed(): + assert find_propagation_problems(["template/v4/v4.5/dirs/a.sh"], HEAD_PATHS, scoped=True) == [] + + +def test_new_minor_must_match_previous_minor(): + base = {"template/v4/v4.6/Dockerfile", "template/v4/v4.6/dirs/a.sh"} + same = _tree( + { + "template/v4/v4.6/Dockerfile": "d1", + "template/v4/v4.6/dirs/a.sh": "a1", + "template/v4/v4.7/Dockerfile": "d1", + "template/v4/v4.7/dirs/a.sh": "a1", + } + ) + assert find_copy_problems(base, same, declared=set()) == [] + + stale = dict(same, **_tree({"template/v4/v4.6/dirs/a.sh": "a2"})) + assert find_copy_problems(base, stale, declared=set()) == [ + "template/v4/v4.7/dirs/a.sh differs from template/v4/v4.6/dirs/a.sh and is not declared." + ] + assert find_copy_problems(base, stale, declared={"template/v4/v4.7/dirs/a.sh"}) == [] + + missing = {p: v for p, v in same.items() if p != "template/v4/v4.7/dirs/a.sh"} + assert find_copy_problems(base, missing, declared=set()) == [ + "template/v4/v4.6/dirs/a.sh is missing from template/v4/v4.7/." + ] + + lost_exec_bit = dict(same, **{"template/v4/v4.6/dirs/a.sh": ("100755", "a1")}) + assert len(find_copy_problems(base, lost_exec_bit, declared=set())) == 1 + + +def test_new_minor_needs_the_minor_before_it(): + head = _tree({"template/v4/v4.6/Dockerfile": "d1", "template/v4/v4.8/Dockerfile": "d1"}) + assert find_copy_problems({"template/v4/v4.6/Dockerfile"}, head, declared=set()) == [ + "template/v4/v4.8/ was added, but there is no template/v4/v4.7/ to copy it from." + ] + + +def test_minor_already_on_main_is_not_compared_again(): + head = _tree({"template/v4/v4.6/Dockerfile": "d1", "template/v4/v4.7/Dockerfile": "d2"}) + assert find_copy_problems(set(head), head, declared=set()) == [] + + +def test_legacy_per_major_paths_are_rejected_once_a_major_has_minor_templates(): + assert find_legacy_path_problems(["template/v4/dirs/etc/x"], HEAD_PATHS) == [ + "template/v4/dirs/etc/x is not in a per-minor template; the build only reads template/v4/v4./." + ] + assert find_legacy_path_problems(["template/v1/dirs/etc/x"], HEAD_PATHS) == [] + + +def test_pr_description_opt_out(): + head = _tree({p: "x" for p in HEAD_PATHS}) + changed = ["template/v4/v4.5/dirs/a.sh"] + assert find_problems(changed, set(head), head, "") != [] + assert find_problems(changed, set(head), head, "Backport only.\n\ntemplate-propagation: scoped\n") == [] + + +def test_run_check_reads_the_git_trees(tmp_path, monkeypatch, capsys): + def git(*args): + result = subprocess.run( + ["git", "-c", "user.name=test", "-c", "user.email=test@example.com", *args], + cwd=tmp_path, + check=True, + capture_output=True, + text=True, + ) + return result.stdout.strip() + + git("init", "-q") + for minor in (5, 6): + (tmp_path / f"template/v4/v4.{minor}").mkdir(parents=True) + (tmp_path / f"template/v4/v4.{minor}/a.sh").write_text("old\n") + (tmp_path / "README.md").write_text("readme\n") + git("add", ".") + git("commit", "-q", "-m", "base") + base = git("rev-parse", "HEAD") + monkeypatch.chdir(tmp_path) + + (tmp_path / "README.md").write_text("readme v2\n") + git("commit", "-q", "-a", "-m", "not a template change") + assert run_check(base, git("rev-parse", "HEAD"), "") == 0 + + (tmp_path / "template/v4/v4.5/a.sh").write_text("new\n") + git("commit", "-q", "-a", "-m", "4.5 only") + head = git("rev-parse", "HEAD") + assert run_check(base, head, "") == 1 + assert "template/v4/v4.6/a.sh did not" in capsys.readouterr().out + assert run_check(base, head, "template-propagation: scoped") == 0 From 3cdf73e28fdeeb1b4ae5160823756f6c32746f99 Mon Sep 17 00:00:00 2001 From: Baggio Wong Date: Wed, 7 Oct 2026 21:09:07 +0000 Subject: [PATCH 2/2] refactor: Simplify the template propagation check Keep only the rule that addresses the root cause: changing an older minor's template requires changing the same file in the newest minor's template. - Replace the `template-propagation: scoped` description string with the `template-propagation-scoped` label. Only maintainers can apply labels, so scoping a change is their decision, and the workflow no longer reads the PR description. - Drop the new-template copy rule and its `template-divergence:` string, and the old per-major path rule. - Move the script next to the other CI helpers in .github/workflows/utils/, and pass inputs as environment variables so the workflow runs one command with no shell logic. - Mention the rule in the pull request template. --- .github/pull_request_template.md | 6 + .../workflows/check-template-propagation.yml | 41 ++-- .../utils/check_template_propagation.py | 92 +++++++++ CONTRIBUTING.md | 6 +- src/template_tools.py | 186 ------------------ test/test_check_template_propagation.py | 67 +++++++ test/test_template_tools.py | 137 ------------- 7 files changed, 178 insertions(+), 357 deletions(-) create mode 100644 .github/workflows/utils/check_template_propagation.py delete mode 100644 src/template_tools.py create mode 100644 test/test_check_template_propagation.py delete mode 100644 test/test_template_tools.py diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 8d26c5bc1..0bf5b7e50 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -17,6 +17,12 @@ Does this change need to be included in patch version releases? By default, any If yes, please explain why: [Explain the criticality of this change and why it should be included in patch releases] +## Template Changes +A change to `template/` reaches future minor versions only if it is also made in the newest minor's template (the highest `template/vX/vX.Y/`). See "Deciding where to make your change" in CONTRIBUTING.md. +- [ ] N/A (no template changes) +- [ ] The change is also made in the newest minor's template +- [ ] The change is deliberately limited to older minors (a maintainer adds the `template-propagation-scoped` label) + ## How Has This Been Tested? [Describe the tests you ran] diff --git a/.github/workflows/check-template-propagation.yml b/.github/workflows/check-template-propagation.yml index d6badadf4..3216a071e 100644 --- a/.github/workflows/check-template-propagation.yml +++ b/.github/workflows/check-template-propagation.yml @@ -1,46 +1,29 @@ name: Check template propagation -# Meant to be a required status check on main, set by a ruleset with no bypass list. See the -# "Modifying the Dockerfile or dirs/" section of CONTRIBUTING.md and src/template_tools.py. +# Fails when a pull request changes an older minor's template without changing the newest minor's. +# Meant to be required on main through a ruleset. See "Deciding where to make your change" in CONTRIBUTING.md. on: - # No paths filter: a required check that never reports leaves the pull request stuck as pending. - # The check exits straight away when a pull request changes no template files. pull_request: branches: [main] - # `edited` reruns the check when someone adds an opt-out line to the description. - types: [opened, synchronize, reopened, edited] - merge_group: + # labeled/unlabeled rerun the check when a maintainer adds or removes the template-propagation-scoped label. + # No paths filter: a required check that never reports leaves the pull request pending forever. The check + # passes straight away when no template files change. + types: [opened, synchronize, reopened, labeled, unlabeled] jobs: check: name: Check template propagation runs-on: ubuntu-latest permissions: contents: read - pull-requests: read steps: - uses: actions/checkout@v6 with: + # The check needs the base commit's history to diff against, but only file names, not contents. fetch-depth: 0 filter: blob:none - - name: Save the pull request description - env: - GH_TOKEN: ${{ github.token }} - EVENT_NAME: ${{ github.event_name }} - # Passed through the environment, never interpolated into the script. - PR_BODY: ${{ github.event.pull_request.body }} - QUEUE_REF: ${{ github.event.merge_group.head_ref }} - run: | - set -euo pipefail - if [ "$EVENT_NAME" = "merge_group" ]; then - # Queue refs look like refs/heads/gh-readonly-queue/main/pr-1363-. - pr="$(sed -nE 's#.*/pr-([0-9]+)-[0-9a-f]+$#\1#p' <<< "$QUEUE_REF")" - gh pr view "$pr" --repo "$GITHUB_REPOSITORY" --json body --jq .body > "$RUNNER_TEMP/pr-body.txt" - else - printf '%s' "$PR_BODY" > "$RUNNER_TEMP/pr-body.txt" - fi - name: Check env: - # For pull_request, github.sha is the pull request merged into main, so the check sees the latest - # main as of this run. Merge queue (or "require branches to be up to date") keeps it current. - BASE: ${{ github.event.merge_group.base_sha || github.event.pull_request.base.sha }} - HEAD: ${{ github.event.merge_group.head_sha || github.sha }} - run: python3 src/template_tools.py check --base "$BASE" --head "$HEAD" --pr-body-file "$RUNNER_TEMP/pr-body.txt" + BASE_SHA: ${{ github.event.pull_request.base.sha }} + # For pull_request events, github.sha is the pull request merged into main. + HEAD_SHA: ${{ github.sha }} + SCOPED: ${{ contains(github.event.pull_request.labels.*.name, 'template-propagation-scoped') }} + run: python3 .github/workflows/utils/check_template_propagation.py diff --git a/.github/workflows/utils/check_template_propagation.py b/.github/workflows/utils/check_template_propagation.py new file mode 100644 index 000000000..dea9fc2cc --- /dev/null +++ b/.github/workflows/utils/check_template_propagation.py @@ -0,0 +1,92 @@ +"""Fail a pull request that changes an older minor's template without changing the newest minor's. + +Each minor version has its own template, template/vX/vX.Y/. A new minor's template is a one-time copy of +the previous minor's, and nothing carries later changes forward. So a change made only to an older minor's +template never reaches future minor versions. 4.6.0 missed #1343 this way. + +Run by .github/workflows/check-template-propagation.yml, which passes its inputs as environment variables: + BASE_SHA the commit the pull request merges into + HEAD_SHA the pull request merged with BASE_SHA + SCOPED "true" when the pull request has the template-propagation-scoped label + +See "Deciding where to make your change" in CONTRIBUTING.md. +""" + +import os +import re +import subprocess +import sys + +# template/v4/v4.6/dirs/etc/x.sh -> major 4, minor 6, path inside the template "dirs/etc/x.sh" +TEMPLATE_FILE = re.compile(r"^template/v(\d+)/v\1\.(\d+)/(.+)$") +SCOPED_LABEL = "template-propagation-scoped" + + +def parse_template_path(path): + """Return (major, minor, path inside the template) for a per-minor template file, or None.""" + match = TEMPLATE_FILE.match(path) + if not match: + return None + return int(match.group(1)), int(match.group(2)), match.group(3) + + +def find_missing_changes(changed_files, all_files): + """Return (changed file, newest-minor counterpart) for each older-minor change the newest minor lacks. + + changed_files: template files the pull request changes. + all_files: every template file after the pull request merges; used to find each major's newest minor. + """ + newest = {} + for path in all_files: + parsed = parse_template_path(path) + if parsed: + major, minor, _ = parsed + newest[major] = max(newest.get(major, minor), minor) + + changed = set(changed_files) + missing = [] + for path in sorted(changed): + parsed = parse_template_path(path) + if not parsed: + continue + major, minor, inner_path = parsed + newest_minor = newest.get(major, minor) + if minor == newest_minor: + continue + counterpart = f"template/v{major}/v{major}.{newest_minor}/{inner_path}" + if counterpart not in changed: + missing.append((path, counterpart)) + return missing + + +def git_lines(*args): + return subprocess.run(["git", *args], check=True, capture_output=True, text=True).stdout.splitlines() + + +def main(): + if os.environ.get("SCOPED") == "true": + print(f"Skipped: the pull request has the {SCOPED_LABEL} label.") + return 0 + + base, head = os.environ["BASE_SHA"], os.environ["HEAD_SHA"] + changed = git_lines("diff", "--name-only", f"{base}...{head}", "--", "template") + all_files = git_lines("ls-tree", "-r", "--name-only", head, "--", "template") + missing = find_missing_changes(changed, all_files) + + if not missing: + print(f"OK: {len(changed)} template file(s) changed, none missing from the newest minor's template.") + return 0 + + for path, counterpart in missing: + print(f"::error file={path}::{path} changed, but {counterpart} did not.") + print( + "\nA change to an older minor's template only reaches future minor versions if it is also made in " + "the newest minor's template. Make the same change in the files listed above. If this change is " + f"deliberately only for older minors, ask a maintainer to add the {SCOPED_LABEL} label. " + "See 'Deciding where to make your change' in CONTRIBUTING.md." + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4536588da..9ba7e27cb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -172,11 +172,7 @@ Include every template directory you created or edited in your PR, and open the > **Why this rule exists:** templates are copied forward only **once** — at the moment a new minor version is first created (see "How new minor version templates are auto-created at build time" below). After that, each minor's template is an independent, frozen snapshot. So editing an *already-released* minor's template does not reach any newer minor. A common past mistake was editing v4.5 while v4.6 was already in flight: the change landed only in the v4.5 line and never reached v4.6+. Following the rule above avoids this — you always land the change on the newest minor template, creating it first if the current newest is already released. -The "Check template propagation" check (`src/template_tools.py check`) backs this up on every pull request to `main`: - -- Changing a file in an older minor's template fails unless the same file also changes in the newest minor's template. For a change that is intentionally scoped to older minor lines only (e.g. a targeted backport), add the line `template-propagation: scoped` to your PR description to opt out. -- Adding a new minor template fails unless it is an exact copy of the previous minor's template, apart from paths listed in the description as `template-divergence: ` lines. -- Editing the old per-major paths, such as `template/v4/dirs/`, fails, because the build only reads `template/v4/v4./`. +The "Check template propagation" check backs this up on every pull request to `main`: if your PR changes a file in an older minor's template, it fails unless the same file also changes in the newest minor's template. The failure lists each file that is missing the change. For a change that is intentionally scoped to older minor lines only (e.g. a targeted backport), ask a maintainer to add the `template-propagation-scoped` label to your PR; the check reruns and passes. The check only confirms that the newest minor's file changed too, so reviewers still need to confirm it changed the same way. #### Applying a security fix or infrastructure change (applies to all supported minor versions) diff --git a/src/template_tools.py b/src/template_tools.py deleted file mode 100644 index e9a9e3944..000000000 --- a/src/template_tools.py +++ /dev/null @@ -1,186 +0,0 @@ -"""Helpers for the per-minor image templates under template/v{major}/v{major}.{minor}/. - -This module only uses the standard library, so workflows can run it with a plain Python -interpreter instead of the sagemaker-distribution conda environment. - -Commands: - - python src/template_tools.py check --base --head [--pr-body-file ] - Check a pull request's template changes. See find_problems() for the rules. - -Why the check exists: a new minor's template is a one-time copy of the previous minor's template. -Nothing carries later template changes forward, so a change merged to an older minor's template -after the copy skips the newer minor unless it is made there too (4.6.0 missed #1343 this way). -""" - -import argparse -import os -import re -import subprocess -import sys - -_MINOR_TEMPLATE_PATH = re.compile(r"^template/v(\d+)/v(\d+)\.(\d+)/(.+)$") -_LEGACY_TEMPLATE_PATH = re.compile(r"^template/v(\d+)/(.+)$") -_SCOPED_OPT_OUT = re.compile(r"^\s*template-propagation:\s*scoped\s*$", re.MULTILINE | re.IGNORECASE) -_DECLARED_DIVERGENCE = re.compile(r"^\s*template-divergence:\s*(\S+)\s*$", re.MULTILINE) - - -def minor_template_dir(major, minor): - return f"template/v{major}/v{major}.{minor}" - - -def parse_minor_template_path(path): - """Return (major, minor, path inside the template) for a per-minor template file, or None.""" - match = _MINOR_TEMPLATE_PATH.match(path) - if not match or match.group(1) != match.group(2): - return None - return int(match.group(2)), int(match.group(3)), match.group(4) - - -def minors_by_major(paths): - """Map each major version to the set of minors that have a template directory in `paths`.""" - minors = {} - for path in paths: - parsed = parse_minor_template_path(path) - if parsed: - minors.setdefault(parsed[0], set()).add(parsed[1]) - return minors - - -def find_propagation_problems(changed_paths, head_paths, scoped): - """Rule 1: changing an older minor's template file must change the same file in the newest minor too. - - The newest minor template is a copy taken once, so nothing else carries the change forward. - `scoped` (from `template-propagation: scoped` in the PR description) waives this for deliberate - backports. - """ - if scoped: - return [] - newest = {major: max(minors) for major, minors in minors_by_major(head_paths).items()} - changed = set(changed_paths) - problems = [] - for path in sorted(changed): - parsed = parse_minor_template_path(path) - if not parsed: - continue - major, minor, inner = parsed - if minor >= newest.get(major, minor): - continue - counterpart = f"{minor_template_dir(major, newest[major])}/{inner}" - if counterpart not in changed: - problems.append(f"{path} changed, but {counterpart} did not.") - return problems - - -def find_copy_problems(base_paths, head_tree, declared): - """Rule 2: a newly added minor template must match the previous minor's template file for file. - - `head_tree` maps path -> (mode, blob id). Files that are meant to differ in the new minor must be - listed in the PR description as `template-divergence: `. Run against the PR merged with the - latest main, this also catches a change to the previous minor that landed after the copy was made. - """ - added = minors_by_major(head_tree) - existing = minors_by_major(base_paths) - problems = [] - for major in sorted(added): - for minor in sorted(added[major] - existing.get(major, set())): - new_dir, previous_dir = minor_template_dir(major, minor), minor_template_dir(major, minor - 1) - if minor == 0: - continue - if minor - 1 not in added[major]: - problems.append(f"{new_dir}/ was added, but there is no {previous_dir}/ to copy it from.") - continue - new_files = {p[len(new_dir) + 1 :]: v for p, v in head_tree.items() if p.startswith(new_dir + "/")} - old_files = { - p[len(previous_dir) + 1 :]: v for p, v in head_tree.items() if p.startswith(previous_dir + "/") - } - for inner in sorted(set(new_files) | set(old_files)): - if f"{new_dir}/{inner}" in declared: - continue - if inner not in new_files: - problems.append(f"{previous_dir}/{inner} is missing from {new_dir}/.") - elif inner not in old_files: - problems.append(f"{new_dir}/{inner} is not in {previous_dir}/ and is not declared.") - elif new_files[inner] != old_files[inner]: - problems.append(f"{new_dir}/{inner} differs from {previous_dir}/{inner} and is not declared.") - return problems - - -def find_legacy_path_problems(changed_paths, head_paths): - """Rule 3: for majors that have per-minor templates, the build no longer reads template/vX/.""" - majors = set(minors_by_major(head_paths)) - problems = [] - for path in sorted(set(changed_paths)): - match = _LEGACY_TEMPLATE_PATH.match(path) - if not match or parse_minor_template_path(path) or int(match.group(1)) not in majors: - continue - major = match.group(1) - problems.append( - f"{path} is not in a per-minor template; the build only reads template/v{major}/v{major}./." - ) - return problems - - -def find_problems(changed_paths, base_paths, head_tree, pr_body): - """Apply all three rules. `head_tree` maps path -> (mode, blob id) for the tree being merged.""" - pr_body = pr_body or "" - scoped = bool(_SCOPED_OPT_OUT.search(pr_body)) - declared = set(_DECLARED_DIVERGENCE.findall(pr_body)) - return ( - find_propagation_problems(changed_paths, head_tree, scoped) - + find_copy_problems(base_paths, head_tree, declared) - + find_legacy_path_problems(changed_paths, head_tree) - ) - - -def _git(*args): - return subprocess.run(["git", *args], check=True, capture_output=True, text=True).stdout - - -def _template_tree(ref): - """Map path -> (mode, blob id) for every file under template/ at `ref`.""" - tree = {} - for line in _git("ls-tree", "-r", ref, "--", "template").splitlines(): - meta, path = line.split("\t", 1) - mode, _, blob = meta.split() - tree[path] = (mode, blob) - return tree - - -def run_check(base, head, pr_body): - changed = [p for p in _git("diff", "--name-only", f"{base}...{head}").splitlines() if p.startswith("template/")] - if not changed: - print("No template changes.") - return 0 - problems = find_problems(changed, set(_template_tree(base)), _template_tree(head), pr_body) - if not problems: - print(f"Template changes look right ({len(changed)} files).") - return 0 - for problem in problems: - print(f"::error::{problem}") - print( - "\nSee CONTRIBUTING.md, 'Modifying the Dockerfile or dirs/'. A change meant only for older minors can add " - "`template-propagation: scoped` to the PR description. A new minor's template can list intended " - "differences as `template-divergence: ` lines." - ) - return 1 - - -def main(argv=None): - parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) - commands = parser.add_subparsers(dest="command", required=True) - check = commands.add_parser("check", help="Check a pull request's template changes.") - check.add_argument("--base", required=True, help="Commit the pull request merges into.") - check.add_argument("--head", required=True, help="The pull request merged with --base.") - check.add_argument("--pr-body-file", help="File containing the pull request description.") - args = parser.parse_args(argv) - - pr_body = "" - if args.pr_body_file: - with open(args.pr_body_file) as f: - pr_body = f.read() - return run_check(args.base, args.head, pr_body) - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/test/test_check_template_propagation.py b/test/test_check_template_propagation.py new file mode 100644 index 000000000..e142db974 --- /dev/null +++ b/test/test_check_template_propagation.py @@ -0,0 +1,67 @@ +import subprocess + +import pytest +from check_template_propagation import find_missing_changes, main, parse_template_path + +pytestmark = pytest.mark.unit + +ALL_FILES = [ + "template/v3/v3.9/Dockerfile", + "template/v4/v4.5/Dockerfile", + "template/v4/v4.5/dirs/a.sh", + "template/v4/v4.6/Dockerfile", + "template/v4/v4.6/dirs/a.sh", +] + + +def test_parse_template_path(): + assert parse_template_path("template/v4/v4.6/dirs/etc/x.sh") == (4, 6, "dirs/etc/x.sh") + assert parse_template_path("template/v4/v3.9/Dockerfile") is None + assert parse_template_path("template/v4/dirs/etc/x.sh") is None + assert parse_template_path("build_artifacts/v4/v4.6/v4.6.0/Dockerfile") is None + + +def test_older_minor_change_must_also_change_the_newest_minor(): + assert find_missing_changes(["template/v4/v4.5/dirs/a.sh"], ALL_FILES) == [ + ("template/v4/v4.5/dirs/a.sh", "template/v4/v4.6/dirs/a.sh") + ] + both = ["template/v4/v4.5/dirs/a.sh", "template/v4/v4.6/dirs/a.sh"] + assert find_missing_changes(both, ALL_FILES) == [] + + +def test_changes_to_the_newest_minor_or_other_files_need_nothing_else(): + assert find_missing_changes(["template/v4/v4.6/dirs/a.sh"], ALL_FILES) == [] + # Each major has its own newest minor: v3.9 is the newest v3 template. + assert find_missing_changes(["template/v3/v3.9/Dockerfile"], ALL_FILES) == [] + assert find_missing_changes(["template/v4/dirs/a.sh", "src/main.py"], ALL_FILES) == [] + + +def test_main_reads_the_git_trees_and_honours_the_label(tmp_path, monkeypatch, capsys): + def git(*args): + command = ["git", "-c", "user.name=test", "-c", "user.email=test@example.com", *args] + return subprocess.run(command, cwd=tmp_path, check=True, capture_output=True, text=True).stdout.strip() + + git("init", "-q") + for minor in (5, 6): + (tmp_path / f"template/v4/v4.{minor}").mkdir(parents=True) + (tmp_path / f"template/v4/v4.{minor}/a.sh").write_text("old\n") + git("add", ".") + git("commit", "-q", "-m", "base") + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("BASE_SHA", git("rev-parse", "HEAD")) + + (tmp_path / "template/v4/v4.5/a.sh").write_text("new\n") + git("commit", "-q", "-a", "-m", "4.5 only") + monkeypatch.setenv("HEAD_SHA", git("rev-parse", "HEAD")) + monkeypatch.setenv("SCOPED", "false") + assert main() == 1 + assert "template/v4/v4.6/a.sh did not" in capsys.readouterr().out + + monkeypatch.setenv("SCOPED", "true") + assert main() == 0 + + (tmp_path / "template/v4/v4.6/a.sh").write_text("new\n") + git("commit", "-q", "-a", "-m", "4.6 too") + monkeypatch.setenv("HEAD_SHA", git("rev-parse", "HEAD")) + monkeypatch.setenv("SCOPED", "false") + assert main() == 0 diff --git a/test/test_template_tools.py b/test/test_template_tools.py deleted file mode 100644 index 8e0e3e529..000000000 --- a/test/test_template_tools.py +++ /dev/null @@ -1,137 +0,0 @@ -import subprocess - -import pytest - -pytestmark = pytest.mark.unit - -from template_tools import ( - find_copy_problems, - find_legacy_path_problems, - find_problems, - find_propagation_problems, - parse_minor_template_path, - run_check, -) - -HEAD_PATHS = { - "template/v3/v3.9/Dockerfile", - "template/v4/v4.5/Dockerfile", - "template/v4/v4.5/dirs/a.sh", - "template/v4/v4.6/Dockerfile", - "template/v4/v4.6/dirs/a.sh", -} - - -def _tree(files): - """path -> blob id => path -> (mode, blob id), as git ls-tree reports it.""" - return {path: ("100644", blob) for path, blob in files.items()} - - -def test_parse_minor_template_path(): - assert parse_minor_template_path("template/v4/v4.6/dirs/etc/x.sh") == (4, 6, "dirs/etc/x.sh") - assert parse_minor_template_path("template/v4/v3.9/Dockerfile") is None - assert parse_minor_template_path("template/v4/dirs/etc/x.sh") is None - assert parse_minor_template_path("build_artifacts/v4/v4.6/v4.6.0/Dockerfile") is None - - -def test_older_minor_change_must_reach_newest_minor(): - assert find_propagation_problems(["template/v4/v4.5/dirs/a.sh"], HEAD_PATHS, scoped=False) == [ - "template/v4/v4.5/dirs/a.sh changed, but template/v4/v4.6/dirs/a.sh did not." - ] - both = ["template/v4/v4.5/dirs/a.sh", "template/v4/v4.6/dirs/a.sh"] - assert find_propagation_problems(both, HEAD_PATHS, scoped=False) == [] - - -def test_newest_minor_and_other_majors_need_nothing_else(): - assert find_propagation_problems(["template/v4/v4.6/dirs/a.sh"], HEAD_PATHS, scoped=False) == [] - assert find_propagation_problems(["template/v3/v3.9/Dockerfile"], HEAD_PATHS, scoped=False) == [] - - -def test_scoped_backport_is_allowed(): - assert find_propagation_problems(["template/v4/v4.5/dirs/a.sh"], HEAD_PATHS, scoped=True) == [] - - -def test_new_minor_must_match_previous_minor(): - base = {"template/v4/v4.6/Dockerfile", "template/v4/v4.6/dirs/a.sh"} - same = _tree( - { - "template/v4/v4.6/Dockerfile": "d1", - "template/v4/v4.6/dirs/a.sh": "a1", - "template/v4/v4.7/Dockerfile": "d1", - "template/v4/v4.7/dirs/a.sh": "a1", - } - ) - assert find_copy_problems(base, same, declared=set()) == [] - - stale = dict(same, **_tree({"template/v4/v4.6/dirs/a.sh": "a2"})) - assert find_copy_problems(base, stale, declared=set()) == [ - "template/v4/v4.7/dirs/a.sh differs from template/v4/v4.6/dirs/a.sh and is not declared." - ] - assert find_copy_problems(base, stale, declared={"template/v4/v4.7/dirs/a.sh"}) == [] - - missing = {p: v for p, v in same.items() if p != "template/v4/v4.7/dirs/a.sh"} - assert find_copy_problems(base, missing, declared=set()) == [ - "template/v4/v4.6/dirs/a.sh is missing from template/v4/v4.7/." - ] - - lost_exec_bit = dict(same, **{"template/v4/v4.6/dirs/a.sh": ("100755", "a1")}) - assert len(find_copy_problems(base, lost_exec_bit, declared=set())) == 1 - - -def test_new_minor_needs_the_minor_before_it(): - head = _tree({"template/v4/v4.6/Dockerfile": "d1", "template/v4/v4.8/Dockerfile": "d1"}) - assert find_copy_problems({"template/v4/v4.6/Dockerfile"}, head, declared=set()) == [ - "template/v4/v4.8/ was added, but there is no template/v4/v4.7/ to copy it from." - ] - - -def test_minor_already_on_main_is_not_compared_again(): - head = _tree({"template/v4/v4.6/Dockerfile": "d1", "template/v4/v4.7/Dockerfile": "d2"}) - assert find_copy_problems(set(head), head, declared=set()) == [] - - -def test_legacy_per_major_paths_are_rejected_once_a_major_has_minor_templates(): - assert find_legacy_path_problems(["template/v4/dirs/etc/x"], HEAD_PATHS) == [ - "template/v4/dirs/etc/x is not in a per-minor template; the build only reads template/v4/v4./." - ] - assert find_legacy_path_problems(["template/v1/dirs/etc/x"], HEAD_PATHS) == [] - - -def test_pr_description_opt_out(): - head = _tree({p: "x" for p in HEAD_PATHS}) - changed = ["template/v4/v4.5/dirs/a.sh"] - assert find_problems(changed, set(head), head, "") != [] - assert find_problems(changed, set(head), head, "Backport only.\n\ntemplate-propagation: scoped\n") == [] - - -def test_run_check_reads_the_git_trees(tmp_path, monkeypatch, capsys): - def git(*args): - result = subprocess.run( - ["git", "-c", "user.name=test", "-c", "user.email=test@example.com", *args], - cwd=tmp_path, - check=True, - capture_output=True, - text=True, - ) - return result.stdout.strip() - - git("init", "-q") - for minor in (5, 6): - (tmp_path / f"template/v4/v4.{minor}").mkdir(parents=True) - (tmp_path / f"template/v4/v4.{minor}/a.sh").write_text("old\n") - (tmp_path / "README.md").write_text("readme\n") - git("add", ".") - git("commit", "-q", "-m", "base") - base = git("rev-parse", "HEAD") - monkeypatch.chdir(tmp_path) - - (tmp_path / "README.md").write_text("readme v2\n") - git("commit", "-q", "-a", "-m", "not a template change") - assert run_check(base, git("rev-parse", "HEAD"), "") == 0 - - (tmp_path / "template/v4/v4.5/a.sh").write_text("new\n") - git("commit", "-q", "-a", "-m", "4.5 only") - head = git("rev-parse", "HEAD") - assert run_check(base, head, "") == 1 - assert "template/v4/v4.6/a.sh did not" in capsys.readouterr().out - assert run_check(base, head, "template-propagation: scoped") == 0