8000 Disable CI status change comments unless the "automerge" label is used by ambv · Pull Request #578 · python/miss-islington · GitHub
[go: up one dir, main page]
Skip to content

Disable CI status change comments unless the "automerge" label is used #578

8000 New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 9, 2022
Merged

Disable CI status change comments unless the "automerge" label is used #578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b19c203
Disable CI status change comments unless the "automerge" label is used
ambv Sep 9, 2022
fef2c2f
Merge failure cases
ambv Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions miss_islington/status_change.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ async def check_status(event, gh, *args, **kwargs):
event.data["commit"].get("committer")
and event.data["commit"]["committer"]["login"] == "miss-islington"
):
await check_ci_status_and_approval(gh, sha, leave_comment=True)
# Leave comment temporarily disabled when automerge not used. See #577.
await check_ci_status_and_approval(gh, sha, leave_comment=False)
else:
pr_for_commit = await util.get_pr_for_commit(gh, sha)
if pr_for_commit:
Expand Down Expand Up @@ -106,12 +107,9 @@ async def check_ci_status_and_approval(
)
success = result["state"] == "success" and not failure
if leave_comment:
if failure:
if failure or not success:
emoji = "❌"
status = "it's a failure or timed out"
elif not success:
emoji = "❌"
status = "it's a failure"
Comment on lines -109 to -114
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change intentional?

@DanielNoord recently fixed this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ambv Was this indeed intentional? If not I could open a PR to revert it to my last fix?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezio-melotti I checked in cpython and it seems to be okay, although there are some weird comments that are probably due to checks timing out and then being rerun.

It doesn't regress on #576 but it does remove the narrowing down that the "it's a failure" comment gives. Previously if that was the only thing in the comment you knew there wasn't a timeout, so there was likely an actual issue with the PR. You no longer get this information, but I'm not sure it warrants a PR to re-introduce that. It doesn't seem to helpful anyway.

That said, it could be simplified to if not success. If failure == True then success will always be False, so that doesn't need double-checking. Want me to open a PR for that?

Copy link
Member

Choose a reason for hiding this com 8000 ment

The reason will be displayed to describe this comment to others. Learn more.

In python/cpython#97820 (comment) I got the Status check is done, and it's a failure or timed out ❌. message, even though there was no evident failure. python/cpython#97821 (comment) reported success.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to debug these comments after the fact.
https://api.github.com/repos/python/cpython/commits/66eca5d2f185cd6943afeb69dae3eba625bbd077/status
https://api.github.com/repos/python/cpython/commits/66eca5d2f185cd6943afeb69dae3eba625bbd077/check-runs

Both show that there are no failed runs. So I don't know what is going on there honestly... But I'm also fairly sure it isn't caused by the changes in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find it anymore but I had an example where it was a timeout but TIMED_OUT was not set so the bot would report a failure. And since the best the message could say anyway was that it's either a failure or a timeout, I decided to just shorten it.

IIRC this change also let me not touch more tests, which otherwise I would have to alter due to the leave_comment=False change.

What's more worrying is that @ezio-melotti's example are still the bot leaving comments even those leave_comment=False is set. What gives?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out this was due to a missing change that I now also cover in #583

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I remember what I meant by "this change also let me not touch more tests". The change in this PR caused test coverage to fall due to one of those branches to not be exercised anymore.

else:
emoji = "✅"
status = "it's a success"
Expand Down
32 changes: 19 additions & 13 deletions tests/test_status_change.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,9 @@ async def test_ci_passed_with_awaiting_merge_label_pr_is_merged():

gh = FakeGH(getitem=getitem, getiter=getiter)
await status_change.router.dispatch(event, gh)
expected_body = "Status check is done, and it's a success ✅."
assert gh.post_data["body"] == expected_body
# Leave comment temporarily disabled when automerge not used. See #577.
# expected_body = "Status check is done, and it's a success ✅."
# assert gh.post_data["body"] == expected_body
assert gh.put_data["sha"] == sha # is merged
assert gh.put_data["merge_method"] == "squash"
assert (
Expand Down Expand Up @@ -197,8 +198,9 @@ async def test_ci_and_check_run_passed_with_no_awaiting_merge_label_pr_is_not_me

gh = FakeGH(getitem=getitem)
await status_change.router.dispatch(event, gh)
expected_body = "Status check is done, and it's a success ✅."
assert gh.post_data["body"] == expected_body
# Leave comment temporarily disabled when automerge not used. See #577.
# expected_body = "Status check is done, and it's a success ✅."
# assert gh.post_data["body"] == expected_body
assert not hasattr(gh, "put_data") # is not merged


Expand Down Expand Up @@ -254,8 +256,9 @@ async def test_ci_not_passed_awaiting_merge_label_pr_is_not_merged():

gh = FakeGH(getitem=getitem)
await status_change.router.dispatch(event, gh)
expected_body = "@miss-islington and @Mariatta: Status check is done, and it's a failure ❌."
assert gh.post_data["body"] == expected_body
# Leave comment temporarily disabled when automerge not used. See #577.
# expected_body = "@miss-islington and @Mariatta: Status check is done, and it's a failure ❌."
# assert gh.post_data["body"] == expected_body
assert not hasattr(gh, "put_data") # is not merged


Expand Down Expand Up @@ -323,8 +326,9 @@ async def test_ci_passed_and_check_run_failure_awaiting_merge_label_pr_is_not_me

gh = FakeGH(getitem=getitem, getiter=getiter)
await status_change.router.dispatch(event, gh)
expected_body = "@miss-islington and @Mariatta: Status check is done, and it's a failure or timed out ❌."
assert gh.post_data["body"] == expected_body
# Leave comment temporarily disabled when automerge not used. See #577.
# expected_body = "@miss-islington and @Mariatta: Status check is done, and it's a failure or timed out ❌."
# assert gh.post_data["body"] == expected_body
assert not hasattr(gh, "put_data") # is not merged


Expand Down Expand Up @@ -471,8 +475,9 @@ async def test_ci_passed_and_check_run_pending_awaiting_merge_label_pr_is_not_me

gh = FakeGH(getitem=getitem, getiter=getiter)
await status_change.router.dispatch(event, gh)
expected_body = "@miss-islington and @Mariatta: Status check is done, and it's a failure or timed out ❌."
assert gh.post_data["body"] == expected_body
# Leave comment temporarily disabled when automerge not used. See #577.
# expected_body = "@miss-islington and @Mariatta: Status check is done, and it's a failure or timed out ❌."
# assert gh.post_data["body"] == expected_body
assert not hasattr(gh, "put_data") # is not merged


Expand Down Expand Up @@ -540,9 +545,10 @@ async def test_ci_passed_and_check_run_timed_out_awaiting_merge_label_pr_is_not_

gh = FakeGH(getitem=getitem, getiter=getiter)
await status_change.router.dispatch(event, gh)
expected_body = ("@miss-islington and @Mariatta: Status check is done, "
"and it's a failure or timed out ❌.")
assert gh.post_data["body"] == expected_body
# Leave comment temporarily disabled when automerge not used. See #577.
# expected_body = ("@miss-islington and @Mariatta: Status check is done, "
# "and it's a failure or timed out ❌.")
# assert gh.post_data["body"] == expected_body
assert not hasattr(gh, "put_data") # is not merged


Expand Down
0