From 7b2710d00a4c0eab0724318abf543cbf3f15c9e3 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 24 Apr 2024 22:27:10 +0530 Subject: [PATCH 1/2] feat: Update the community PR message This change updates the community PR message to provide better guidance and updated links to the latest documentation. The message now also directs contributors to the correct team or person to tag to get their PR reviewed and merged. --- openedx_webhooks/bot_comments.py | 39 ++++- openedx_webhooks/info.py | 7 +- .../github_community_pr_comment.md.j2 | 134 ++++++++++++------ tests/test_pull_request_opened.py | 13 +- 4 files changed, 138 insertions(+), 55 deletions(-) diff --git a/openedx_webhooks/bot_comments.py b/openedx_webhooks/bot_comments.py index 96449e7d..fef44800 100644 --- a/openedx_webhooks/bot_comments.py +++ b/openedx_webhooks/bot_comments.py @@ -10,15 +10,22 @@ from typing import Dict import arrow +import yaml from flask import render_template from openedx_webhooks.info import ( get_jira_server_info, is_draft_pull_request, - pull_request_has_cla, + pull_request_has_cla, read_github_file, ) from openedx_webhooks.types import JiraId, PrDict +from openedx_webhooks.utils import memoize_timed +# Author association values for which we should consider the author new +GITHUB_NEW_AUTHOR_ASSOCIATIONS = ( + "FIRST_TIMER", # Author has not previously committed to GitHub. + "FIRST_TIME_CONTRIBUTOR", # Author has not previously committed to the repository. +) class BotComment(Enum): """ @@ -79,6 +86,7 @@ class BotComment(Enum): BotComment.NO_CONTRIBUTIONS, } + def is_comment_kind(kind: BotComment, text: str) -> bool: """ Is this `text` a comment of this `kind`? @@ -86,6 +94,30 @@ def is_comment_kind(kind: BotComment, text: str) -> bool: return any(snip in text for snip in BOT_COMMENT_INDICATORS[kind]) +@memoize_timed(minutes=30) +def get_repo_catalog(repo_full_name: str) -> dict: + """ + Get and load the catalog-info.yaml file for the repo. + """ + return yaml.safe_load(read_github_file(repo_full_name, "catalog-info.yaml", "")) + + +def _get_repo_owner(repo_full_name: str) -> str | None: + """ + Get the owner of the repo from its catalog-info.yaml file. + """ + repo_catalog = get_repo_catalog(repo_full_name) + if not repo_catalog: + return None + owner = repo_catalog["spec"]["owner"] + owner_type = "group" + if ":" in owner: + owner_type, owner = owner.split(":") + if owner_type == "group": + owner = f"openedx/{owner}" + return owner + + def github_community_pr_comment(pull_request: PrDict) -> str: """ For a newly-created pull request from an open source contributor, @@ -95,12 +127,17 @@ def github_community_pr_comment(pull_request: PrDict) -> str: * check for contributor agreement * contain a link to our process documentation """ + is_first_time = pull_request.get("author_association", None) in GITHUB_NEW_AUTHOR_ASSOCIATIONS + owner = _get_repo_owner(pull_request["base"]["repo"]["full_name"]) + return render_template( "github_community_pr_comment.md.j2", user=pull_request["user"]["login"], has_signed_agreement=pull_request_has_cla(pull_request), is_draft=is_draft_pull_request(pull_request), is_merged=pull_request.get("merged", False), + is_first_time=is_first_time, + owner=owner, ) diff --git a/openedx_webhooks/info.py b/openedx_webhooks/info.py index f6a5e431..35995009 100644 --- a/openedx_webhooks/info.py +++ b/openedx_webhooks/info.py @@ -47,10 +47,10 @@ def _read_data_file(filename): """ Read the text of an openedx-webhooks-data file. """ - return _read_github_file("openedx/openedx-webhooks-data", filename) + return read_github_file("openedx/openedx-webhooks-data", filename) -def _read_github_file(repo_fullname: str, file_path: str, not_there: Optional[str] = None) -> str: +def read_github_file(repo_fullname: str, file_path: str, not_there: Optional[str] = None) -> str: """ Read a GitHub file from the main or master branch of a repo. @@ -67,6 +67,7 @@ def _read_github_file(repo_fullname: str, file_path: str, not_there: Optional[st """ return _read_github_url(_github_file_url(repo_fullname, file_path), not_there) + def _read_github_url(url: str, not_there: Optional[str] = None) -> str: """ Read the content of a GitHub URL. @@ -302,7 +303,7 @@ def get_bot_comments(prid: PrId) -> Iterable[PrCommentDict]: def get_catalog_info(repo_fullname: str) -> Dict: """Get the parsed catalog-info.yaml data from a repo, or {} if missing.""" - yml = _read_github_file(repo_fullname, "catalog-info.yaml", not_there="{}") + yml = read_github_file(repo_fullname, "catalog-info.yaml", not_there="{}") return yaml.safe_load(yml) diff --git a/openedx_webhooks/templates/github_community_pr_comment.md.j2 b/openedx_webhooks/templates/github_community_pr_comment.md.j2 index 366fa089..c0b92b1a 100644 --- a/openedx_webhooks/templates/github_community_pr_comment.md.j2 +++ b/openedx_webhooks/templates/github_community_pr_comment.md.j2 @@ -1,52 +1,100 @@ -{% filter replace("\n", " ")|trim %} -Thanks for the pull request, @{{ user }}! -Please note that it may take us up to several weeks or months to complete a review and merge your PR. -{% endfilter %} - -Feel free to add as much of the following information to the ticket as you can: -- supporting documentation -- [Open edX discussion forum threads](https://discuss.openedx.org/) -- timeline information ("this must be merged by XX date", and why that is) -- partner information ("this is a course on edx.org") -- any other information that can help Product understand the context for the PR - -{% filter replace("\n", " ")|trim %} -All technical communication about the code itself will be done via the -GitHub pull request interface. As a reminder, -[our process documentation is here](http://edx-developer-guide.readthedocs.org/en/latest/process/overview.html). -{% endfilter %} +Thanks for the pull request, `@{{ user }}`! -{% if is_draft %} -{% filter replace("\n", " ")|trim %} -This is currently a draft pull request. When it is ready for our review and all tests are green, -click "Ready for Review", or remove "WIP" from the title, as appropriate. -{% endfilter %} +### What's next? - -{% else %} -Please let us know once your PR is ready for our review and all tests are green. -{% endif %} +*Please work through the following steps to get your changes ready for engineering review:* + +#### :radio_button: Get product approval + +If you haven't already, [check this list](https://openedx.atlassian.net/wiki/spaces/COMM/pages/3875962884/How+to+submit+an+open+source+contribution+for+Product+Review#Does-my-contribution-require-Product-Review%3F) to see if your contribution needs to go through the product review process. + +- If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the [Product Working Group](https://openedx.atlassian.net/wiki/spaces/COMM/pages/3449028609/Product+Working+Group). + - This process (including the steps you'll need to take) is documented [here](https://openedx.atlassian.net/wiki/spaces/COMM/pages/3875962884/How+to+submit+an+open+source+contribution+for+Product+Review#Product-Review-Process). +- If it doesn't, simply proceed with the next step. + +#### :radio_button: Provide context + +To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can: + +- Dependencies + > This PR must be merged before / after / at the same time as ... +- Blockers + > This PR is waiting for OEP-1234 to be accepted. +- Timeline information + > This PR must be merged by XX date **because** ... +- Partner information + > This is for a course on edx.org. +- Supporting documentation +- Relevant [Open edX discussion forum](https://discuss.openedx.org/) threads {% if not has_signed_agreement %} - {%- filter replace("\n", " ")|trim %} - :warning: We can't start reviewing your pull request until you've submitted a - [signed contributor agreement](https://openedx.org/cla) - or indicated your institutional affiliation. Please see the [CONTRIBUTING](https://github.com/openedx/.github/blob/master/CONTRIBUTING.md) - file for more information. - If you've signed an agreement in the past, you may need to re-sign. See - [The New Home of the Open edX Codebase](https://open.edx.org/blog/the-new-home-of-the-open-edx-codebase/) - for details. - {%- endfilter %} - - {{ '\n' }} - - {%- filter replace("\n", " ")|trim %} - Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the - CLA check by adding a comment here that you have signed it. If the problem persists, you can tag the `@openedx/cla-problems` team in a - comment on your PR for further assistance. - {%- endfilter %} +#### :radio_button: Submit a signed contributor agreement (CLA) + +:warning: We ask all contributors to the Open edX project to submit a [signed contributor agreement](https://openedx.org/cla) or indicate their institutional affiliation. +Please see the [CONTRIBUTING](https://github.com/openedx/.github/blob/master/CONTRIBUTING.md) file for more information. + +If you've signed an agreement in the past, you may need to re-sign. +See [The New Home of the Open edX Codebase](https://open.edx.org/blog/the-new-home-of-the-open-edx-codebase/) for details. + +Once you've signed the CLA, please allow 1 business day for it to be processed. +After this time, you can re-run the CLA check by adding a comment below that you have signed it. +If the CLA check continues to fail, you can tag the `@openedx/cla-problems` team in a comment for further assistance. {% endif %} +{% if is_first_time %} +#### :radio_button: Wait for tests to be enabled + +It looks like you are contributing to this repository for the first time. +This means that automated tests won't run automatically, and that you'll need to wait for a test run to be authorized. + +If you're experiencing a delay of two weeks or more at this step, tag the [community contributions project managers](https://openedx.atlassian.net/wiki/spaces/COMM/pages/3548807177/Community+Contributions+Project+Manager#Current-OSPR-Project-Managers) in a comment and ask for help. +{% endif %} + +#### :radio_button: Get a green build + +If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. + +{% if is_draft %} +#### :radio_button: Update the status of your PR + +Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. + + +{% endif %} + +#### :radio_button: Let us know that your PR is ready for review: + +### Who will review my changes? + +{% if owner %} +This repository is currently maintained by `@{{ owner }}`. Tag them in a comment and let them know that your changes are ready for review. +{% else %} +This repository is currently unmaintained. To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review: + +1. On the right-hand side of the PR, find the Contributions project, click the caret in the top right corner to expand it, and check the "Primary PM" field for the name of your PM. +2. Find their GitHub handle [here](https://openedx.atlassian.net/wiki/spaces/COMM/pages/3548807177/Community+Contributions+Project+Manager#Current-OSPR-Project-Managers). +{% endif %} + +### Where can I find more information? + +If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: + +- [Overview of Review Process for Community Contributions](https://docs.openedx.org/en/latest/developers/references/developer_guide/process/FAQ-about-pull-requests.html) +- [Pull Request Status Guide](https://docs.openedx.org/en/latest/developers/references/developer_guide/process/pull-request-statuses.html) +- [Making changes to your pull request](https://docs.openedx.org/en/latest/documentors/how-tos/make_changes_to_your_pull_request.html) + +### When can I expect my changes to be merged? + +Our goal is to get community contributions seen and reviewed as efficiently as possible. + +However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as: + +- The size and impact of the changes that it introduces +- The need for product review +- Maintenance status of the parent repository + +:bulb: *As a result it may take up to several weeks or months to complete a review and merge your PR.* + diff --git a/tests/test_pull_request_opened.py b/tests/test_pull_request_opened.py index 542c62f7..c20c98f0 100644 --- a/tests/test_pull_request_opened.py +++ b/tests/test_pull_request_opened.py @@ -99,7 +99,7 @@ def test_external_pr_opened_no_cla(fake_github): assert len(pr_comments) == 1 body = pr_comments[0].body check_issue_link_in_markdown(body, None) - assert "Thanks for the pull request, @new_contributor!" in body + assert "Thanks for the pull request, `@new_contributor`!" in body assert is_comment_kind(BotComment.NEED_CLA, body) assert is_comment_kind(BotComment.WELCOME, body) @@ -132,7 +132,7 @@ def test_external_pr_opened_with_cla(fake_github): assert len(pr_comments) == 1 body = pr_comments[0].body check_issue_link_in_markdown(body, None) - assert "Thanks for the pull request, @tusbar!" in body + assert "Thanks for the pull request, `@tusbar`!" in body assert is_comment_kind(BotComment.WELCOME, body) assert not is_comment_kind(BotComment.NEED_CLA, body) @@ -235,8 +235,7 @@ def test_draft_pr_opened(pr_type, fake_github, mocker): pr_comments = pr.list_comments() assert len(pr_comments) == 1 body = pr_comments[0].body - assert 'This is currently a draft pull request' in body - assert 'click "Ready for Review"' in body + assert is_comment_kind(BotComment.END_OF_WIP, body) expected_labels = set() expected_labels.add("blended" if pr_type == "blended" else "open-source-contribution") assert pr.labels == expected_labels @@ -263,8 +262,7 @@ def test_draft_pr_opened(pr_type, fake_github, mocker): pr_comments = pr.list_comments() assert len(pr_comments) == 1 body = pr_comments[0].body - assert 'This is currently a draft pull request' not in body - assert 'click "Ready for Review"' not in body + assert not is_comment_kind(BotComment.END_OF_WIP, body) # Oops, it goes back to draft! pr.title = title1 @@ -274,8 +272,7 @@ def test_draft_pr_opened(pr_type, fake_github, mocker): pr_comments = pr.list_comments() assert len(pr_comments) == 1 body = pr_comments[0].body - assert 'This is currently a draft pull request' in body - assert 'click "Ready for Review"' in body + assert is_comment_kind(BotComment.END_OF_WIP, body) def test_dont_add_internal_prs_to_project(fake_github): From 6121962df54d3f7448335e6b0ccc35a997c24d01 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Thu, 2 May 2024 17:21:19 +0530 Subject: [PATCH 2/2] fixup!: Update message, add tests --- openedx_webhooks/bot_comments.py | 35 +++++++-------- .../github_community_pr_comment.md.j2 | 11 +++-- tests/test_pull_request_opened.py | 44 +++++++++++++++++-- 3 files changed, 64 insertions(+), 26 deletions(-) diff --git a/openedx_webhooks/bot_comments.py b/openedx_webhooks/bot_comments.py index fef44800..2d49243f 100644 --- a/openedx_webhooks/bot_comments.py +++ b/openedx_webhooks/bot_comments.py @@ -5,21 +5,21 @@ import binascii import json import re +from collections import namedtuple from enum import Enum, auto -from typing import Dict +from typing import Dict, Literal import arrow -import yaml from flask import render_template from openedx_webhooks.info import ( get_jira_server_info, is_draft_pull_request, - pull_request_has_cla, read_github_file, + pull_request_has_cla, + get_catalog_info, ) from openedx_webhooks.types import JiraId, PrDict -from openedx_webhooks.utils import memoize_timed # Author association values for which we should consider the author new GITHUB_NEW_AUTHOR_ASSOCIATIONS = ( @@ -94,28 +94,24 @@ def is_comment_kind(kind: BotComment, text: str) -> bool: return any(snip in text for snip in BOT_COMMENT_INDICATORS[kind]) -@memoize_timed(minutes=30) -def get_repo_catalog(repo_full_name: str) -> dict: - """ - Get and load the catalog-info.yaml file for the repo. - """ - return yaml.safe_load(read_github_file(repo_full_name, "catalog-info.yaml", "")) +Lifecycle = Literal["experimental", "production", "deprecated"] +RepoSpec: (str | None, Lifecycle | None) = namedtuple('RepoSpec', ['owner', 'lifecycle']) -def _get_repo_owner(repo_full_name: str) -> str | None: +def _get_repo_spec(repo_full_name: str) -> RepoSpec: """ Get the owner of the repo from its catalog-info.yaml file. """ - repo_catalog = get_repo_catalog(repo_full_name) - if not repo_catalog: - return None - owner = repo_catalog["spec"]["owner"] - owner_type = "group" + catalog_info = get_catalog_info(repo_full_name) + if not catalog_info: + return RepoSpec(None, None) + owner = catalog_info["spec"].get("owner", "") + owner_type = None if ":" in owner: owner_type, owner = owner.split(":") if owner_type == "group": owner = f"openedx/{owner}" - return owner + return RepoSpec(owner, catalog_info["spec"]["lifecycle"]) def github_community_pr_comment(pull_request: PrDict) -> str: @@ -128,7 +124,7 @@ def github_community_pr_comment(pull_request: PrDict) -> str: * contain a link to our process documentation """ is_first_time = pull_request.get("author_association", None) in GITHUB_NEW_AUTHOR_ASSOCIATIONS - owner = _get_repo_owner(pull_request["base"]["repo"]["full_name"]) + spec = _get_repo_spec(pull_request["base"]["repo"]["full_name"]) return render_template( "github_community_pr_comment.md.j2", @@ -137,7 +133,8 @@ def github_community_pr_comment(pull_request: PrDict) -> str: is_draft=is_draft_pull_request(pull_request), is_merged=pull_request.get("merged", False), is_first_time=is_first_time, - owner=owner, + owner=spec.owner, + lifecycle=spec.lifecycle, ) diff --git a/openedx_webhooks/templates/github_community_pr_comment.md.j2 b/openedx_webhooks/templates/github_community_pr_comment.md.j2 index c0b92b1a..eb5da04d 100644 --- a/openedx_webhooks/templates/github_community_pr_comment.md.j2 +++ b/openedx_webhooks/templates/github_community_pr_comment.md.j2 @@ -1,4 +1,4 @@ -Thanks for the pull request, `@{{ user }}`! +Thanks for the pull request, @{{ user }}! ### What's next? @@ -70,8 +70,13 @@ Your PR is currently marked as a draft. After completing the steps above, updat {% if owner %} This repository is currently maintained by `@{{ owner }}`. Tag them in a comment and let them know that your changes are ready for review. -{% else %} -This repository is currently unmaintained. To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review: +{% else %} +{% if lifecycle == 'production' %} +This repository has no maintainer (yet). +{% else %} +This repository is currently unmaintained. +{% endif %} +To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review: 1. On the right-hand side of the PR, find the Contributions project, click the caret in the top right corner to expand it, and check the "Primary PM" field for the name of your PM. 2. Find their GitHub handle [here](https://openedx.atlassian.net/wiki/spaces/COMM/pages/3548807177/Community+Contributions+Project+Manager#Current-OSPR-Project-Managers). diff --git a/tests/test_pull_request_opened.py b/tests/test_pull_request_opened.py index c20c98f0..a49a49c2 100644 --- a/tests/test_pull_request_opened.py +++ b/tests/test_pull_request_opened.py @@ -1,9 +1,11 @@ """Tests of tasks/github.py:pull_request_changed for opening pull requests.""" import textwrap +from unittest import mock import pytest +from openedx_webhooks import settings from openedx_webhooks.bot_comments import ( BotComment, is_comment_kind, @@ -16,10 +18,8 @@ CLA_STATUS_NO_CONTRIBUTIONS, CLA_STATUS_PRIVATE, ) -from openedx_webhooks import settings from openedx_webhooks.gh_projects import pull_request_projects from openedx_webhooks.tasks.github import pull_request_changed - from .helpers import check_issue_link_in_markdown # These tests should run when we want to test flaky GitHub behavior. @@ -75,6 +75,42 @@ def test_pr_in_nocontrib_repo_opened(fake_github, user): assert pull_request_projects(pr.as_json()) == set() +@pytest.mark.parametrize("owner,tag", [ + ("group:arch-bom", "@openedx/arch-bom"), + ("user:feanil", "@feanil"), + ("feanil", "@feanil"), +]) +@mock.patch("openedx_webhooks.bot_comments.get_catalog_info") +def test_pr_with_owner_repo_opened(get_catalog_info, fake_github, owner, tag): + get_catalog_info.return_value = { + 'spec': {'owner': owner, 'lifecycle': 'production'} + } + pr = fake_github.make_pull_request(owner="openedx", repo="edx-platform") + result = pull_request_changed(pr.as_json()) + assert not result.jira_issues + pr_comments = pr.list_comments() + assert len(pr_comments) == 1 + body = pr_comments[0].body + assert f"This repository is currently maintained by `{tag}`" in body + +@pytest.mark.parametrize("lifecycle", ["production", "deprecated", None]) +@mock.patch("openedx_webhooks.bot_comments.get_catalog_info") +def test_pr_without_owner_repo_opened(get_catalog_info, fake_github, lifecycle): + get_catalog_info.return_value = { + 'spec': {'lifecycle': lifecycle} + } if lifecycle else None + pr = fake_github.make_pull_request(owner="openedx", repo="edx-platform") + result = pull_request_changed(pr.as_json()) + assert not result.jira_issues + pr_comments = pr.list_comments() + assert len(pr_comments) == 1 + body = pr_comments[0].body + if lifecycle == "production": + assert f"This repository has no maintainer (yet)." in body + else: + assert f"This repository is currently unmaintained." in body + + def test_pr_opened_by_bot(fake_github): fake_github.make_user(login="some_bot", type="Bot") pr = fake_github.make_pull_request(user="some_bot") @@ -99,7 +135,7 @@ def test_external_pr_opened_no_cla(fake_github): assert len(pr_comments) == 1 body = pr_comments[0].body check_issue_link_in_markdown(body, None) - assert "Thanks for the pull request, `@new_contributor`!" in body + assert "Thanks for the pull request, @new_contributor!" in body assert is_comment_kind(BotComment.NEED_CLA, body) assert is_comment_kind(BotComment.WELCOME, body) @@ -132,7 +168,7 @@ def test_external_pr_opened_with_cla(fake_github): assert len(pr_comments) == 1 body = pr_comments[0].body check_issue_link_in_markdown(body, None) - assert "Thanks for the pull request, `@tusbar`!" in body + assert "Thanks for the pull request, @tusbar!" in body assert is_comment_kind(BotComment.WELCOME, body) assert not is_comment_kind(BotComment.NEED_CLA, body)