diff --git a/Tests/scripts/common.py b/Tests/scripts/common.py index 3ba7843f06f7..342f98128e76 100644 --- a/Tests/scripts/common.py +++ b/Tests/scripts/common.py @@ -12,6 +12,7 @@ from Tests.scripts.utils import logging_wrapper as logging from gitlab.v4.objects.pipelines import ProjectPipeline from gitlab.v4.objects.commits import ProjectCommit +from itertools import pairwise CONTENT_NIGHTLY = 'Content Nightly' @@ -257,9 +258,9 @@ def get_pipelines_and_commits(gitlab_client: Gitlab, project_id, return pipelines, commits -def get_person_in_charge(commit): +def get_person_in_charge(commit: ProjectCommit) -> tuple[str, str, str] | tuple[None, None, None]: """ - Returns the name, email, and PR link for the author of the provided commit. + Returns the name of the person in charge of the commit, the PR link and the beginning of the PR name. Args: commit: The Gitlab commit object containing author info. @@ -267,55 +268,63 @@ def get_person_in_charge(commit): Returns: name: The name of the commit author. pr: The GitHub PR link for the Gitlab commit. + beginning_of_pr_name: The beginning of the PR name. """ name = commit.author_name # pr number is always the last id in the commit title, starts with a number sign, may or may not be in parenthesis. pr_number = commit.title.split("#")[-1].strip("()") + beginning_of_pr_name = commit.title[:20] + "..." if pr_number.isnumeric(): pr = f"https://github.com/demisto/content/pull/{pr_number}" - return name, pr + return name, pr, beginning_of_pr_name else: - return None, None + return None, None, None -def are_pipelines_in_order(current_pipeline: ProjectPipeline, previous_pipeline: ProjectPipeline) -> bool: +def are_pipelines_in_order(pipeline_a: ProjectPipeline, pipeline_b: ProjectPipeline) -> bool: """ - This function checks if the current pipeline was created after the previous pipeline, to avoid rare conditions - that pipelines are not in the same order as the commits. + Check if the pipelines are in the same order of their commits. Args: - current_pipeline: The current pipeline object. - previous_pipeline: The previous pipeline object. + pipeline_a: The first pipeline object. + pipeline_b: The second pipeline object. Returns: bool """ - previous_pipeline_timestamp = parser.parse(previous_pipeline.created_at) - current_pipeline_timestamp = parser.parse(current_pipeline.created_at) - return current_pipeline_timestamp > previous_pipeline_timestamp + pipeline_a_timestamp = parser.parse(pipeline_a.created_at) + pipeline_b_timestamp = parser.parse(pipeline_b.created_at) + return pipeline_a_timestamp > pipeline_b_timestamp -def is_pivot(current_pipeline: ProjectPipeline, previous_pipeline: ProjectPipeline) -> bool | None: +def is_pivot(current_pipeline: ProjectPipeline, pipeline_to_compare: ProjectPipeline) -> bool | None: """ Is the current pipeline status a pivot from the previous pipeline status. Args: current_pipeline: The current pipeline object. - previous_pipeline: The previous pipeline object. + pipeline_to_compare: a pipeline object to compare to. Returns: True status changed from success to failed False if the status changed from failed to success None if the status didn't change or the pipelines are not in order of commits """ - in_order = are_pipelines_in_order(current_pipeline, previous_pipeline) + in_order = are_pipelines_in_order(pipeline_a=current_pipeline, pipeline_b=pipeline_to_compare) if in_order: - if previous_pipeline.status == 'success' and current_pipeline.status == 'failed': + if pipeline_to_compare.status == 'success' and current_pipeline.status == 'failed': return True - if previous_pipeline.status == 'failed' and current_pipeline.status == 'success': + if pipeline_to_compare.status == 'failed' and current_pipeline.status == 'success': return False return None def get_reviewer(pr_url: str) -> str | None: + """ + Get the first reviewer who approved the PR. + Args: + pr_url: The URL of the PR. + Returns: + The name of the first reviewer who approved the PR. + """ approved_reviewer = None try: # Extract the owner, repo, and pull request number from the URL @@ -337,6 +346,14 @@ def get_reviewer(pr_url: str) -> str | None: def get_slack_user_name(name: str | None, name_mapping_path: str) -> str: + """ + Get the slack user name for a given Github name. + Args: + name: The name to convert. + name_mapping_path: The path to the name mapping file. + Returns: + The slack user name. + """ with open(name_mapping_path) as map: mapping = json.load(map) # If the name is the name of the 'docker image update bot' reviewer - return the owner of that bot. @@ -347,30 +364,131 @@ def get_slack_user_name(name: str | None, name_mapping_path: str) -> str: def get_commit_by_sha(commit_sha: str, list_of_commits: list[ProjectCommit]) -> ProjectCommit | None: + """ + Get a commit by its SHA. + Args: + commit_sha: The SHA of the commit. + list_of_commits: A list of commits. + Returns: + The commit object. + """ return next((commit for commit in list_of_commits if commit.id == commit_sha), None) def get_pipeline_by_commit(commit: ProjectCommit, list_of_pipelines: list[ProjectPipeline]) -> ProjectPipeline | None: + """ + Get a pipeline by its commit. + Args: + commit: The commit object. + list_of_pipelines: A list of pipelines. + Returns: + The pipeline object. + """ return next((pipeline for pipeline in list_of_pipelines if pipeline.sha == commit.id), None) -def create_shame_message(current_commit: ProjectCommit, - pipeline_changed_status: bool, name_mapping_path: str) -> tuple[str, str, str] | None: +def create_shame_message(suspicious_commits: list[ProjectCommit], + pipeline_changed_status: bool, name_mapping_path: str) -> tuple[str, str, str, str] | None: """ - Create a shame message for the person in charge of the commit. + Create a shame message for the person in charge of the commit, or for multiple people in case of multiple suspicious commits. + Args: + suspicious_commits: A list of suspicious commits. + pipeline_changed_status: A boolean indicating if the pipeline status changed. + name_mapping_path: The path to the name mapping file. + Returns: + A tuple of strings containing the message, the person in charge, the PR link and the color of the message. """ - name, pr = get_person_in_charge(current_commit) - if name and pr: - if name == CONTENT_BOT: - name = get_reviewer(pr) - name = get_slack_user_name(name, name_mapping_path) - msg = "broke" if pipeline_changed_status else "fixed" - color = "danger" if pipeline_changed_status else "good" - emoji = ":cry:" if pipeline_changed_status else ":muscle:" - return (f"Hi @{name}, You {msg} the build! {emoji} ", - f" That was done in this {slack_link(pr,'PR.')}", color) - return None + hi_and_status = person_in_charge = in_this_pr = color = "" + for suspicious_commit in suspicious_commits: + name, pr, beginning_of_pr = get_person_in_charge(suspicious_commit) + if name and pr and beginning_of_pr: + if name == CONTENT_BOT: + name = get_reviewer(pr) + name = get_slack_user_name(name, name_mapping_path) + msg = "broken" if pipeline_changed_status else "fixed" + color = "danger" if pipeline_changed_status else "good" + emoji = ":cry:" if pipeline_changed_status else ":muscle:" + if suspicious_commits.index(suspicious_commit) == 0: + hi_and_status = f"Hi, The build was {msg} {emoji} by:" + person_in_charge = f"@{name}" + in_this_pr = f" That was done in this PR: {slack_link(pr, beginning_of_pr)}" + + else: + person_in_charge += f" or @{name}" + in_this_pr = "" + + return (hi_and_status, person_in_charge, in_this_pr, color) if hi_and_status and person_in_charge and color else None def slack_link(url: str, text: str) -> str: + """ + Create a slack link. + Args: + url: The URL to link to. + text: The text to display. + Returns: + The slack link. + """ return f"<{url}|{text}>" + + +def was_message_already_sent(commit_index: int, list_of_commits: list, list_of_pipelines: list) -> bool: + """ + Check if a message was already sent for newer commits, this is possible if pipelines of later commits, + finished before the pipeline of the current commit. + Args: + commit_index: The index of the current commit. + list_of_commits: A list of commits. + list_of_pipelines: A list of pipelines. + Returns: + + """ + for previous_commit, current_commit in pairwise(reversed(list_of_commits[:commit_index])): + current_pipeline = get_pipeline_by_commit(current_commit, list_of_pipelines) + previous_pipeline = get_pipeline_by_commit(previous_commit, list_of_pipelines) + # in rare cases some commits have no pipeline + if current_pipeline and previous_pipeline and (is_pivot(current_pipeline, previous_pipeline) is not None): + return True + return False + + +def get_nearest_newer_commit_with_pipeline(list_of_pipelines: list[ProjectPipeline], list_of_commits: list[ProjectCommit], + current_commit_index: int) -> tuple[ProjectPipeline, list] | tuple[None, None]: + """ + Get the nearest newer commit that has a pipeline. + Args: + list_of_pipelines: A list of pipelines. + list_of_commits: A list of commits. + current_commit_index: The index of the current commit. + Returns: + A tuple of the nearest pipeline and a list of suspicious commits that have no pipelines. + """ + suspicious_commits = [] + for index in reversed(range(0, current_commit_index - 1)): + next_commit = list_of_commits[index] + suspicious_commits.append(list_of_commits[index + 1]) + next_pipeline = get_pipeline_by_commit(next_commit, list_of_pipelines) + if next_pipeline: + return next_pipeline, suspicious_commits + return None, None + + +def get_nearest_older_commit_with_pipeline(list_of_pipelines: list[ProjectPipeline], list_of_commits: list[ProjectCommit], + current_commit_index: int) -> tuple[ProjectPipeline, list] | tuple[None, None]: + """ + Get the nearest oldest commit that has a pipeline. + Args: + list_of_pipelines: A list of pipelines. + list_of_commits: A list of commits. + current_commit_index: The index of the current commit. + Returns: + A tuple of the nearest pipeline and a list of suspicious commits that have no pipelines. + """ + suspicious_commits = [] + for index in range(current_commit_index, len(list_of_commits) - 1): + previous_commit = list_of_commits[index + 1] + suspicious_commits.append(list_of_commits[index]) + previous_pipeline = get_pipeline_by_commit(previous_commit, list_of_pipelines) + if previous_pipeline: + return previous_pipeline, suspicious_commits + return None, None diff --git a/Tests/scripts/gitlab_slack_notifier.py b/Tests/scripts/gitlab_slack_notifier.py index 4283abdf7411..5850a85a202b 100644 --- a/Tests/scripts/gitlab_slack_notifier.py +++ b/Tests/scripts/gitlab_slack_notifier.py @@ -25,7 +25,8 @@ replace_escape_characters from Tests.scripts.github_client import GithubPullRequest from Tests.scripts.common import get_pipelines_and_commits, is_pivot, get_commit_by_sha, get_pipeline_by_commit, \ - create_shame_message, slack_link + create_shame_message, slack_link, was_message_already_sent, get_nearest_newer_commit_with_pipeline, \ + get_nearest_older_commit_with_pipeline from Tests.scripts.test_modeling_rule_report import calculate_test_modeling_rule_results, \ read_test_modeling_rule_to_jira_mapping, get_summary_for_test_modeling_rule, TEST_MODELING_RULES_TO_JIRA_TICKETS_CONVERTED from Tests.scripts.test_playbooks_report import read_test_playbook_to_jira_mapping, TEST_PLAYBOOKS_TO_JIRA_TICKETS_CONVERTED @@ -360,7 +361,7 @@ def construct_slack_msg(triggering_workflow: str, pipeline_url: str, pipeline_failed_jobs: list[ProjectPipelineJob], pull_request: GithubPullRequest | None, - shame_message: tuple[str, str, str] | None) -> tuple[list[dict[str, Any]], list[dict[str, Any]]]: + shame_message: tuple[str, str, str, str] | None) -> tuple[list[dict[str, Any]], list[dict[str, Any]]]: # report failing jobs content_fields = [] @@ -441,9 +442,9 @@ def construct_slack_msg(triggering_workflow: str, title += title_append slack_msg_start = [] if shame_message: - shame_title, shame_value, shame_color = shame_message + hi_and_status, person_in_charge, in_this_pr, shame_color = shame_message slack_msg_start.append({ - "title": f"{shame_title}\n{shame_value}", + "title": f"{hi_and_status}\n{person_in_charge}\n{in_this_pr}", "color": shame_color }) return slack_msg_start + [{ @@ -562,31 +563,52 @@ def main(): pipeline_url, pipeline_failed_jobs = collect_pipeline_data(gitlab_client, project_id, pipeline_id) shame_message = None if options.current_branch == DEFAULT_BRANCH and triggering_workflow == CONTENT_MERGE: - # We check if the previous build failed and this one passed, or wise versa. + computed_slack_channel = "dmst-build-test" + # Check if the current commit's pipeline differs from the previous one. If the previous pipeline is still running, + # compare the next build. For commits without pipelines, compare the current one to the nearest commit with a + # pipeline and all those in between, marking them as suspicious. list_of_pipelines, list_of_commits = get_pipelines_and_commits(gitlab_client=gitlab_client, project_id=project_id, look_back_hours=LOOK_BACK_HOURS) current_commit = get_commit_by_sha(commit_sha, list_of_commits) if current_commit: current_commit_index = list_of_commits.index(current_commit) + # If the current commit is the last commit in the list, there is no previous commit, # since commits are in ascending order - if current_commit_index != len(list_of_commits) - 1: - previous_commit = list_of_commits[current_commit_index + 1] + # or if we already sent a shame message for newer commits, we don't want to send another one for older commits. + if (current_commit_index != len(list_of_commits) - 1 + and not was_message_already_sent(current_commit_index, list_of_commits, list_of_pipelines)): current_pipeline = get_pipeline_by_commit(current_commit, list_of_pipelines) - previous_pipeline = get_pipeline_by_commit(previous_commit, list_of_pipelines) - if current_pipeline and previous_pipeline: - pipeline_changed_status = is_pivot(current_pipeline, previous_pipeline) + + # looking backwards until we find a commit with a pipeline to compare with + previous_pipeline, suspicious_commits = get_nearest_older_commit_with_pipeline( + list_of_pipelines, list_of_commits, current_commit_index) + if previous_pipeline and suspicious_commits and current_pipeline: + pipeline_changed_status = is_pivot(current_pipeline=current_pipeline, + pipeline_to_compare=previous_pipeline) + logging.info( - f"Checking pipeline {current_pipeline}, the commit is {current_commit} " - f"and the pipeline change status is: {pipeline_changed_status}" - ) + f"Checking pipeline id: {current_pipeline.id}, of commit: {current_commit.title}, " + f"after comparing with pipeline id: {previous_pipeline.id}," + f"the change status is: {pipeline_changed_status}") + + if pipeline_changed_status is None and current_commit_index > 0: + # looking_forward until we find a commit with a pipeline to compare with + next_pipeline, suspicious_commits = get_nearest_newer_commit_with_pipeline( + list_of_pipelines, list_of_commits, current_commit_index) + + if next_pipeline and suspicious_commits: + pipeline_changed_status = is_pivot(current_pipeline=next_pipeline, + pipeline_to_compare=current_pipeline) + logging.info( + f" after comparing with pipeline id: {next_pipeline.id}," + f"the change status is: {pipeline_changed_status}") + if pipeline_changed_status is not None: - shame_message = create_shame_message( - current_commit, pipeline_changed_status, options.name_mapping_path - ) + shame_message = create_shame_message(suspicious_commits, pipeline_changed_status, # type: ignore + options.name_mapping_path) computed_slack_channel = "test_slack_notifier_when_master_is_broken" - else: - computed_slack_channel = "dmst-build-test" + slack_msg_data, threaded_messages = construct_slack_msg(triggering_workflow, pipeline_url, pipeline_failed_jobs, pull_request, shame_message) diff --git a/Tests/scripts/infrastructure_tests/common_test.py b/Tests/scripts/infrastructure_tests/common_test.py index 47f71e6a2e3b..de51808ddab9 100644 --- a/Tests/scripts/infrastructure_tests/common_test.py +++ b/Tests/scripts/infrastructure_tests/common_test.py @@ -1,11 +1,9 @@ from pathlib import Path -from Tests.scripts.common import get_reviewer, get_person_in_charge, are_pipelines_in_order, is_pivot, get_slack_user_name +from Tests.scripts.common import get_reviewer, get_person_in_charge, are_pipelines_in_order, is_pivot, get_slack_user_name, \ + was_message_already_sent, get_nearest_newer_commit_with_pipeline, get_nearest_older_commit_with_pipeline from requests_mock import MockerCore -NAME_AND_PR_URL = ('John Doe', 'https://github.com/demisto/content/pull/123') - - def test_get_person_in_charge(mocker): """ Given: @@ -13,14 +11,14 @@ def test_get_person_in_charge(mocker): When: The function get_person_in_charge is called with that commit Then: - It should return a tuple with the author name and the pull request URL + It should return a tuple with the author name and the pull request URL and the title beginning (up to 20 characters) """ commit = mocker.Mock() commit.author_name = 'John Doe' commit.title = 'Fix a bug (#123)' result = get_person_in_charge(commit) - assert result == NAME_AND_PR_URL + assert result == ('John Doe', 'https://github.com/demisto/content/pull/123', 'Fix a bug (#123)...') def test_get_person_in_charge__multiple_IDs(mocker): @@ -30,14 +28,15 @@ def test_get_person_in_charge__multiple_IDs(mocker): When: The function get_person_in_charge is called with that commit Then: - It should return the a tuple with the author name and the pull request URL, with only the last ID in the URL + It should return the a tuple with the author name and the pull request URL, with only the last ID in the URL, + and the title beginning (up to 20 characters) """ commit = mocker.Mock() commit.author_name = 'John Doe' commit.title = 'Fix a bug (#456) (#123)' result = get_person_in_charge(commit) - assert result == NAME_AND_PR_URL + assert result == ('John Doe', 'https://github.com/demisto/content/pull/123', 'Fix a bug (#456) (#1...') def test_get_person_in_charge__no_parenthesis(mocker): @@ -47,14 +46,15 @@ def test_get_person_in_charge__no_parenthesis(mocker): When: The function get_person_in_charge is called with the commit Then: - It should return the author name and the pull request URL even if the ID was not in parenthesis + It should return the author name and the pull request URL (even if the ID was not in parenthesis) + and the title beginning (up to 20 characters) """ commit = mocker.Mock() commit.author_name = 'John Doe' commit.title = 'Fix a bug #123' result = get_person_in_charge(commit) - assert result == NAME_AND_PR_URL + assert result == ('John Doe', 'https://github.com/demisto/content/pull/123', 'Fix a bug #123...') def test_get_person_in_charge__no_number_sign(mocker): @@ -71,7 +71,7 @@ def test_get_person_in_charge__no_number_sign(mocker): commit.title = 'Fix a bug (123)' result = get_person_in_charge(commit) - assert result == (None, None) + assert result == (None, None, None) def test_pipelines_are_in_correct_order__false(mocker): @@ -302,3 +302,133 @@ def test_get_slack_user_name__name_is_github_actions_bot(): name = "github-actions[bot]" result = get_slack_user_name(name, str(Path(__file__).parent / 'tests_data/test_mapping.json')) assert result == "docker images bot owner" + + +COMMITS = ['commit1', 'commit2', 'commit3', 'commit4', 'commit5'] +PIPELINES = ['pipeline1', 'pipeline2', 'pipeline3', 'pipeline4', 'pipeline5'] + + +def test_was_message_already_sent__was_sent_for_true_pivot(mocker): + """ + Given: + An index of a commit and a list of commits and pipelines with a positive pivot in newer pipelines + When: + The function was_message_already_sent is called with the index, commits and pipelines + Then: + It should return True since the message was already sent for newer pipelines + """ + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', side_effect=lambda commit, pipelines: commit) + mocker.patch('Tests.scripts.common.is_pivot', return_value=True) + + assert was_message_already_sent(2, COMMITS, PIPELINES) is True + + +def test_was_message_already_sent__was_sent_for_false_pivot(mocker): + """ + Given: + An index of a commit and a list of commits and pipelines with a negative pivot in newer pipelines + When: + The function was_message_already_sent is called with the index, commits and pipelines + Then: + It should return True since the message was already sent for newer pipelines + """ + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', side_effect=lambda commit, pipelines: commit) + mocker.patch('Tests.scripts.common.is_pivot', return_value=False) + assert was_message_already_sent(2, COMMITS, PIPELINES) is True + + +def test_was_message_already_sent__was_not_sent(mocker): + """ + Given: + An index of a commit and a list of commits and pipelines with a no pivots in newer pipelines + When: + The function was_message_already_sent is called with the index, commits and pipelines + Then: + It should return False since the message was not sent for newer pipelines + """ + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', side_effect=lambda commit, pipelines: commit) + mocker.patch('Tests.scripts.common.is_pivot', return_value=None) + assert was_message_already_sent(2, COMMITS, PIPELINES) is False + + +def test_was_message_already_sent__was_not_sent_no_pipeline(mocker): + """ + Given: + An index of a commit that has no pipeline and a list of commits and pipelines with a positive pivot in newer pipelines + When: + The function was_message_already_sent is called with the index, commits and pipelines + Then: + It should return False since the message was not sent for newer pipelines since current commit has no pipeline + """ + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', side_effect=lambda commit, pipelines: commit) + mocker.patch('Tests.scripts.common.is_pivot', return_value=True) + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', side_effect=lambda commit, + pipelines: None if commit == 'commit2' else commit) + assert was_message_already_sent(2, COMMITS, PIPELINES) is False + + +def test_get_nearest_newer_commit__with_pipeline(mocker): + """ + Given: + A list of commits and pipelines, but only the first commit has a pipeline + When: + The function get_nearest_commit_with_pipeline is called with the list of commits, + the index of current commit and "newer" as the direction + Then: + It should return the first commit since he is the closest with a pipeline, + and a list of all commits between the first commit and the current one that are suspicious + """ + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', side_effect=lambda commit, + pipelines: commit if commit == 'commit1' else None) + pipeline, suspicious_commits = get_nearest_newer_commit_with_pipeline(PIPELINES, COMMITS, 3) + assert pipeline == 'commit1' + assert suspicious_commits == ['commit3', 'commit2'] + + +def test_get_nearest_older_commit__with_pipeline(mocker): + """ + Given: + A list of commits and pipelines, but only the last commit has a pipeline + When: + The function get_nearest_older_commit_with_pipeline is called with the list of commits, + Then: + It should return the last commit since he is the closest with a pipeline, + and a list of all commits between the last commit and the current one that are suspicious + """ + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', side_effect=lambda commit, + pipelines: commit if commit == 'commit5' else None) + pipeline, suspicious_commits = get_nearest_older_commit_with_pipeline(PIPELINES, COMMITS, 1) + assert pipeline == 'commit5' + assert suspicious_commits == ['commit2', 'commit3', 'commit4'] + + +def test_get_nearest_newer_commit_with_pipeline__no_pipelines(mocker): + """ + Given: + A list of commits and pipelines, but no commit has a pipeline + When: + The function get_nearest_newer_commit_with_pipeline is called with the list of commits, + Then: + It should return None since no commit has a pipeline. + """ + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', return_value='pipeline_for_commit') + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', return_value=None) + pipeline, suspicious_commits = get_nearest_newer_commit_with_pipeline(PIPELINES, COMMITS, 2) + assert pipeline is None + assert suspicious_commits is None + + +def test_get_nearest_older_commit_with_pipeline__no_pipelines(mocker): + """ + Given: + A list of commits and pipelines, but no commit has a pipeline + When: + The function get_nearest_older_commit_with_pipeline is called with the list of commits, + Then: + It should return None since no commit has a pipeline. + """ + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', return_value='pipeline_for_commit') + mocker.patch('Tests.scripts.common.get_pipeline_by_commit', return_value=None) + pipeline, suspicious_commits = get_nearest_older_commit_with_pipeline(PIPELINES, COMMITS, 2) + assert pipeline is None + assert suspicious_commits is None