From 2ba5b7fd231b8abca0bf14b8896ad12c25565eb3 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Fri, 15 Mar 2024 16:57:59 +0100 Subject: [PATCH] Refactor labels in CI, move them to shared package --- tests/ci/build_check.py | 34 +++++++++---------- .../cancel_and_rerun_workflow_lambda/app.py | 6 ++-- tests/ci/cherry_pick.py | 25 +++++--------- tests/ci/commit_status_helper.py | 5 +-- .../lambda_shared_package/lambda_shared/pr.py | 32 +++++++++++++++-- tests/ci/pr_info.py | 6 ++-- tests/ci/release.py | 5 +-- tests/ci/run_check.py | 30 ++++++++-------- 8 files changed, 81 insertions(+), 62 deletions(-) diff --git a/tests/ci/build_check.py b/tests/ci/build_check.py index c927148986a..260b77b0ee5 100644 --- a/tests/ci/build_check.py +++ b/tests/ci/build_check.py @@ -1,31 +1,27 @@ #!/usr/bin/env python3 import argparse -from pathlib import Path -from typing import Tuple -import subprocess import logging +import subprocess import sys import time +from pathlib import Path +from typing import Tuple -from ci_config import CI_CONFIG, BuildConfig - -from env_helper import ( - REPO_COPY, - S3_BUILDS_BUCKET, - TEMP_PATH, -) -from git_helper import Git -from pr_info import PRInfo -from report import FAILURE, JobReport, StatusType, SUCCESS -from tee_popen import TeePopen import docker_images_helper +from ci_config import CI_CONFIG, BuildConfig +from env_helper import REPO_COPY, S3_BUILDS_BUCKET, TEMP_PATH +from git_helper import Git +from lambda_shared_package.lambda_shared.pr import Labels +from pr_info import PRInfo +from report import FAILURE, SUCCESS, JobReport, StatusType +from stopwatch import Stopwatch +from tee_popen import TeePopen from version_helper import ( ClickHouseVersion, get_version_from_repo, update_version_local, ) -from stopwatch import Stopwatch IMAGE_NAME = "clickhouse/binary-builder" BUILD_LOG_NAME = "build_log.log" @@ -111,6 +107,10 @@ def build_clickhouse( return build_log_path, SUCCESS if success else FAILURE +def is_release_pr(pr_info: PRInfo) -> bool: + return Labels.RELEASE in pr_info.labels or Labels.RELEASE_LTS in pr_info.labels + + def get_release_or_pr(pr_info: PRInfo, version: ClickHouseVersion) -> Tuple[str, str]: "Return prefixes for S3 artifacts paths" # FIXME performance @@ -119,7 +119,7 @@ def get_release_or_pr(pr_info: PRInfo, version: ClickHouseVersion) -> Tuple[str, # It should be fixed in performance-comparison image eventually # For performance tests we always set PRs prefix performance_pr = "PRs/0" - if "release" in pr_info.labels or "release-lts" in pr_info.labels: + if is_release_pr(pr_info): # for release pull requests we use branch names prefixes, not pr numbers return pr_info.head_ref, performance_pr if pr_info.number == 0: @@ -163,7 +163,7 @@ def main(): official_flag = pr_info.number == 0 version_type = "testing" - if "release" in pr_info.labels or "release-lts" in pr_info.labels: + if is_release_pr(pr_info): version_type = "stable" official_flag = True diff --git a/tests/ci/cancel_and_rerun_workflow_lambda/app.py b/tests/ci/cancel_and_rerun_workflow_lambda/app.py index 625936ec5c8..9ee884c801a 100644 --- a/tests/ci/cancel_and_rerun_workflow_lambda/app.py +++ b/tests/ci/cancel_and_rerun_workflow_lambda/app.py @@ -9,7 +9,7 @@ from threading import Thread from typing import Any, Dict, List, Optional import requests -from lambda_shared.pr import check_pr_description +from lambda_shared.pr import Labels, check_pr_description from lambda_shared.token import get_cached_access_token NEED_RERUN_OR_CANCELL_WORKFLOWS = { @@ -261,7 +261,7 @@ def main(event): print("Freshly opened PR, nothing to do") return - if action == "closed" or label == "do not test": + if action == "closed" or label == Labels.DO_NOT_TEST: print("PR merged/closed or manually labeled 'do not test', will kill workflows") workflow_descriptions = get_workflows_description_for_pull_request( pull_request, token @@ -281,7 +281,7 @@ def main(event): exec_workflow_url(urls_to_cancel, token) return - if label == "can be tested": + if label == Labels.CAN_BE_TESTED: print("PR marked with can be tested label, rerun workflow") workflow_descriptions = get_workflows_description_for_pull_request( pull_request, token diff --git a/tests/ci/cherry_pick.py b/tests/ci/cherry_pick.py index d92504e30bd..96906d55899 100644 --- a/tests/ci/cherry_pick.py +++ b/tests/ci/cherry_pick.py @@ -37,19 +37,10 @@ from env_helper import TEMP_PATH from get_robot_token import get_best_robot_token from git_helper import git_runner, is_shallow from github_helper import GitHub, PullRequest, PullRequests, Repository +from lambda_shared_package.lambda_shared.pr import Labels from ssh import SSHKey -class Labels: - MUST_BACKPORT = "pr-must-backport" - MUST_BACKPORT_CLOUD = "pr-must-backport-cloud" - BACKPORT = "pr-backport" - BACKPORTS_CREATED = "pr-backports-created" - BACKPORTS_CREATED_CLOUD = "pr-backports-created-cloud" - CHERRYPICK = "pr-cherrypick" - DO_NOT_TEST = "do not test" - - class ReleaseBranch: CHERRYPICK_DESCRIPTION = """Original pull-request #{pr_number} @@ -99,7 +90,7 @@ close it. name: str, pr: PullRequest, repo: Repository, - backport_created_label: str = Labels.BACKPORTS_CREATED, + backport_created_label: str = Labels.PR_BACKPORTS_CREATED, ): self.name = name self.pr = pr @@ -247,12 +238,12 @@ close it. pr_number=self.pr.number, pr_url=self.pr.html_url, backport_created_label=self.backport_created_label, - label_cherrypick=Labels.CHERRYPICK, + label_cherrypick=Labels.PR_CHERRYPICK, ), base=self.backport_branch, head=self.cherrypick_branch, ) - self.cherrypick_pr.add_to_labels(Labels.CHERRYPICK) + self.cherrypick_pr.add_to_labels(Labels.PR_CHERRYPICK) self.cherrypick_pr.add_to_labels(Labels.DO_NOT_TEST) self._assign_new_pr(self.cherrypick_pr) # update cherrypick PR to get the state for PR.mergable @@ -288,7 +279,7 @@ close it. base=self.name, head=self.backport_branch, ) - self.backport_pr.add_to_labels(Labels.BACKPORT) + self.backport_pr.add_to_labels(Labels.PR_BACKPORT) self._assign_new_pr(self.backport_pr) def ping_cherry_pick_assignees(self, dry_run: bool) -> None: @@ -518,7 +509,7 @@ class Backport: ) bp_cp_prs = self.gh.get_pulls_from_search( query=f"type:pr repo:{self._repo_name} {query_suffix}", - label=f"{Labels.BACKPORT},{Labels.CHERRYPICK}", + label=f"{Labels.PR_BACKPORT},{Labels.PR_CHERRYPICK}", ) for br in branches: br.pop_prs(bp_cp_prs) @@ -588,8 +579,8 @@ def parse_args(): ) parser.add_argument( "--backport-created-label", - default=Labels.BACKPORTS_CREATED, - choices=(Labels.BACKPORTS_CREATED, Labels.BACKPORTS_CREATED_CLOUD), + default=Labels.PR_BACKPORTS_CREATED, + choices=(Labels.PR_BACKPORTS_CREATED, Labels.PR_BACKPORTS_CREATED_CLOUD), help="label to mark PRs as backported", ) parser.add_argument( diff --git a/tests/ci/commit_status_helper.py b/tests/ci/commit_status_helper.py index 98d355742d6..330eb3469c3 100644 --- a/tests/ci/commit_status_helper.py +++ b/tests/ci/commit_status_helper.py @@ -19,7 +19,8 @@ from github.Repository import Repository from ci_config import CHECK_DESCRIPTIONS, REQUIRED_CHECKS, CheckDescription from env_helper import GITHUB_REPOSITORY, GITHUB_RUN_URL, TEMP_PATH -from pr_info import SKIP_MERGEABLE_CHECK_LABEL, PRInfo +from lambda_shared_package.lambda_shared.pr import Labels +from pr_info import PRInfo from report import ( ERROR, FAILURE, @@ -438,7 +439,7 @@ def set_mergeable_check( def update_mergeable_check(commit: Commit, pr_info: PRInfo, check_name: str) -> None: "check if the check_name in REQUIRED_CHECKS and then trigger update" not_run = ( - pr_info.labels.intersection({SKIP_MERGEABLE_CHECK_LABEL, "release"}) + pr_info.labels.intersection({Labels.SKIP_MERGEABLE_CHECK, Labels.RELEASE}) or check_name not in REQUIRED_CHECKS or pr_info.release_pr or pr_info.number == 0 diff --git a/tests/ci/lambda_shared_package/lambda_shared/pr.py b/tests/ci/lambda_shared_package/lambda_shared/pr.py index 4ac787229c0..f80ac896c9b 100644 --- a/tests/ci/lambda_shared_package/lambda_shared/pr.py +++ b/tests/ci/lambda_shared_package/lambda_shared/pr.py @@ -48,11 +48,35 @@ TRUSTED_CONTRIBUTORS = { ] } + +class Labels: + CAN_BE_TESTED = "can be tested" + DO_NOT_TEST = "do not test" + MUST_BACKPORT = "pr-must-backport" + MUST_BACKPORT_CLOUD = "pr-must-backport-cloud" + JEPSEN_TEST = "jepsen-test" + SKIP_MERGEABLE_CHECK = "skip mergeable check" + PR_BACKPORT = "pr-backport" + PR_BACKPORTS_CREATED = "pr-backports-created" + PR_BACKPORTS_CREATED_CLOUD = "pr-backports-created-cloud" + PR_CHERRYPICK = "pr-cherrypick" + PR_CI = "pr-ci" + PR_FEATURE = "pr-feature" + PR_SYNCED_TO_CLOUD = "pr-synced-to-cloud" + PR_SYNC_UPSTREAM = "pr-sync-upstream" + RELEASE = "release" + RELEASE_LTS = "release-lts" + SUBMODULE_CHANGED = "submodule changed" + + # pr-bugfix autoport can lead to issues in releases, let's do ci fixes only + AUTO_BACKPORT = {"pr-ci"} + + # Descriptions are used in .github/PULL_REQUEST_TEMPLATE.md, keep comments there # updated accordingly # The following lists are append only, try to avoid editing them # They still could be cleaned out after the decent time though. -LABELS = { +LABEL_CATEGORIES = { "pr-backward-incompatible": ["Backward Incompatible Change"], "pr-bugfix": [ "Bug Fix", @@ -81,7 +105,9 @@ LABELS = { "pr-ci": ["CI Fix or Improvement (changelog entry is not required)"], } -CATEGORY_TO_LABEL = {c: lb for lb, categories in LABELS.items() for c in categories} +CATEGORY_TO_LABEL = { + c: lb for lb, categories in LABEL_CATEGORIES.items() for c in categories +} def check_pr_description(pr_body: str, repo_name: str) -> Tuple[str, str]: @@ -93,7 +119,7 @@ def check_pr_description(pr_body: str, repo_name: str) -> Tuple[str, str]: # Check if body contains "Reverts ClickHouse/ClickHouse#36337" if [True for line in lines if re.match(rf"\AReverts {repo_name}#[\d]+\Z", line)]: - return "", LABELS["pr-not-for-changelog"][0] + return "", LABEL_CATEGORIES["pr-not-for-changelog"][0] category = "" entry = "" diff --git a/tests/ci/pr_info.py b/tests/ci/pr_info.py index f134487c0a2..afe4e2b87b0 100644 --- a/tests/ci/pr_info.py +++ b/tests/ci/pr_info.py @@ -15,8 +15,8 @@ from env_helper import ( GITHUB_RUN_URL, GITHUB_SERVER_URL, ) +from lambda_shared_package.lambda_shared.pr import Labels -SKIP_MERGEABLE_CHECK_LABEL = "skip mergeable check" NeedsDataType = Dict[str, Dict[str, Union[str, Dict[str, str]]]] DIFF_IN_DOCUMENTATION_EXT = [ @@ -252,7 +252,7 @@ class PRInfo: self.head_ref = pull_request["head"]["ref"] self.head_name = pull_request["head"]["repo"]["full_name"] self.pr_html_url = pull_request["html_url"] - if "pr-backport" in self.labels: + if Labels.PR_BACKPORT in self.labels: # head1...head2 gives changes in head2 since merge base # Thag's why we need {self.head_ref}...master to get # files changed in upstream AND master...{self.head_ref} @@ -275,7 +275,7 @@ class PRInfo: ] else: self.diff_urls.append(self.compare_pr_url(pull_request)) - if "release" in self.labels: + if Labels.RELEASE in self.labels: # For release PRs we must get not only files changed in the PR # itself, but as well files changed since we branched out self.diff_urls.append( diff --git a/tests/ci/release.py b/tests/ci/release.py index b7ccc59f7c1..2775d31285e 100755 --- a/tests/ci/release.py +++ b/tests/ci/release.py @@ -25,6 +25,7 @@ from contextlib import contextmanager from typing import Any, Final, Iterator, List, Optional, Tuple from git_helper import Git, commit, release_branch +from lambda_shared_package.lambda_shared.pr import Labels from report import SUCCESS from version_helper import ( FILE_WITH_VERSION_PATH, @@ -407,9 +408,9 @@ class Release: self._git.update() new_version = self.version.patch_update() version_type = self.get_stable_release_type() - pr_labels = "--label release" + pr_labels = f"--label {Labels.RELEASE}" if version_type == VersionType.LTS: - pr_labels += " --label release-lts" + pr_labels += f" --label {Labels.RELEASE_LTS}" new_version.with_description(version_type) self._update_cmake_contributors(new_version) self._commit_cmake_contributors(new_version) diff --git a/tests/ci/run_check.py b/tests/ci/run_check.py index 204f7b0ece1..e06e8f955d5 100644 --- a/tests/ci/run_check.py +++ b/tests/ci/run_check.py @@ -20,6 +20,7 @@ from get_robot_token import get_best_robot_token from lambda_shared_package.lambda_shared.pr import ( CATEGORY_TO_LABEL, TRUSTED_CONTRIBUTORS, + Labels, check_pr_description, ) from pr_info import PRInfo @@ -29,13 +30,8 @@ TRUSTED_ORG_IDS = { 54801242, # clickhouse } -OK_SKIP_LABELS = {"release", "pr-backport", "pr-cherrypick"} -CAN_BE_TESTED_LABEL = "can be tested" -FEATURE_LABEL = "pr-feature" -SUBMODULE_CHANGED_LABEL = "submodule changed" +OK_SKIP_LABELS = {Labels.RELEASE, Labels.PR_BACKPORT, Labels.PR_CHERRYPICK} PR_CHECK = "PR Check" -# pr-bugfix autoport can lead to issues in releases, let's do ci fixes only -AUTO_BACKPORT_LABELS = ["pr-ci"] def pr_is_by_trusted_user(pr_user_login, pr_user_orgs): @@ -68,11 +64,12 @@ def should_run_ci_for_pr(pr_info: PRInfo) -> Tuple[bool, str]: if OK_SKIP_LABELS.intersection(pr_info.labels): return True, "Don't try new checks for release/backports/cherry-picks" - if CAN_BE_TESTED_LABEL not in pr_info.labels and not pr_is_by_trusted_user( + if Labels.CAN_BE_TESTED not in pr_info.labels and not pr_is_by_trusted_user( pr_info.user_login, pr_info.user_orgs ): print( - f"PRs by untrusted users need the '{CAN_BE_TESTED_LABEL}' label - please contact a member of the core team" + f"PRs by untrusted users need the '{Labels.CAN_BE_TESTED}' label - " + "please contact a member of the core team" ) return False, "Needs 'can be tested' label" @@ -116,11 +113,11 @@ def main(): pr_labels_to_remove.append(label) if pr_info.has_changes_in_submodules(): - pr_labels_to_add.append(SUBMODULE_CHANGED_LABEL) - elif SUBMODULE_CHANGED_LABEL in pr_info.labels: - pr_labels_to_remove.append(SUBMODULE_CHANGED_LABEL) + pr_labels_to_add.append(Labels.SUBMODULE_CHANGED) + elif Labels.SUBMODULE_CHANGED in pr_info.labels: + pr_labels_to_remove.append(Labels.SUBMODULE_CHANGED) - if any(label in AUTO_BACKPORT_LABELS for label in pr_labels_to_add): + if any(label in Labels.AUTO_BACKPORT for label in pr_labels_to_add): backport_labels = [Labels.MUST_BACKPORT, Labels.MUST_BACKPORT_CLOUD] pr_labels_to_add += [ label for label in backport_labels if label not in pr_info.labels @@ -160,16 +157,19 @@ def main(): ) sys.exit(1) - if FEATURE_LABEL in pr_info.labels and not pr_info.has_changes_in_documentation(): + if ( + Labels.PR_FEATURE in pr_info.labels + and not pr_info.has_changes_in_documentation() + ): print( - f"The '{FEATURE_LABEL}' in the labels, " + f"The '{Labels.PR_FEATURE}' in the labels, " "but there's no changed documentation" ) post_commit_status( commit, FAILURE, "", - f"expect adding docs for {FEATURE_LABEL}", + f"expect adding docs for {Labels.PR_FEATURE}", PR_CHECK, pr_info, )