From 63690eb89adf6fb9324c9ed083324b1a38b2795e Mon Sep 17 00:00:00 2001 From: Nikola Milosavljevic <73236646+NikolaMilosa@users.noreply.github.com> Date: Tue, 25 Jun 2024 14:12:39 +0200 Subject: [PATCH] feat: ci check for release index (#526) --- .github/workflows/release.yaml | 10 ++ .pre-commit-config.yaml | 2 +- release-controller/ci_check.py | 196 ++++++++++++++++++++++---- release-controller/git_repo.py | 91 ++++++++++++ release-controller/release_index.py | 21 --- release-controller/test_reconciler.py | 6 - release-index-schema.json | 76 +--------- release-index.yaml | 38 +---- 8 files changed, 281 insertions(+), 159 deletions(-) diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 98d80747c..b6dc78336 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -6,6 +6,10 @@ on: jobs: changed_files: runs-on: ubuntu-latest + # runs-on: + # labels: dre-runner-custom + # # This image is based on ubuntu:20.04 + # container: ghcr.io/dfinity/dre/actions-runner:0.2.1 name: Test changed-files steps: - uses: actions/checkout@v4 @@ -27,3 +31,9 @@ jobs: description: 'Passed' state: 'success' sha: ${{github.event.pull_request.head.sha || github.sha}} + + # - name: Run checks for release index + # Uncomment once the testing is finished + # if: ${{ steps.changed-files.outputs.all_changed_files_count > 0 && steps.changed-files.outputs.other_changed_files_count == 0 }} + # run: | + # python3 release-controller/ci_check.py --repo-path /home/runner/.cache diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 701b8fc01..c17f82554 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -67,7 +67,7 @@ repos: - --match=.* - repo: https://github.com/PyCQA/pylint - rev: v2.12.2 + rev: v2.17.7 hooks: - id: pylint name: pylint diff --git a/release-controller/ci_check.py b/release-controller/ci_check.py index d3a1efe4b..10181dfd9 100644 --- a/release-controller/ci_check.py +++ b/release-controller/ci_check.py @@ -1,25 +1,171 @@ -# TODO: -# validate that realease_notes_ready: false if release doesn't exist on main -# validate that there's no double entries in override_versions -# validate that there's no duplicates in rollout plan -# validate that there are no subnets missing in rollout plan -# validate that there's a stage to update unassigned nodes -# check that all versions within same release have an unique name -# check that all rollout versions (default and override_versions) have valid entries in the release -# TODO: instead of doing this, we can just halt the rollout if the version is missing -# check that commits are ordered linearly in each release -# check that releases are ordered linearly -# check that previous rollout finished -# check that versions from a release cannot be removed if notes were published to the forum -# check that version exists on ic repo, unless it's marked as a security fix -# validate that excludes_subnets are present on the rollout plan (i.e. valid subnets) -# validate that wait_for_next_week is only set for last stage -# check that version belongs to specified RC - -# TODO: additionally consider these -# generate rollout plan to PR if it's different from main branch - how would that look like? -# write all failed validations as a comment on the PR, otherwise just generate a test report in a nice way -# instruct user to remove old RC from the index - we don't need this currently, these versions will be ignored by reconciler in any case - -# TODO: other things to consider -# proposed version can be rejected. +import argparse +import json +import os +import pathlib +from datetime import datetime + +import yaml +from colorama import Fore +from git_repo import GitRepo +from jsonschema import validate +from release_index import ReleaseIndex +from release_index_loader import ReleaseLoader + +BASE_VERSION_NAME = "base" + + +def parse_args(): + parser = argparse.ArgumentParser(description="Tool for checking release index") + parser.add_argument("--path", type=str, dest="path", help="Path to the release index", default="release-index.yaml") + parser.add_argument( + "--schema-path", + type=str, + dest="schema_path", + help="Path to the release index schema", + default="release-index-schema.json", + ) + parser.add_argument( + "--repo-path", type=str, dest="repo_path", help="Path to the repo", default=os.environ.get("IC_REPO_PATH") + ) + + return parser.parse_args() + + +def success_print(message: str): + print(f"{Fore.GREEN}{message}{Fore.RESET}") + + +def error_print(message: str): + print(f"{Fore.RED}{message}{Fore.RESET}") + + +def warn_print(message: str): + print(f"{Fore.YELLOW}{message}{Fore.RESET}") + + +def validate_schema(index: dict, schema_path: str): + with open(schema_path, "r", encoding="utf8") as f: + schema = json.load(f) + try: + validate(instance=index, schema=schema) + except Exception as e: + error_print(f"Schema validation failed: \n{e}") + exit(1) + + success_print("Schema validation passed") + + +def check_if_commits_really_exist(index: ReleaseIndex, repo: GitRepo): + for release in index.releases: + for version in release.versions: + commit = repo.show(version.version) + if commit is None: + error_print(f"Commit {version.version} does not exist") + exit(1) + + success_print("All commits exist") + + +def check_if_there_is_a_base_version(index: ReleaseIndex): + for release in index.releases: + found = False + for version in release.versions: + if version.name == BASE_VERSION_NAME: + found = True + break + if not found: + error_print(f"Release {release.rc_name} does not have a base version") + exit(1) + + success_print("All releases have a base version") + + +def check_unique_version_names_within_release(index: ReleaseIndex): + for release in index.releases: + version_names = set() + for version in release.versions: + if version.name in version_names: + error_print( + f"Version {version.name} in release {release.rc_name} has the same name as another version from the same release" + ) + exit(1) + version_names.add(version.name) + + success_print("All version names are unique within the respective releases") + + +def check_version_to_tags_consistency(index: ReleaseIndex, repo: GitRepo): + for release in index.releases: + for version in release.versions: + tag_name = f"release-{release.rc_name.removeprefix('rc--')}-{version.name}" + tag = repo.show(tag_name) + commit = repo.show(version.version) + if tag is None: + warn_print(f"Tag {tag_name} does not exist") + continue + if tag.sha != commit.sha: + error_print(f"Tag {tag_name} points to {tag.sha} not {commit.sha}") + exit(1) + + success_print("Finished consistency check") + + +def check_rc_order(index: ReleaseIndex): + date_format = "%Y-%m-%d_%H-%M" + parsed = [ + {"name": release.rc_name, "date": datetime.strptime(release.rc_name.removeprefix("rc--"), date_format)} + for release in index.releases + ] + + for i in range(1, len(parsed)): + if parsed[i]["date"] > parsed[i - 1]["date"]: + error_print(f"Release {parsed[i]['name']} is older than {parsed[i - 1]['name']}") + exit(1) + + success_print("All RC's are ordered descending by date") + + +def check_versions_on_specific_branches(index: ReleaseIndex, repo: GitRepo): + for release in index.releases: + for version in release.versions: + commit = repo.show(version.version) + if commit is None: + error_print(f"Commit {version.version} does not exist") + exit(1) + if release.rc_name not in commit.branches: + error_print( + f"Commit {version.version} is not on branch {release.rc_name}. Commit found on brances: {', '.join(commit.branches)}" + ) + exit(1) + + success_print("All versions are on the correct branches") + + +if __name__ == "__main__": + args = parse_args() + print( + "Checking release index at '%s' against schmea at '%s' and repo at '%s'" + % (args.path, args.schema_path, args.repo_path) + ) + index = yaml.load(open(args.path, "r", encoding="utf8"), Loader=yaml.FullLoader) + + validate_schema(index, args.schema_path) + + index = ReleaseLoader(pathlib.Path(args.path).parent).index().root + + check_if_there_is_a_base_version(index) + check_unique_version_names_within_release(index) + check_rc_order(index) + + repo = GitRepo( + "https://github.com/dfinity/ic.git", repo_cache_dir=pathlib.Path(args.repo_path).parent, main_branch="master" + ) + repo.fetch() + repo.ensure_branches([release.rc_name for release in index.releases]) + + check_if_commits_really_exist(index, repo) + check_versions_on_specific_branches(index, repo) + check_version_to_tags_consistency(index, repo) + # Check that versions from a release cannot be removed if notes were published to the forum + + success_print("All checks passed") diff --git a/release-controller/git_repo.py b/release-controller/git_repo.py index a976da639..9738e02cb 100644 --- a/release-controller/git_repo.py +++ b/release-controller/git_repo.py @@ -8,6 +8,20 @@ from release_index import Version +class Commit: + """Class for representing a git commit.""" + + def __init__( + self, sha: str, message: str, author: str, date: str, branches: list[str] = [] + ): # pylint: disable=dangerous-default-value + """Create a new Commit object.""" + self.sha = sha + self.message = message + self.author = author + self.date = date + self.branches = branches + + class GitRepo: """Class for interacting with a git repository.""" @@ -24,12 +38,89 @@ def __init__(self, repo: str, repo_cache_dir=pathlib.Path.home() / ".cache/git", repo_cache_dir = pathlib.Path(self.cache_temp_dir.name) self.dir = repo_cache_dir / (repo.split("@", 1)[1] if "@" in repo else repo.removeprefix("https://")) + self.cache = {} def __del__(self): """Clean up the temporary directory.""" if hasattr(self, "cache_temp_dir"): self.cache_temp_dir.cleanup() + def ensure_branches(self, branches: list[str]): + """Ensure that the given branches exist.""" + for branch in branches: + try: + subprocess.check_call( + ["git", "checkout", branch], + cwd=self.dir, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + except subprocess.CalledProcessError: + print("Branch {} does not exist".format(branch)) + + subprocess.check_call( + ["git", "checkout", self.main_branch], + cwd=self.dir, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + + def show(self, obj: str) -> Commit | None: + """Show the commit for the given object.""" + if obj in self.cache: + return self.cache[obj] + + try: + result = subprocess.run( + [ + "git", + "show", + "--no-patch", + "--format=%H%n%B%n%an%n%ad", + obj, + ], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + check=True, + cwd=self.dir, + ) + + output = result.stdout.strip().splitlines() + + commit = Commit(output[0], output[1], output[2], output[3]) + except subprocess.CalledProcessError: + return None + + try: + branch_result = subprocess.run( + ["git", "branch", "--contains", commit.sha], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + check=True, + cwd=self.dir, + ) + + # Parse the result of the git branch command + branches = branch_result.stdout.strip().splitlines() + for branch in branches: + branch = branch.strip() + if branch.startswith("* "): + branch = branch[2:] + if "remotes/origin/HEAD" in branch: + continue + if branch.startswith("remotes/origin/"): + branch = branch[len("remotes/origin/") :] + commit.branches.append(branch) + + except subprocess.CalledProcessError: + return None + + self.cache[obj] = commit + + return commit + def fetch(self): """Fetch the repository.""" if (self.dir / ".git").exists(): diff --git a/release-controller/release_index.py b/release-controller/release_index.py index 5be86307b..ac5dd0d3e 100644 --- a/release-controller/release_index.py +++ b/release-controller/release_index.py @@ -3,7 +3,6 @@ from __future__ import annotations -from datetime import date from typing import List, Optional from pydantic import BaseModel, ConfigDict, RootModel @@ -19,16 +18,6 @@ class Version(BaseModel): subnets: Optional[List[str]] = None -class Stage(BaseModel): - model_config = ConfigDict( - extra='forbid', - ) - subnets: Optional[List[str]] = None - bake_time: Optional[str] = None - update_unassigned_nodes: Optional[bool] = None - wait_for_next_week: Optional[bool] = None - - class Release(BaseModel): model_config = ConfigDict( extra='forbid', @@ -37,20 +26,10 @@ class Release(BaseModel): versions: List[Version] -class Rollout(BaseModel): - model_config = ConfigDict( - extra='forbid', - ) - pause: Optional[bool] = None - skip_days: Optional[List[date]] = None - stages: List[Stage] - - class ReleaseIndex(BaseModel): model_config = ConfigDict( extra='forbid', ) - rollout: Rollout releases: List[Release] diff --git a/release-controller/test_reconciler.py b/release-controller/test_reconciler.py index 261c2912d..14b041344 100644 --- a/release-controller/test_reconciler.py +++ b/release-controller/test_reconciler.py @@ -143,9 +143,6 @@ def test_versions_to_unelect(): index = parse_yaml_raw_as( release_index.Model, """ -rollout: - stages: [] - releases: - rc_name: rc--2024-02-21_23-01 versions: @@ -223,9 +220,6 @@ def test_oldest_active_release(): index = parse_yaml_raw_as( release_index.Model, """ -rollout: - stages: [] - releases: - rc_name: rc--2024-02-21_23-01 versions: diff --git a/release-index-schema.json b/release-index-schema.json index ee9baffc5..b1d486845 100644 --- a/release-index-schema.json +++ b/release-index-schema.json @@ -1,14 +1,11 @@ { "$schema": "http://json-schema.org/draft-06/schema#", - "$ref": "#/definitions/ReleaseIndex", + "$ref": "#/definitions/Welcome4", "definitions": { - "ReleaseIndex": { + "Welcome4": { "type": "object", "additionalProperties": false, "properties": { - "rollout": { - "$ref": "#/definitions/Rollout" - }, "releases": { "type": "array", "items": { @@ -17,10 +14,9 @@ } }, "required": [ - "releases", - "rollout" + "releases" ], - "title": "ReleaseIndex" + "title": "Welcome4" }, "Release": { "type": "object", @@ -46,20 +42,11 @@ "type": "object", "additionalProperties": false, "properties": { - "version": { - "type": "string" - }, "name": { "type": "string" }, - "release_notes_ready": { - "type": "boolean" - }, - "subnets": { - "type": "array", - "items": { - "type": "string" - } + "version": { + "type": "string" } }, "required": [ @@ -67,55 +54,6 @@ "version" ], "title": "Version" - }, - "Rollout": { - "type": "object", - "additionalProperties": false, - "properties": { - "pause": { - "type": "boolean" - }, - "skip_days": { - "type": "array", - "items": { - "type": "string", - "format": "date" - } - }, - "stages": { - "type": "array", - "items": { - "$ref": "#/definitions/Stage" - } - } - }, - "required": [ - "stages" - ], - "title": "Rollout" - }, - "Stage": { - "type": "object", - "additionalProperties": false, - "properties": { - "subnets": { - "type": "array", - "items": { - "type": "string" - } - }, - "bake_time": { - "type": "string" - }, - "update_unassigned_nodes": { - "type": "boolean" - }, - "wait_for_next_week": { - "type": "boolean" - } - }, - "required": [], - "title": "Stage" } } -} \ No newline at end of file +} diff --git a/release-index.yaml b/release-index.yaml index 5059d2c4a..cfa127095 100644 --- a/release-index.yaml +++ b/release-index.yaml @@ -1,39 +1,3 @@ -rollout: - pause: false - skip_days: - - 2024-02-26 - stages: - - subnets: [io67a] - bake_time: 8h - - subnets: [shefu, uzr34] - bake_time: 4h - - subnets: [pjljw, qdvhd, bkfrj] - bake_time: 4h - - subnets: [snjp4, w4asl, qxesv] - bake_time: 2h - - subnets: [4zbus, ejbmu, 2fq7c] - bake_time: 2h - - subnets: [pae4o, 5kdm2, csyj4] - bake_time: 2h - - subnets: [eq6en, lhg73, brlsh] - bake_time: 2h - - subnets: [k44fs, cv73p, 4ecnw] - bake_time: 1h - - subnets: [opn46, lspz2, o3ow2] - bake_time: 1h - - subnets: [w4rem, 6pbhf, e66qm] - bake_time: 1h - - subnets: [yinp6, fuqsr, jtdsg] - bake_time: 30m - - subnets: [mpubz, x33ed, pzp6e] - bake_time: 30m - - subnets: [3hhby, nl6hn, gmq5v] - bake_time: 30m - - update_unassigned_nodes: true - - subnets: [tdb26] - bake_time: 2h - wait_for_next_week: true - releases: - rc_name: rc--2024-06-19_23-01 versions: @@ -158,7 +122,7 @@ releases: version: 425a0012aeb40008e2e72d913318bc9dbdf3b4f4 - rc_name: rc--2024-03-06_23-01 versions: - - name: default + - name: base version: 778d2bb870f858952ca9fbe69324f9864e3cf5e7 - name: p2p version: fff20526e154f8b8d24373efd9b50f588d147e91