From 0ab9d6787e0e0173ad7d13175079c4a368be2211 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Thu, 23 Feb 2023 15:21:19 +0100 Subject: [PATCH 1/4] Add format_description for commit status descriptions --- tests/ci/ast_fuzzer_check.py | 6 ++++-- tests/ci/commit_status_helper.py | 12 ++++++++---- tests/ci/docker_images_check.py | 5 ++--- tests/ci/docker_manifests_merge.py | 5 ++--- tests/ci/docker_server.py | 5 ++--- tests/ci/install_check.py | 9 ++++++--- tests/ci/run_check.py | 5 +++-- tests/ci/sqlancer_check.py | 6 ++++-- 8 files changed, 31 insertions(+), 22 deletions(-) diff --git a/tests/ci/ast_fuzzer_check.py b/tests/ci/ast_fuzzer_check.py index 932964ddcd6..3c2fa05016f 100644 --- a/tests/ci/ast_fuzzer_check.py +++ b/tests/ci/ast_fuzzer_check.py @@ -9,7 +9,7 @@ from github import Github from build_download_helper import get_build_name_for_check, read_build_urls from clickhouse_helper import ClickHouseHelper, prepare_tests_results_for_clickhouse -from commit_status_helper import post_commit_status +from commit_status_helper import format_description, post_commit_status from docker_pull_helper import get_image_with_version from env_helper import ( GITHUB_REPOSITORY, @@ -145,11 +145,13 @@ if __name__ == "__main__": with open( os.path.join(workspace_path, "description.txt"), "r", encoding="utf-8" ) as desc_f: - description = desc_f.readline().rstrip("\n")[:140] + description = desc_f.readline().rstrip("\n") except: status = "failure" description = "Task failed: $?=" + str(retcode) + description = format_description(description) + test_result = TestResult(description, "OK") if "fail" in status: test_result.status = "FAIL" diff --git a/tests/ci/commit_status_helper.py b/tests/ci/commit_status_helper.py index 785250c3904..55aa7f562f3 100644 --- a/tests/ci/commit_status_helper.py +++ b/tests/ci/commit_status_helper.py @@ -111,6 +111,12 @@ def fail_mergeable_check(commit: Commit, description: str) -> None: ) +def format_description(description: str) -> str: + if len(description) > 140: + description = description[:137] + "..." + return description + + def reset_mergeable_check(commit: Commit, description: str = "") -> None: commit.create_status( context="Mergeable Check", @@ -149,12 +155,10 @@ def update_mergeable_check(gh: Github, pr_info: PRInfo, check_name: str) -> None description = "failed: " + ", ".join(fail) if success: description += "; succeeded: " + ", ".join(success) - if len(description) > 140: - description = description[:137] + "..." + description = format_description(description) fail_mergeable_check(commit, description) return description = ", ".join(success) - if len(description) > 140: - description = description[:137] + "..." + description = format_description(description) reset_mergeable_check(commit, description) diff --git a/tests/ci/docker_images_check.py b/tests/ci/docker_images_check.py index d5d1b1a1085..192d216614e 100644 --- a/tests/ci/docker_images_check.py +++ b/tests/ci/docker_images_check.py @@ -14,7 +14,7 @@ from typing import Any, Dict, List, Optional, Set, Tuple, Union from github import Github from clickhouse_helper import ClickHouseHelper, prepare_tests_results_for_clickhouse -from commit_status_helper import post_commit_status +from commit_status_helper import format_description, post_commit_status from env_helper import GITHUB_WORKSPACE, RUNNER_TEMP, GITHUB_RUN_URL from get_robot_token import get_best_robot_token, get_parameter_from_ssm from pr_info import PRInfo @@ -456,8 +456,7 @@ def main(): else: description = "Nothing to update" - if len(description) >= 140: - description = description[:136] + "..." + description = format_description(description) with open(changed_json, "w", encoding="utf-8") as images_file: json.dump(result_images, images_file) diff --git a/tests/ci/docker_manifests_merge.py b/tests/ci/docker_manifests_merge.py index 9a77a91647e..e0917581089 100644 --- a/tests/ci/docker_manifests_merge.py +++ b/tests/ci/docker_manifests_merge.py @@ -10,7 +10,7 @@ from typing import List, Dict, Tuple from github import Github from clickhouse_helper import ClickHouseHelper, prepare_tests_results_for_clickhouse -from commit_status_helper import post_commit_status +from commit_status_helper import format_description, post_commit_status from env_helper import RUNNER_TEMP from get_robot_token import get_best_robot_token, get_parameter_from_ssm from pr_info import PRInfo @@ -218,8 +218,7 @@ def main(): else: description = "Nothing to update" - if len(description) >= 140: - description = description[:136] + "..." + format_description(description) gh = Github(get_best_robot_token(), per_page=100) post_commit_status(gh, pr_info.sha, NAME, description, status, url) diff --git a/tests/ci/docker_server.py b/tests/ci/docker_server.py index fbe934367b4..fa4969a98d5 100644 --- a/tests/ci/docker_server.py +++ b/tests/ci/docker_server.py @@ -15,7 +15,7 @@ from github import Github from build_check import get_release_or_pr from clickhouse_helper import ClickHouseHelper, prepare_tests_results_for_clickhouse -from commit_status_helper import post_commit_status +from commit_status_helper import format_description, post_commit_status from docker_images_check import DockerImage from env_helper import CI, GITHUB_RUN_URL, RUNNER_TEMP, S3_BUILDS_BUCKET, S3_DOWNLOAD from get_robot_token import get_best_robot_token, get_parameter_from_ssm @@ -369,8 +369,7 @@ def main(): description = f"Processed tags: {', '.join(tags)}" - if len(description) >= 140: - description = description[:136] + "..." + format_description(description) gh = Github(get_best_robot_token(), per_page=100) post_commit_status(gh, pr_info.sha, NAME, description, status, url) diff --git a/tests/ci/install_check.py b/tests/ci/install_check.py index 48d0da195ce..b0d0af380bd 100644 --- a/tests/ci/install_check.py +++ b/tests/ci/install_check.py @@ -18,7 +18,11 @@ from clickhouse_helper import ( mark_flaky_tests, prepare_tests_results_for_clickhouse, ) -from commit_status_helper import post_commit_status, update_mergeable_check +from commit_status_helper import ( + format_description, + post_commit_status, + update_mergeable_check, +) from compress_files import compress_fast from docker_pull_helper import get_image_with_version, DockerImage from env_helper import CI, TEMP_PATH as TEMP, REPORTS_PATH @@ -341,8 +345,7 @@ def main(): ch_helper = ClickHouseHelper() mark_flaky_tests(ch_helper, args.check_name, test_results) - if len(description) >= 140: - description = description[:136] + "..." + format_description(description) post_commit_status(gh, pr_info.sha, args.check_name, description, state, report_url) diff --git a/tests/ci/run_check.py b/tests/ci/run_check.py index 7119f443719..e64c2f1d534 100644 --- a/tests/ci/run_check.py +++ b/tests/ci/run_check.py @@ -7,6 +7,7 @@ from typing import Tuple from github import Github from commit_status_helper import ( + format_description, get_commit, post_labels, remove_labels, @@ -157,7 +158,7 @@ def check_pr_description(pr_info: PRInfo) -> Tuple[str, str]: + second_category + "'" ) - return result_status[:140], category + return result_status, category elif re.match( r"(?i)^[#>*_ ]*(short\s*description|change\s*log\s*entry)", lines[i] @@ -249,7 +250,7 @@ if __name__ == "__main__": ) commit.create_status( context=NAME, - description=description_error[:139], + description=format_description(description_error), state="failure", target_url=url, ) diff --git a/tests/ci/sqlancer_check.py b/tests/ci/sqlancer_check.py index 73802740975..d71b9ec3e9c 100644 --- a/tests/ci/sqlancer_check.py +++ b/tests/ci/sqlancer_check.py @@ -10,7 +10,7 @@ from github import Github from build_download_helper import get_build_name_for_check, read_build_urls from clickhouse_helper import ClickHouseHelper, prepare_tests_results_for_clickhouse -from commit_status_helper import post_commit_status +from commit_status_helper import format_description, post_commit_status from docker_pull_helper import get_image_with_version from env_helper import ( GITHUB_REPOSITORY, @@ -171,11 +171,13 @@ def main(): with open( os.path.join(workspace_path, "description.txt"), "r", encoding="utf-8" ) as desc_f: - description = desc_f.readline().rstrip("\n")[:140] + description = desc_f.readline().rstrip("\n") except: # status = "failure" description = "Task failed: $?=" + str(retcode) + format_description(description) + report_url = upload_results( s3_helper, pr_info.number, From 067e656ff3822878b52ac413f4066a96584939cd Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Thu, 23 Feb 2023 15:25:29 +0100 Subject: [PATCH 2/4] Reuse code, do not update Mergeable Check unnecessary --- tests/ci/commit_status_helper.py | 59 +++++++++++++++++--------------- tests/ci/run_check.py | 4 +-- 2 files changed, 33 insertions(+), 30 deletions(-) diff --git a/tests/ci/commit_status_helper.py b/tests/ci/commit_status_helper.py index 55aa7f562f3..44d46162115 100644 --- a/tests/ci/commit_status_helper.py +++ b/tests/ci/commit_status_helper.py @@ -3,7 +3,7 @@ import csv import os import time -from typing import List +from typing import List, Literal import logging from ci_config import CI_CONFIG, REQUIRED_CHECKS @@ -15,6 +15,7 @@ from pr_info import PRInfo, SKIP_MERGEABLE_CHECK_LABEL RETRY = 5 CommitStatuses = List[CommitStatus] +MERGEABLE_NAME = "Mergeable Check" def override_status(status: str, check_name: str, invert: bool = False) -> str: @@ -102,26 +103,21 @@ def post_labels(gh: Github, pr_info: PRInfo, labels_names: List[str]) -> None: pull_request.add_to_labels(label) -def fail_mergeable_check(commit: Commit, description: str) -> None: - commit.create_status( - context="Mergeable Check", - description=description, - state="failure", - target_url=GITHUB_RUN_URL, - ) - - def format_description(description: str) -> str: if len(description) > 140: description = description[:137] + "..." return description -def reset_mergeable_check(commit: Commit, description: str = "") -> None: +def set_mergeable_check( + commit: Commit, + description: str = "", + state: Literal["success", "failure"] = "success", +) -> None: commit.create_status( - context="Mergeable Check", + context=MERGEABLE_NAME, description=description, - state="success", + state=state, target_url=GITHUB_RUN_URL, ) @@ -133,32 +129,39 @@ def update_mergeable_check(gh: Github, pr_info: PRInfo, check_name: str) -> None logging.info("Update Mergeable Check by %s", check_name) commit = get_commit(gh, pr_info.sha) - checks = { - check.context: check.state - for check in filter( - lambda check: (check.context in REQUIRED_CHECKS), - # get_statuses() returns generator, which cannot be reversed - we need comprehension - # pylint: disable=unnecessary-comprehension - reversed([status for status in commit.get_statuses()]), - ) - } + statuses = get_commit_filtered_statuses(commit) + + required_checks = [ + status for status in statuses if status.context in REQUIRED_CHECKS + ] + + mergeable_status = None + for status in statuses: + if status.context == MERGEABLE_NAME: + mergeable_status = status + break success = [] fail = [] - for name, state in checks.items(): - if state == "success": - success.append(name) + for status in required_checks: + if status.state == "success": + success.append(status.context) else: - fail.append(name) + fail.append(status.context) if fail: description = "failed: " + ", ".join(fail) if success: description += "; succeeded: " + ", ".join(success) description = format_description(description) - fail_mergeable_check(commit, description) + if mergeable_status is not None and mergeable_status.description == description: + return + set_mergeable_check(commit, description, "failure") return description = ", ".join(success) description = format_description(description) - reset_mergeable_check(commit, description) + if mergeable_status is not None and mergeable_status.description == description: + return + + set_mergeable_check(commit, description) diff --git a/tests/ci/run_check.py b/tests/ci/run_check.py index e64c2f1d534..e80987e8bc5 100644 --- a/tests/ci/run_check.py +++ b/tests/ci/run_check.py @@ -11,7 +11,7 @@ from commit_status_helper import ( get_commit, post_labels, remove_labels, - reset_mergeable_check, + set_mergeable_check, ) from env_helper import GITHUB_RUN_URL, GITHUB_REPOSITORY, GITHUB_SERVER_URL from get_robot_token import get_best_robot_token @@ -232,7 +232,7 @@ if __name__ == "__main__": if pr_labels_to_remove: remove_labels(gh, pr_info, pr_labels_to_remove) - reset_mergeable_check(commit, "skipped") + set_mergeable_check(commit, "skipped") if description_error: print( From 19bea6a9e8e67f072b0b6247c32be88e6eaad702 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Thu, 23 Feb 2023 15:38:07 +0100 Subject: [PATCH 3/4] Add more cases when we don't need Mergeable Check --- tests/ci/commit_status_helper.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/ci/commit_status_helper.py b/tests/ci/commit_status_helper.py index 44d46162115..2faa0ec6688 100644 --- a/tests/ci/commit_status_helper.py +++ b/tests/ci/commit_status_helper.py @@ -123,7 +123,14 @@ def set_mergeable_check( def update_mergeable_check(gh: Github, pr_info: PRInfo, check_name: str) -> None: - if SKIP_MERGEABLE_CHECK_LABEL in pr_info.labels: + not_run = ( + pr_info.labels.intersection({SKIP_MERGEABLE_CHECK_LABEL, "release"}) + or check_name not in REQUIRED_CHECKS + or pr_info.release_pr + or pr_info.number == 0 + ) + if not_run: + # Let's avoid unnecessary work return logging.info("Update Mergeable Check by %s", check_name) From 900fe7ec9a4a6ae2603c25d5603b7555c359ced4 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Thu, 23 Feb 2023 16:06:29 +0100 Subject: [PATCH 4/4] Reduce returns for update_mergeable_check --- tests/ci/commit_status_helper.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/ci/commit_status_helper.py b/tests/ci/commit_status_helper.py index 2faa0ec6688..28c2742e271 100644 --- a/tests/ci/commit_status_helper.py +++ b/tests/ci/commit_status_helper.py @@ -161,14 +161,11 @@ def update_mergeable_check(gh: Github, pr_info: PRInfo, check_name: str) -> None if success: description += "; succeeded: " + ", ".join(success) description = format_description(description) - if mergeable_status is not None and mergeable_status.description == description: - return - set_mergeable_check(commit, description, "failure") + if mergeable_status is None or mergeable_status.description != description: + set_mergeable_check(commit, description, "failure") return description = ", ".join(success) description = format_description(description) - if mergeable_status is not None and mergeable_status.description == description: - return - - set_mergeable_check(commit, description) + if mergeable_status is None or mergeable_status.description != description: + set_mergeable_check(commit, description)