From 5c388f1ed0167f2324b3a81c4ebecd5a7cfe003a Mon Sep 17 00:00:00 2001 From: Nicolas Schweitzer Date: Thu, 23 Jan 2025 19:34:28 +0100 Subject: [PATCH] use the correct bot in workflow, restrict to dependabot and add tests --- .github/workflows/add_dependabot_pr_to_mq.yml | 6 + .../workflows/ask_dependabot_pr_review.yml | 3 +- tasks/issue.py | 13 +- tasks/unit_tests/issue_tests.py | 173 +++++++++++++++++- 4 files changed, 190 insertions(+), 5 deletions(-) diff --git a/.github/workflows/add_dependabot_pr_to_mq.yml b/.github/workflows/add_dependabot_pr_to_mq.yml index db929605af22c7..fa19b5a13c5e4d 100644 --- a/.github/workflows/add_dependabot_pr_to_mq.yml +++ b/.github/workflows/add_dependabot_pr_to_mq.yml @@ -14,9 +14,15 @@ pull-requests: write steps: + - uses: actions/create-github-app-token@c1a285145b9d317df6ced56c09f525b5c2b6f755 # v1.11.1 + id: app-token + with: + app-id: ${{ vars.DD_GITHUB_TOKEN_GENERATOR_APP_ID }} + private-key: ${{ secrets.DD_GITHUB_TOKEN_GENERATOR_PRIVATE_KEY }} - name: Add Merge Comment to Pull Request uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 with: + github-token: ${{ steps.app-token.outputs.token }} script: | const pullRequestNumber = context.payload.pull_request.number; const commentBody = "/merge"; diff --git a/.github/workflows/ask_dependabot_pr_review.yml b/.github/workflows/ask_dependabot_pr_review.yml index a7f5c315f34f43..98fe6420e6b076 100644 --- a/.github/workflows/ask_dependabot_pr_review.yml +++ b/.github/workflows/ask_dependabot_pr_review.yml @@ -16,6 +16,7 @@ permissions: {} jobs: add_reviewers: + if: github.event.pull_request.user.login == 'dependabot[bot]' runs-on: ubuntu-latest permissions: pull-requests: write @@ -35,7 +36,7 @@ - name: Install dependencies run: pip install -r requirements.txt -r tasks/requirements.txt - - name: Check code review complexity + - name: Add reviewers and ask review env: PR_NUMBER: ${{ github.event.pull_request.number }} run: inv -e issue.add-reviewers -p $PR_NUMBER diff --git a/tasks/issue.py b/tasks/issue.py index 7ec96af2dd828f..c4f5a0160f93a5 100644 --- a/tasks/issue.py +++ b/tasks/issue.py @@ -118,16 +118,22 @@ def add_reviewers(ctx, pr_id): print("This is not a (dependabot) bump PR, this action should not be run on it.") return + folder = "" if pr.title.startswith("Bump the "): - match = re.match(r"^Bump the (\S+).*$", pr.title) + match = re.match(r"^Bump the (\S+) group (.*$)", pr.title) + if match.group(2).startswith("in"): + match_folder = re.match(r"^in (\S+).*$", match.group(2)) + folder = match_folder.group(1).removeprefix("/") else: match = re.match(r"^Bump (\S+) from (\S+) to (\S+)$", pr.title) dependency = match.group(1) # Find the responsible person for each file owners = set() - git_files = ctx.run("git ls-files | grep \".go$\"", hide=True).stdout + git_files = ctx.run("git ls-files | grep -e \"^.*.go$\"", hide=True).stdout for file in git_files.splitlines(): + if not file.startswith(folder): + continue in_import = False with open(file) as f: for line in f: @@ -142,5 +148,6 @@ def add_reviewers(ctx, pr_id): if re.search(dependency, line): owners.update(set(search_owners(file, ".github/CODEOWNERS"))) break - pr.create_review_request(team_reviewers=[owner.replace("@DataDog/", "") for owner in owners]) + # Teams are added by slug, so we need to remove the @DataDog/ prefix + pr.create_review_request(team_reviewers=[owner.casefold().removeprefix("@datadog/") for owner in owners]) pr.add_to_labels("ask-review") diff --git a/tasks/unit_tests/issue_tests.py b/tasks/unit_tests/issue_tests.py index e72676fc50e3fb..a55ca5d2712bca 100644 --- a/tasks/unit_tests/issue_tests.py +++ b/tasks/unit_tests/issue_tests.py @@ -1,6 +1,9 @@ import unittest -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch +from invoke.context import MockContext, Result + +from tasks.issue import add_reviewers from tasks.libs.issue.assign import guess_from_keywords, guess_from_labels @@ -34,3 +37,171 @@ def test_with_a_file(self): def test_no_match(self): issue = MagicMock(title="fix bug", body="It comes from the file... hm I don't know.") self.assertEqual(guess_from_keywords(issue), "triage") + + +class TestAddReviewers(unittest.TestCase): + @patch('builtins.print') + @patch('tasks.issue.GithubAPI') + def test_dependabot_only(self, gh_mock, print_mock): + pr_mock = MagicMock() + pr_mock.user.login = "InvisibleMan" + gh_mock.repo.get_pull.return_value = pr_mock + c = MockContext() + add_reviewers(c, 1234) + print_mock.assert_called_once_with("This is not a (dependabot) bump PR, this action should not be run on it.") + + @patch('builtins.print') + @patch('tasks.issue.GithubAPI') + def test_single_dependency_one_reviewer(self, gh_mock, print_mock): + pr_mock = MagicMock() + pr_mock.user.login = "dependabot[bot]" + pr_mock.title = "Bump modernc.org/mathutil from 1.6.0 to 1.7.0" + gh_instance = MagicMock() + gh_instance.repo.get_pull.return_value = pr_mock + gh_mock.return_value = gh_instance + c = MockContext( + run={ + "git ls-files | grep -e \"^.*.go$\"": Result("""pkg/security/proto/ebpfless/msg.go +pkg/security/ptracer/span.go +pkg/security/secl/go.mod +pkg/security/secl/model/model.go +pkg/security/secl/model/model_helpers_unix.go +pkg/security/secl/model/model_unix.go +""") + } + ) + add_reviewers(c, 1234) + print_mock.assert_not_called() + pr_mock.create_review_request.assert_called_once_with(team_reviewers=['agent-security']) + + @patch('builtins.print') + @patch('tasks.issue.GithubAPI') + def test_single_dependency_several_reviewers(self, gh_mock, print_mock): + pr_mock = MagicMock() + pr_mock.user.login = "dependabot[bot]" + pr_mock.title = "Bump github.com/go-delve/delve from 1.6.0 to 1.7.0" + gh_instance = MagicMock() + gh_instance.repo.get_pull.return_value = pr_mock + gh_mock.return_value = gh_instance + c = MockContext( + run={ + "git ls-files | grep -e \"^.*.go$\"": Result("""generate_tools.go +pkg/dynamicinstrumentation/diconfig/dwarf.go +pkg/network/go/asmscan/scan.go +pkg/network/go/bininspect/dwarf.go +pkg/network/go/bininspect/newproc.go +pkg/network/go/bininspect/types.go +pkg/network/go/bininspect/utils.go +pkg/network/go/dwarfutils/compile_unit.go +pkg/network/go/dwarfutils/locexpr/exec.go +pkg/network/go/dwarfutils/type_finder.go +pkg/network/go/goid/goid_offset.go +pkg/network/go/goversion/version.go +pkg/network/go/lutgen/run.go +pkg/network/protocols/http/gotls/lookup/luts.go""") + } + ) + add_reviewers(c, 1234) + print_mock.assert_not_called() + self.assertCountEqual( + pr_mock.create_review_request.call_args[1]['team_reviewers'], + ['universal-service-monitoring', 'debugger', 'agent-devx-infra'], + ) + + @patch('builtins.print') + @patch('tasks.issue.GithubAPI') + def test_group_dependency(self, gh_mock, print_mock): + pr_mock = MagicMock() + pr_mock.user.login = "dependabot[bot]" + pr_mock.title = "Bump the aws-sdk-go-v2 group with 5 updates" + gh_instance = MagicMock() + gh_instance.repo.get_pull.return_value = pr_mock + gh_mock.return_value = gh_instance + c = MockContext( + run={ + "git ls-files | grep -e \"^.*.go$\"": Result("""pkg/databasemonitoring/aws/aurora.go +pkg/databasemonitoring/aws/aurora_test.go +pkg/databasemonitoring/aws/client.go +pkg/databasemonitoring/aws/rdsclient_mockgen.go +pkg/serverless/apikey/api_key.go +pkg/serverless/apikey/api_key_test.go +pkg/serverless/trace/inferredspan/propagation_test.go +pkg/serverless/trace/propagation/carriers_test.go +pkg/serverless/trace/propagation/extractor_test.go +pkg/serverless/trigger/extractor.go +pkg/util/ec2/ec2_tags.go +test/new-e2e/examples/ecs_test.go +test/new-e2e/go.mod +test/new-e2e/pkg/provisioners/aws/kubernetes/kubernetes_dump.go +test/new-e2e/pkg/runner/parameters/store_aws.go +test/new-e2e/pkg/utils/clients/aws.go +test/new-e2e/pkg/utils/e2e/client/ecs/ecs.go +test/new-e2e/pkg/utils/e2e/client/ecs/session-manager-plugin.go +test/new-e2e/tests/containers/ecs_test.go +test/new-e2e/tests/windows/common/pipeline/pipeline.go""") + } + ) + add_reviewers(c, 1234) + print_mock.assert_not_called() + self.assertCountEqual( + pr_mock.create_review_request.call_args[1]['team_reviewers'], + [ + 'windows-agent', + 'database-monitoring', + 'container-integrations', + 'agent-devx-loops', + 'serverless', + 'container-platform', + 'windows-kernel-integrations', + 'agent-shared-components', + 'agent-e2e-testing', + 'serverless-aws', + ], + ) + + @patch('builtins.print') + @patch('tasks.issue.GithubAPI') + def test_group_dependency_scoped(self, gh_mock, print_mock): + pr_mock = MagicMock() + pr_mock.user.login = "dependabot[bot]" + pr_mock.title = "Bump the aws-sdk-go-v2 group in /test/new-e2e with 5 updates" + gh_instance = MagicMock() + gh_instance.repo.get_pull.return_value = pr_mock + gh_mock.return_value = gh_instance + c = MockContext( + run={ + "git ls-files | grep -e \"^.*.go$\"": Result("""pkg/databasemonitoring/aws/aurora.go +pkg/databasemonitoring/aws/aurora_test.go +pkg/databasemonitoring/aws/client.go +pkg/databasemonitoring/aws/rdsclient_mockgen.go +pkg/serverless/apikey/api_key.go +pkg/serverless/apikey/api_key_test.go +pkg/serverless/trace/inferredspan/propagation_test.go +pkg/serverless/trace/propagation/carriers_test.go +pkg/serverless/trace/propagation/extractor_test.go +pkg/serverless/trigger/extractor.go +pkg/util/ec2/ec2_tags.go +test/new-e2e/examples/ecs_test.go +test/new-e2e/go.mod +test/new-e2e/pkg/provisioners/aws/kubernetes/kubernetes_dump.go +test/new-e2e/pkg/runner/parameters/store_aws.go +test/new-e2e/pkg/utils/clients/aws.go +test/new-e2e/pkg/utils/e2e/client/ecs/ecs.go +test/new-e2e/pkg/utils/e2e/client/ecs/session-manager-plugin.go +test/new-e2e/tests/containers/ecs_test.go +test/new-e2e/tests/windows/common/pipeline/pipeline.go""") + } + ) + add_reviewers(c, 1234) + print_mock.assert_not_called() + self.assertCountEqual( + pr_mock.create_review_request.call_args[1]['team_reviewers'], + [ + 'windows-agent', + 'container-integrations', + 'agent-devx-loops', + 'container-platform', + 'windows-kernel-integrations', + 'agent-e2e-testing', + ], + )