python / bedevere Goto Github PK

This is instead of a "trivial" label to give finer-grained control over what is (or is not) important when other checks are added that should also be conditionally skipped.

See https://mail.python.org/pipermail/python-dev/2017-June/148486.html

Just a mild cosmetic issue but FYI.

Example:
python/cpython#5412 (comment)

As an enhancement to python/core-workflow#168 and #60, bedevere can dismiss any codeowner review requests for PRs that tries to merge the maintenance branch into master.

API for deleting PR review request:
https://developer.github.com/v3/pulls/review_requests/#delete-a-review-request

Recent example for such PR: python/cpython#4047

Currently readme.md only mentions that this bot identifies missing issue number.

It actually does more now:

• identifies missing misc/news.d file
• automatically links bpo
• automatically closes invalid pr
• automatically labels documentation prs with type-documentation label
• copies over labels from master PR to backport PRs
• etc

It would be great if the readme can be updated with these information.

Same issue as described in python/the-knights-who-say-ni#148.
Bedevere is attempting to apply a label to such PR, causing gidgethub.BadRequest: Not Found error.

Perhaps we can catch the error here, so I won't keep getting error alerts.
Thanks.

See python/cpython#3077 (comment) where core devs who only commented were notified.

Link to bugs.python.org, not www.bugs.python.org.

We're only seeing: No issue number prepended to the title or "skip is...
python/cpython#2975

When I run bedevere's test suite in Python 3.7.2, there are failing tests, but I don't know whether the fix is in bedevere or one of the dependencies.

$ python3.7
Python 3.7.2 (v3.7.2:9a3ffc0492, Dec 24 2018, 02:44:43) 
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.

python3.7 -m coverage run --branch -m pytest tests/
=========================================================================================================== test session starts ===========================================================================================================
platform darwin -- Python 3.7.2, pytest-4.2.0, py-1.7.0, pluggy-0.8.1
rootdir: /Users/mariatta/python_github/bedevere, inifile: pytest.ini
plugins: asyncio-0.10.0, aiohttp-0.3.0
collected 128 items                                                                                                                                                                                                                       

tests/test___main__.py ...                                                                                                                                                                                                          [  2%]
tests/test_backport.py ...FFFFF............                                                                                                                                                                                         [ 17%]
tests/test_bpo.py ................................                                                                                                                                                                                  [ 42%]
tests/test_close_pr.py .......                                                                                                                                                                                                      [ 48%]
tests/test_filepaths.py ......                                                                                                                                                                                                      [ 53%]
tests/test_follow_up.py FF.                                                                                                                                                                                                         [ 55%]
tests/test_news.py ........................                                                                                                                                                                                         [ 74%]
tests/test_prtype.py .........                                                                                                                                                                                                      [ 81%]
tests/test_stage.py F.....FFFFF.....                                                                                                                                                                                                [ 93%]
tests/test_util.py ........                                                                                                                                                                                                         [100%]

One of the results:

_______________________________________________________________________________________ test_awaiting_label_removed_when_pr_merged[awaiting review] _______________________________________________________________________________________

label = 'awaiting review'

    @pytest.mark.parametrize("label", awaiting_labels)
    async def test_awaiting_label_removed_when_pr_merged(label):
        encoded_label = label.replace(" ", "%20")
    
        issue_url = "https://api.github.com/repos/org/proj/issues/3749"
        data = {
            "action": "closed",
            "pull_request": {
                "merged": True,
                "issue_url": issue_url,
            }
        }
        event = sansio.Event(data, event="pull_request", delivery_id="12345")
    
        issue_data = {
            issue_url: {
                "labels": [
                    {"url": f"https://api.github.com/repos/python/cpython/labels/{encoded_label}",
                     "name": label,
                     },
                    {
                      "url": "https://api.github.com/repos/python/cpython/labels/CLA%20signed",
                      "name": "CLA signed",
                    },
                ],
                "labels_url": "https://api.github.com/repos/python/cpython/issues/12345/labels{/name}",
            },
        }
    
        gh = FakeGH(getitem=issue_data)
    
>       await awaiting.router.dispatch(event, gh)

tests/test_stage.py:585: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
venv/lib/python3.7/site-packages/gidgethub/routing.py:80: in dispatch
    await callback(event, *args, **kwargs)
bedevere/stage.py:232: in closed_pr
    await _remove_stage_labels(gh, issue)
bedevere/stage.py:120: in _remove_stage_labels
    await gh.delete(issue["labels_url"], {"name": stale_name})
tests/test_stage.py:36: in delete
    self.delete_url = sansio.format_url(url, url_vars)
venv/lib/python3.7/site-packages/gidgethub/sansio.py:341: in format_url
    expanded_url: str = uritemplate.expand(url, var_dict=url_vars)
venv/lib/python3.7/site-packages/uritemplate/api.py:33: in expand
    return URITemplate(uri).expand(var_dict, **kwargs)
venv/lib/python3.7/site-packages/uritemplate/template.py:132: in expand
    return self._expand(_merge(var_dict, kwargs), False)
venv/lib/python3.7/site-packages/uritemplate/template.py:97: in _expand
    expanded.update(v.expand(expansion))
venv/lib/python3.7/site-packages/uritemplate/variable.py:338: in expand
    expanded = expansion(name, value, opts['explode'], opts['prefix'])
venv/lib/python3.7/site-packages/uritemplate/variable.py:204: in _label_path_expansion
    if dict_test(value) or tuples:
venv/lib/python3.7/site-packages/uritemplate/variable.py:363: in dict_test
    return isinstance(value, (dict, collections.MutableMapping))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

name = 'MutableMapping'

    def __getattr__(name):
        # For backwards compatibility, continue to make the collections ABCs
        # through Python 3.6 available through the collections module.
        # Note, no new collections ABCs were added in Python 3.7
        if name in _collections_abc.__all__:
            obj = getattr(_collections_abc, name)
            import warnings
            warnings.warn("Using or importing the ABCs from 'collections' instead "
                          "of from 'collections.abc' is deprecated, "
                          "and in 3.8 it will stop working",
>                         DeprecationWarning, stacklevel=2)
E           DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working

/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/collections/__init__.py:52: DeprecationWarning

When the Knight adds the 'CLA signed' label, also delete the big message about how to sign the CLA. Once signed, it is just noise that detracts from reading the page. I have sometimes done this manually, as with #2975, but the bot would be better.

Merging PR with 'awaiting merge' or 'awaiting review' does not remove labels.

Since we don't have pre-commit hooks to block people from doing something that goes against what we outline in the devguide, we could leave a comment post-commit to remind core devs of how they should be doing something. That way there's at least a feedback loop to help core devs change their habits going forward.

PR python/cpython#5500 is a backport for python/cpython#5265

Bedevere should have removed the needs backport PR from python/cpython#5265.

E.g. make "bpo-31404" into "bpo-31404" in any comment.

Proposed in python/core-workflow#58 (comment)

Since @brettcannon said this can be a bedevere thing, I opened this issue :)

When a PR is created, bedevere can check if [X.Y] is in the PR title, and then apply cherry-pick for X.Y label.

1. backports created with cherry_picker.py have [X.Y] for sure
2. those not using cherry_picker.py have been instructed to do that in the devguide
3. even when the cherry-picking bot is in place, I suppose it will also provide [X.Y] as the PR title. This might simplify the cherry-picking bot to just do the cherry-pick and PR creation, and not worry about applying labels.
4. (non core-dev) contributors can still use cherry_picker.py and the labels still get applied without coredev's intervention.
5. Backports created using cherry_picker.py will also have the link to the original GitHub PR, (GH-XXX in the title/desc), bedevere can remove the needs backport to [X.Y] label from the original PR (I think)

Currently, the link back to b.p.o is only shown as a status check when PR is not yet merged.
Often I still re-read a merged PR, for example while backporting changes.
After a cherry-pick PR has been merged, we're supposed to go back to the bug tracker and close the issue.
However, since the PR has been merged, the status check is no longer visible.
So I think it will be nice if the link to the bug tracker can be added as a GitHub comment that is always going to be available.

What do you think about this?

In PR python/cpython#3274 bedevere correctly kept the "No news entry in Misc/NEWS.d or "skip news" label found" warning on the PR. Presumably because it knew I had a typo in my NEWS.d filename (.bps- instead if .bpo-)?

I misunderstood its warning as just not noticing that I had added a NEWS.d file later. Could its warning text highlight unexpected filenames within a NEWS.d directory? or is this not worth doing? (I've learned - the question is how many others will learn the hard way)

When I updated python/cpython#2573, a bedevere/news 'No news entry' comment appeared, with a red X. I created one with blurb and pushed again -- see the end of
https://github.com/python/cpython/pull/2573/files#diff-3b70d006c35749af8a303582e5346202
However, bedevere did not recheck and is now wrong after 20 minutes.

For miss-islington, I'm finding myself needing to write the same code that's already in bedevere/util.py, specifically normalize_title(), is_core_dev(), issue_for_PR() methods.

Wondering if we want to extract those as a shared library for the bots.
Wanted to get your thoughts about this @brettcannon.

Saw 500 errors in the webhooks for several PRs:
python/cpython#5748
python/cpython#5746
python/cpython#5747

The library has changed testing API.
The bot uses non-public attributes, that's why tests fail.

I could find a time for working on issue on next week if nobody will pickup the task first

The labels for type-documentation and type-tests could automatically be added to pull requests based on the types of files included. For example, type-documentation would contain all .rst files, while type-tests would contain all files that start with test_ (except for the blurb). It may not catch all the changes for documentation and tests, but it would identify some, making triage of pull requests a bit easier. The other types (bugfix or enhancement) might be able to be retrieved from the bpo.

I started working on this, but I ran into a question. Currently check_news reads through the files to look for the blurb. Since I plan on looking at all the file names as well, I was wondering if the code to get all the files from the PR should be moved to a level above check_news and the new function.

For example in new files.py:

@router.register('pull_request', action='opened')
async def file_checks(event, gh, *args, **kwargs):
    pr_number = event.data['number']
    filelist = get_files_from_PR(number)
    await check_news(filelist)
    await check_type(filelist)

instead of retrieving the files twice:

@router.register('pull_request', action='opened')
async def check_news(event, gh, *args, **kwargs):
    pr_number = event.data['number']
    files_url = f'/repos/python/cpython/pulls/{pr_number}/files'
    async for file_data in gh.getiter(files_url):
        filename = file_data['filename']
        etc

@router.register('pull_request', action='opened')
async def check_type(event, gh, *args, **kwargs):
    pr_number = event.data['number']
    files_url = f'/repos/python/cpython/pulls/{pr_number}/files'
    async for file_data in gh.getiter(files_url):
        filename = file_data['filename']
        etc

I don't know what the pitfalls would be of trying to get all the files upfront, especially with this comment in check_news.

    # Could have collected all the filenames in an async set comprehension,
    # but doing it this way potentially minimizes the number of API calls.

I know it will be deployed after the PR was merged, but I guess it still take some moments before it finished deployed in Heroku.

I'd be interested to know when it has been deployed so I can test the changes afterwards.

I was going through several old PRs, and tried to retrigger all CI and status checks by closing and re-opening the PR.

Seems like the news status check is not being done when PR is reopened.

Example: python/cpython#1766

In python/cpython#8857, they created an empty news file (to make the status pass?) 🤷‍♀️
I think it is possible to check for the content of the file, and ensure it is not empty, before posting the success status.

python/cpython#3226 showed that this can lead to the PR creator accidentally causing a transition to awaiting core review simply by leaving a comment themselves to ask a question.

This one keeps catching me out, if bedevere can spot the pattern to generate the warning, could it not just pick out what it needs?

eg: python/cpython#11176

The bot continues to ping everyone who has submitted a review, even if they have approved their review. See this example where Serhiy was pinged. python/cpython#3066 (comment)

See python/cpython#3077 (comment) for what currently happens.

In python/cpython#3114, the backport labels were not automatically removed by bedevere-bot.

The backport PRs were initially created without [X.Y] prefix in PR title.
After the title has been edited, the backport PR labels were still not removed.

This happened in python/cpython#1804 too, where I ended up removing the label myself.

Just curious if this is because the title was edited afterwards, or is there another reason of why bedevere couldn't detect the backport PRs?

Thanks.

When there are multiple reviewers who requested changes, the "awaiting change" label should only be removed after all reviewers approved the new changes.

As brought up by @1st1 in python/cpython#4314:

core-dev A requests changes; bedevere-bot assigns an awaiting changes label.
core-dev B approves the PR; bedevere-bot removes the awaiting changes label.
The label should only be removed when all reviewers clear the PR.

I have some closed/merged PRs that have one of the "awaiting" on them, and I guess it's common to forget to remove tags. I believe these are unnecessary entries that clutter the view when you people filter the list.

I thought they could be automatically removed when a PR is closed/merged. They aren't useful anymore, right?

If this is deemed useful, I could make a PR.

Perhaps backport PRs made by miss-islington can go straight to awaiting merge, so it can be automatically merged after all CI passed.

The bpo number provided in python/cpython#1798 is not valid (46895)
https://bugs.python.org/issue46895 is a 404

Maybe bedevere can also validate whether the issue number is correct :)

Some of the recent maintenance branch merge PRs (such as python/cpython#10104) appear to actually be people trying to create a bug report in a very poor manner. It would be nice to leave a message from @bedevere-bot in such a case; just something along the lines of "PRs attempting to merge a maintenance branch into the master branch are deemed to be spam and automatically closed. If you were attempting to report a bug, please go to bugs.python.org; see devguide.python.org for further instruction as needed." Currently the only message is the general "sign the CLA!" message from @the-knights-who-say-ni, which I imagine could be quite confusing for someone who's already confused about how to report a bug :)

#3635 I had to add [skip news] twice to get bedevere to recognize it. For the backport #3637, thrice. This is common with [skip news] and a nuisance. Can bedevere be made more 'attentive'? Can a 'check again' button be added? Can clicking details be made to recheck?

On python/cpython#3684 the file was added but bedevere doesn’t see it. Adding the skip label makes the check pass, removing it doesn’t cause a succesfull re-check.

python/cpython#7765
is a trivial doc change intended for master, 3.7, 3.6, and 2.7. I reviewed,
approved, marked labels, and after a 2nd approval, merged it. miss-islington
cherry-picked it to 3.7, but failed for 3.6 and 2.7. The author then discovered
that the 3.6 failure was because the original PR was (accidentally) against
3.6, not master.

On core_mentorship, Nick suggested that the best way to catch such an error is for Bedevere to check consistency between between branch merge target (in above case, 3.6) and the presence or absence of a branch label (in above case, none).

"When bedevere checks for the BPO issue reference, it could also check
that the target branch matches the commit message prefix (i.e. if
there's no "[3.6] " at the start, then targetting the 3.6 branch is
wrong - PRs without a version prefix should always be targeting
master, and PRs deliberately targeting a maintenance branch directly
should include the prefix).
"
Possible message:
"If this PR is really for 3.6, prefix [3.6] to the title.
If this PR is for master, CLOSE IT and make a new PR."

After the review that prompted python/cpython#3077 (comment), I did a second review where I replied to some of Yury & Serhiy's comments.

Bedevere picked that up as a new review and repeated its comment: python/cpython#3077 (comment)

Without knowing the details of how Bedevere works, I'm guessing the simplest way of avoiding that would be to skip commenting if the "awaiting changes" label is already set.

Hi,

You can check with this PR, the-knights-who-say-hi add the label CLA not signed but after, bedevere add awaiting review.

Normally, the bedevere should only add awaiting review if the CLA is signed. Is it right?

python/cpython#9840

When backporting, I try to copy over labels from the original PR to the backport PR, for example the "trivial" or "documentation" labels.

Perhaps this is something that can be automated and done by bedevere.
At the very least, the "trivial" label should be copied over.

UPDATE:
Copy over skip issue label instead of trivial. trivial label is no more.

UPDATE 2.0:
At the very least, copy over skip issue and skip news labels.

python/cpython#8449 is a fix for a bug that doesn't appear in Python 3. There's no corresponding PR for master. But, Bedevere's backport check sees a PR for a maintenance branch, and insists that there should be a (GH-XXX) note in the title.

I didn't find guidance for this specific case in the devguide, but I think the correct thing to do is to merge without the (GH-XXX) note, ignoring the status check.

For example: python/cpython#1120

It has both bpo-NNNN in the PR title and trivial label.
bedevere status check should still have details link to the said issue.

Thanks :)

Automatically changing bpo-DDDD to a link to bugs.python.org was introduced in #121 . It's a useful feature but there are a few cases where the PR was raised with wrong title format and while specifying the correct format and the bot rewrites bpo number in the text as a link causing confusion. It does rewriting without notification and thus the comments are silently overwritten unless OP re-reads it.

Example : While commenting the correct format in python/cpython#9906 (comment) bedevere changed bpo-34996: Add name parameter to proccess and thread pool to [bpo-34996](https://bugs.python.org/issue34996): Add name parameter to proccess and thread pool . The user was confused and changed the PR title with URL. Using backticks also doesn't stop the bot. Is there a way to specify bpo-DDDD without adding hyperlinks in the comments? Maybe this can be skipped in the backticks?

Another example where spaces have to be used to stop rewriting : python/cpython#9755 (comment)

Thanks

I left a comment in this commit python/cpython@eeadf5f#r31642634, mentioning bpo-31339.
I was expecting that it would be linked to bugs.python.org/issue31339, but it didn't 🙁

We don't leave commit comments very often, but it would be nice it this had worked :)

If this gets implemented, we need to enable the commit comment webhook event. (currently it is not enabled).

This PR from @vstinner was initially labeled "awaiting merge", which I suppose is the default behavior when a core dev makes a PR, even before review.

I (not a core dev) reviewed the code and approved the changes, which triggered bedevere to remove "awaiting merge" and add "awaiting core review".

I'm guessing this is because my review triggered the logic that normally transitions from "awaiting review" to "awaiting core review", and this probably checked to see if there were any reviews from core devs (there were not) and assumed that core review was necessary.

I would think the correct behavior for an accepted review would be to leave "awaiting merge" in place. For a rejected review, adding "awaiting core review" might be the right thing to do.

I wonder whether it's possible or a good idea to automatically add pull request participants to bpo issue nosy list. Sometimes PR authors or reviewers don't add themselves to bpo issue nosy list and then might lose some notifications happened in bpo.

If PR title starts with wip or wip:, apply DO-NOT-MERGE label.
Remove the label if wip has been removed.

Requested by @asvetlov during sprint.