From d357f60105f09d087affa33d25c29ac4ccc76b1a Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sat, 12 Apr 2025 16:49:03 +0200 Subject: [PATCH 1/5] Add `app_slug` support to `required_status_checks` This adds the possibility to use application slugs instead of application numerical ids to specify the required checks for a PR. The slugs for third-party GitHub apps can be found on [GitHub Marketplace]. All slugs can be checked by accessing `https://github.com/apps/`. Two common examples of internal GitHub apps are `github-actions` and `github-advanced-security`. ```yaml github: protected_branches: main: required_status_checks: checks: # A Github Actions workflow name that must pass - context: build / build (ubuntu-latest) app_slug: github-actions # A security check that must pass - context: CodeQL app_slug: github-advanced-security ``` Closes #65 --- README.md | 18 +++++- asfyaml/feature/github/__init__.py | 27 ++++++--- asfyaml/feature/github/branch_protection.py | 45 +++++++++------ tests/github_branch_protection.py | 64 +++++++++++++++++++++ 4 files changed, 126 insertions(+), 28 deletions(-) create mode 100644 tests/github_branch_protection.py diff --git a/README.md b/README.md index 8ec190e..13e5ae9 100644 --- a/README.md +++ b/README.md @@ -423,6 +423,7 @@ required_status_checks: checks: - context: app_id: + app_slug: ``` If not explicitly specified, these values will be used by default: @@ -439,14 +440,18 @@ required_pull_request_reviews: required_status_checks: strict: false contexts: ~ - checks: ~ + checks: ``` **Notes** 1. Enabling any of the above checks overrides what you may have set previously, so you'll need to add all the existing checks to your `.asf.yaml` file to reproduce any that Infra set manually for you. 2. If you need to remove a required check in order to push a change to `.asf.yaml`, create an Infra Jira ticket with a request to have the check manually removed. -Using the 'contexts' list will automatically set an app ID of `-1` (any source) for checks. If you wish to specify a specific source app ID, you can make use of the expanded `checks` list instead: +Using the 'contexts' list will automatically set an app ID of `-1` (any source) for checks. If you wish to specify a specific source app, you can make use of the expanded `checks` list instead and provide: + +- either an `app_slug` like `github-actions` or `github-advanced-security`. The correctness of the slug can be checked + accessing the URL `https://github.com/apps/`, e.g. https://github.com/apps/github-actions. +- or an `app_id` ~~~yaml github: @@ -456,10 +461,17 @@ github: # strict means "Require branches to be up to date before merging". strict: true checks: + # A Github Actions workflow name that must pass + - context: build / build (ubuntu-latest) + app_slug: github-actions + # A security check that must pass + - context: CodeQL + app_slug: github-advanced-security + # GitHub App specified by id - context: gh-infra/jenkins app_id: 1234 + # Equivalent to any GitHub App - context: another/build-that-must-pass - app_id: -1 ... ~~~ diff --git a/asfyaml/feature/github/__init__.py b/asfyaml/feature/github/__init__.py index 5b39eb0..31a42ac 100644 --- a/asfyaml/feature/github/__init__.py +++ b/asfyaml/feature/github/__init__.py @@ -99,8 +99,7 @@ class ASFGitHubFeature(ASFYamlFeature, name="github"): strictyaml.Optional("ghp_branch"): strictyaml.Str(), strictyaml.Optional("ghp_path", default="/docs"): strictyaml.Str(), # Branch protection rules - TODO: add actual schema - strictyaml.Optional("protected_branches"): asfyaml.validators.EmptyValue() - | strictyaml.MapPattern( + strictyaml.Optional("protected_branches"): strictyaml.MapPattern( strictyaml.Str(), strictyaml.Map( { @@ -122,7 +121,8 @@ class ASFGitHubFeature(ASFYamlFeature, name="github"): strictyaml.Optional("checks"): strictyaml.Seq( strictyaml.Map( { - strictyaml.Optional("context"): strictyaml.Str(), + "context": strictyaml.Str(), + strictyaml.Optional("app_slug"): strictyaml.Str(), strictyaml.Optional("app_id", default=-1): strictyaml.Int(), } ) @@ -142,6 +142,7 @@ class ASFGitHubFeature(ASFYamlFeature, name="github"): def __init__(self, parent: ASFYamlInstance, yaml: strictyaml.YAML, **kwargs): super().__init__(parent, yaml) + self._github: pygithub.Github | None = None self._ghrepo: pygithubrepo.Repository | None = None @property @@ -151,6 +152,18 @@ def ghrepo(self) -> pygithubrepo.Repository: else: return self._ghrepo + def _get_app_id_slug(self, slug: str) -> int | None: + # Test run + if "noop" in self.instance.environments_enabled: + return 1234 + if self._github is None: + raise RuntimeError("something went wrong, github is not set") + try: + return self._github.get_app(slug).id + except pygithub.GithubException as e: + print(f"[github] Unable to find GitHub app for slug {slug}.") + return None + def run(self): """GitHub features""" # Test if we need to process this (only works on the default branch) @@ -183,11 +196,11 @@ def run(self): if not gh_token: gh_token = open(GH_TOKEN_FILE).read().strip() - pgh = pygithub.Github(auth=pygithubAuth.Token(gh_token)) - self._ghrepo = pgh.get_repo(f"{self.repository.org_id}/{self.repository.name}") + self._github = pygithub.Github(auth=pygithubAuth.Token(gh_token)) + self._ghrepo = self._github.get_repo(f"{self.repository.org_id}/{self.repository.name}") elif gh_token: # If supplied from OS env, load the ghrepo object anyway - pgh = pygithub.Github(auth=pygithubAuth.Token(gh_token)) - self._ghrepo = pgh.get_repo(f"{self.repository.org_id}/{self.repository.name}") + self._github = pygithub.Github(auth=pygithubAuth.Token(gh_token)) + self._ghrepo = self._github.get_repo(f"{self.repository.org_id}/{self.repository.name}") # For each sub-feature we see (with the @directive decorator on it), run it for _feat in _features: diff --git a/asfyaml/feature/github/branch_protection.py b/asfyaml/feature/github/branch_protection.py index ec3d907..0aea0cb 100644 --- a/asfyaml/feature/github/branch_protection.py +++ b/asfyaml/feature/github/branch_protection.py @@ -83,15 +83,23 @@ def get_head_refs(self: ASFGitHubFeature) -> list[Mapping[str, Any]]: return [] +def __get_app_id_check(self: ASFGitHubFeature, check: dict) -> int: + app_slug = check.get("app_slug") + app_id = check.get("app_id") if app_slug is None else self._get_app_id_slug(app_slug) + return -1 if app_id is None else int(app_id) + + @directive def branch_protection(self: ASFGitHubFeature): # Branch protections if "protected_branches" not in self.yaml: return + # Testing mode + dry_run: bool = self.noop("protected_branches") # Collect all branches and whether they have active branch protection rules try: - refs = get_head_refs(self) + refs = get_head_refs(self) if not dry_run else [] except Exception as ex: print(f"Error: failed to retrieve current refs: {ex!s}") refs = [] @@ -115,15 +123,6 @@ def branch_protection(self: ASFGitHubFeature): protected_branches.remove(branch) branch_changes = [] - try: - ghbranch = self.ghrepo.get_branch(branch=branch) - except pygithub.GithubException as e: - if e.status == 404: # No such branch, skip to next rule - protection_changes[branch] = [f"Branch {branch} does not exist, protection could not be configured"] - continue - else: - # propagate other errors, GitHub API might have an outage - raise e # We explicitly disable force pushes when branch protections are enabled allow_force_push = False @@ -165,7 +164,8 @@ def branch_protection(self: ASFGitHubFeature): contexts = required_status_checks.get("contexts", []) checks = required_status_checks.get("checks", []) - checks_as_dict = {**{ctx: -1 for ctx in contexts}, **{c["context"]: int(c["app_id"]) for c in checks}} + checks_as_dict = {**{ctx: -1 for ctx in contexts}, + **{c["context"]: __get_app_id_check(self, c) for c in checks}} required_checks = list(checks_as_dict.items()) @@ -179,7 +179,7 @@ def branch_protection(self: ASFGitHubFeature): # Log changes that will be applied try: - live_branch_protection_settings = ghbranch.get_protection() + live_branch_protection_settings = self.ghrepo.get_branch(branch=branch).get_protection() if not dry_run else None except pygithub.GithubException: live_branch_protection_settings = None @@ -254,7 +254,16 @@ def branch_protection(self: ASFGitHubFeature): branch_changes.append(f" - {ctx} (app_id: {appid})") # Apply all the changes - if not self.noop("protected_branches"): + if not dry_run: + try: + ghbranch = self.ghrepo.get_branch(branch=branch) + except pygithub.GithubException as e: + if e.status == 404: # No such branch, skip to next rule + protection_changes[branch] = [f"Branch {branch} does not exist, protection could not be configured"] + continue + else: + # propagate other errors, GitHub API might have an outage + raise e branch_protection_settings = ghbranch.edit_protection( allow_force_pushes=allow_force_push, required_linear_history=required_linear, @@ -292,16 +301,16 @@ def branch_protection(self: ASFGitHubFeature): # remove branch protection from all remaining protected branches for branch_name in protected_branches: - branch = self.ghrepo.get_branch(branch_name) - protection_changes[branch] = [f"Remove branch protection from branch '{branch_name}'"] + protection_changes[branch_name] = [f"Remove branch protection from branch '{branch_name}'"] - if not self.noop("github::protected_branches"): + if not dry_run: + branch = self.ghrepo.get_branch(branch_name) branch.remove_protection() if protection_changes: summary = "" - for branch, changes in protection_changes.items(): - summary += f"Updates to the {branch} branch:\n" + for branch_name, changes in protection_changes.items(): + summary += f"Updates to the {branch_name} branch:\n" for change in changes: summary += f" - {change}\n" print(summary) diff --git a/tests/github_branch_protection.py b/tests/github_branch_protection.py new file mode 100644 index 0000000..f569bac --- /dev/null +++ b/tests/github_branch_protection.py @@ -0,0 +1,64 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Unit tests for .asf.yaml GitHub branch protection""" +import strictyaml.exceptions + +from helpers import YamlTest +import asfyaml.asfyaml +import asfyaml.dataobjects + +# Set .asf.yaml to debug mode +asfyaml.asfyaml.DEBUG = True + + +valid_github_checks = YamlTest( + None, + None, + """ +github: + protected_branches: + main: + required_status_checks: + checks: + # Only slug + - context: "check1" + app_slug: github-actions + # Only id + - context: "check2" + app_id: 15368 + # Only context + - context: "check3" +""", +) + + +def test_basic_yaml(test_repo: asfyaml.dataobjects.Repository): + print("[github] Testing branch protection") + + tests_to_run = ( + valid_github_checks, + ) + + for test in tests_to_run: + with test.ctx(): + a = asfyaml.asfyaml.ASFYamlInstance( + repo=test_repo, committer="humbedooh", config_data=test.yaml, branch=asfyaml.dataobjects.DEFAULT_BRANCH + ) + a.environments_enabled.add("noop") + a.no_cache = True + a.run_parts() From 34df38ea2835df5c87263e04e728f8727f9a13b1 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Sun, 13 Apr 2025 21:37:30 +0200 Subject: [PATCH 2/5] apply ruff to reformat --- asfyaml/feature/github/__init__.py | 2 +- asfyaml/feature/github/branch_protection.py | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/asfyaml/feature/github/__init__.py b/asfyaml/feature/github/__init__.py index 31a42ac..499e271 100644 --- a/asfyaml/feature/github/__init__.py +++ b/asfyaml/feature/github/__init__.py @@ -160,7 +160,7 @@ def _get_app_id_slug(self, slug: str) -> int | None: raise RuntimeError("something went wrong, github is not set") try: return self._github.get_app(slug).id - except pygithub.GithubException as e: + except pygithub.GithubException: print(f"[github] Unable to find GitHub app for slug {slug}.") return None diff --git a/asfyaml/feature/github/branch_protection.py b/asfyaml/feature/github/branch_protection.py index 0aea0cb..c1165ed 100644 --- a/asfyaml/feature/github/branch_protection.py +++ b/asfyaml/feature/github/branch_protection.py @@ -164,8 +164,10 @@ def branch_protection(self: ASFGitHubFeature): contexts = required_status_checks.get("contexts", []) checks = required_status_checks.get("checks", []) - checks_as_dict = {**{ctx: -1 for ctx in contexts}, - **{c["context"]: __get_app_id_check(self, c) for c in checks}} + checks_as_dict = { + **{ctx: -1 for ctx in contexts}, + **{c["context"]: __get_app_id_check(self, c) for c in checks}, + } required_checks = list(checks_as_dict.items()) @@ -179,7 +181,9 @@ def branch_protection(self: ASFGitHubFeature): # Log changes that will be applied try: - live_branch_protection_settings = self.ghrepo.get_branch(branch=branch).get_protection() if not dry_run else None + live_branch_protection_settings = ( + self.ghrepo.get_branch(branch=branch).get_protection() if not dry_run else None + ) except pygithub.GithubException: live_branch_protection_settings = None From cd9d70ffb34915eb0653ce05e05aeb94a6b2fc5e Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 14 Apr 2025 20:51:05 +0200 Subject: [PATCH 3/5] Merge `checks` and `contexts` --- README.md | 25 ++++++++-------- asfyaml/feature/github/__init__.py | 22 +++----------- asfyaml/feature/github/branch_protection.py | 33 ++++++++++++++------- tests/github_branch_protection.py | 15 +++++----- 4 files changed, 45 insertions(+), 50 deletions(-) diff --git a/README.md b/README.md index 581aa92..226b3b0 100644 --- a/README.md +++ b/README.md @@ -388,7 +388,8 @@ github: # contexts are the names of checks that must pass. contexts: - gh-infra/jenkins - - another/build-that-must-pass + - context: CodeQL + app: github-advanced-security required_pull_request_reviews: dismiss_stale_reviews: true require_last_push_approval: false @@ -421,10 +422,8 @@ required_status_checks: strict: contexts: - - checks: - context: - app_id: - app_slug: + app: or ``` If not explicitly specified, these values will be used by default: @@ -441,18 +440,17 @@ required_pull_request_reviews: required_status_checks: strict: false contexts: ~ - checks: ``` **Notes** 1. Enabling any of the above checks overrides what you may have set previously, so you'll need to add all the existing checks to your `.asf.yaml` file to reproduce any that Infra set manually for you. 2. If you need to remove a required check in order to push a change to `.asf.yaml`, create an Infra Jira ticket with a request to have the check manually removed. -Using the 'contexts' list will automatically set an app ID of `-1` (any source) for checks. If you wish to specify a specific source app, you can make use of the expanded `checks` list instead and provide: - -- either an `app_slug` like `github-actions` or `github-advanced-security`. The correctness of the slug can be checked - accessing the URL `https://github.com/apps/`, e.g. https://github.com/apps/github-actions. -- or an `app_id` +The `contexts` list allows two kinds of entries: +- If you wish to specify a specific source app, you need to provide a `context` property for the name of the check and an `app` property for the app. + You can use either the application's id or its slug. + The correctness of the slug can be checked accessing the URL `https://github.com/apps/`, e.g. https://github.com/apps/github-actions. +- Otherwise, you can just specify the name of the check. ~~~yaml github: @@ -464,15 +462,16 @@ github: checks: # A Github Actions workflow name that must pass - context: build / build (ubuntu-latest) - app_slug: github-actions + app: github-actions # A security check that must pass - context: CodeQL - app_slug: github-advanced-security + app: github-advanced-security # GitHub App specified by id - context: gh-infra/jenkins - app_id: 1234 + app: 1234 # Equivalent to any GitHub App - context: another/build-that-must-pass + - a-third/build-that-must-pass ... ~~~ diff --git a/asfyaml/feature/github/__init__.py b/asfyaml/feature/github/__init__.py index 7700340..17fee7d 100644 --- a/asfyaml/feature/github/__init__.py +++ b/asfyaml/feature/github/__init__.py @@ -117,16 +117,13 @@ class ASFGitHubFeature(ASFYamlFeature, name="github"): strictyaml.Optional("required_status_checks"): strictyaml.Map( { strictyaml.Optional("strict", default=False): strictyaml.Bool(), - strictyaml.Optional("contexts"): strictyaml.Seq(strictyaml.Str()), - strictyaml.Optional("checks"): strictyaml.Seq( - strictyaml.Map( + strictyaml.Optional("contexts"): strictyaml.Seq( + strictyaml.Str() | strictyaml.Map( { "context": strictyaml.Str(), - strictyaml.Optional("app_slug"): strictyaml.Str(), - strictyaml.Optional("app_id", default=-1): strictyaml.Int(), + strictyaml.Optional("app"): strictyaml.Int() | strictyaml.Str(), } - ) - ), + )) } ), } @@ -191,17 +188,6 @@ def ghrepo(self) -> pygithubrepo.Repository: else: return self._ghrepo - def _get_app_id_slug(self, slug: str) -> int | None: - # Test run - if "noop" in self.instance.environments_enabled: - return 1234 - if self._github is None: - raise RuntimeError("something went wrong, github is not set") - try: - return self._github.get_app(slug).id - except pygithub.GithubException: - print(f"[github] Unable to find GitHub app for slug {slug}.") - return None def run(self): """GitHub features""" diff --git a/asfyaml/feature/github/branch_protection.py b/asfyaml/feature/github/branch_protection.py index c1165ed..5328549 100644 --- a/asfyaml/feature/github/branch_protection.py +++ b/asfyaml/feature/github/branch_protection.py @@ -83,10 +83,27 @@ def get_head_refs(self: ASFGitHubFeature) -> list[Mapping[str, Any]]: return [] -def __get_app_id_check(self: ASFGitHubFeature, check: dict) -> int: - app_slug = check.get("app_slug") - app_id = check.get("app_id") if app_slug is None else self._get_app_id_slug(app_slug) - return -1 if app_id is None else int(app_id) +def __context_get_name(context: str | dict) -> str: + return context if type(context) is str else context["context"] + + +def __slug_get_app_id(self: ASFGitHubFeature, slug: str) -> int | None: + # Test run + if "noop" in self.instance.environments_enabled: + return 1234 + try: + return self.gh.get_app(slug).id + except pygithub.GithubException: + print(f"[github] Unable to find GitHub app for slug {slug}.") + return None + + +# Get the application id for a protected branch check +def __context_get_app_id(self: ASFGitHubFeature, context: str | dict) -> int: + if type(context) is str: + return -1 + app = context.get("app") + return -1 if app is None else __slug_get_app_id(self, app) if type(app) is str else int(app) @directive @@ -163,13 +180,7 @@ def branch_protection(self: ASFGitHubFeature): require_strict = required_status_checks.get("strict", NotSet) contexts = required_status_checks.get("contexts", []) - checks = required_status_checks.get("checks", []) - checks_as_dict = { - **{ctx: -1 for ctx in contexts}, - **{c["context"]: __get_app_id_check(self, c) for c in checks}, - } - - required_checks = list(checks_as_dict.items()) + required_checks = list((__context_get_name(c), __context_get_app_id(self, c)) for c in contexts) # if no checks are defined, we remove the status checks completely if len(required_checks) == 0: diff --git a/tests/github_branch_protection.py b/tests/github_branch_protection.py index f569bac..a25d751 100644 --- a/tests/github_branch_protection.py +++ b/tests/github_branch_protection.py @@ -16,8 +16,6 @@ # under the License. """Unit tests for .asf.yaml GitHub branch protection""" -import strictyaml.exceptions - from helpers import YamlTest import asfyaml.asfyaml import asfyaml.dataobjects @@ -34,15 +32,16 @@ protected_branches: main: required_status_checks: - checks: + contexts: + - "check1" # Only slug - - context: "check1" - app_slug: github-actions - # Only id - context: "check2" - app_id: 15368 - # Only context + app: "github-actions" + # Only id - context: "check3" + app: 15368 + # Only context + - context: "check4" """, ) From 5d02fdd3f2d74e4f41038d1f1cb2d1ff9408eb55 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 14 Apr 2025 20:54:38 +0200 Subject: [PATCH 4/5] Fix formatting issues --- asfyaml/feature/github/__init__.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/asfyaml/feature/github/__init__.py b/asfyaml/feature/github/__init__.py index 17fee7d..9b114b3 100644 --- a/asfyaml/feature/github/__init__.py +++ b/asfyaml/feature/github/__init__.py @@ -118,12 +118,14 @@ class ASFGitHubFeature(ASFYamlFeature, name="github"): { strictyaml.Optional("strict", default=False): strictyaml.Bool(), strictyaml.Optional("contexts"): strictyaml.Seq( - strictyaml.Str() | strictyaml.Map( + strictyaml.Str() + | strictyaml.Map( { "context": strictyaml.Str(), strictyaml.Optional("app"): strictyaml.Int() | strictyaml.Str(), } - )) + ) + ), } ), } @@ -188,7 +190,6 @@ def ghrepo(self) -> pygithubrepo.Repository: else: return self._ghrepo - def run(self): """GitHub features""" # Test if we need to process this (only works on the default branch) From bd4d7cbe76d46e989b7c6692c52d08122eab8c36 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 14 Apr 2025 21:09:07 +0200 Subject: [PATCH 5/5] Fix mypy errors --- asfyaml/feature/github/branch_protection.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/asfyaml/feature/github/branch_protection.py b/asfyaml/feature/github/branch_protection.py index 5328549..f6e7c7b 100644 --- a/asfyaml/feature/github/branch_protection.py +++ b/asfyaml/feature/github/branch_protection.py @@ -84,7 +84,7 @@ def get_head_refs(self: ASFGitHubFeature) -> list[Mapping[str, Any]]: def __context_get_name(context: str | dict) -> str: - return context if type(context) is str else context["context"] + return context if isinstance(context, str) else context["context"] def __slug_get_app_id(self: ASFGitHubFeature, slug: str) -> int | None: @@ -100,10 +100,12 @@ def __slug_get_app_id(self: ASFGitHubFeature, slug: str) -> int | None: # Get the application id for a protected branch check def __context_get_app_id(self: ASFGitHubFeature, context: str | dict) -> int: - if type(context) is str: + if isinstance(context, str): return -1 app = context.get("app") - return -1 if app is None else __slug_get_app_id(self, app) if type(app) is str else int(app) + if isinstance(app, str): + app = __slug_get_app_id(self, app) + return -1 if app is None else int(app) @directive