Skip to content
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.

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

Change how build failure comments are generated #631

Merged
merged 7 commits into from
Aug 7, 2024
Merged

Change how build failure comments are generated #631

merged 7 commits into from
Aug 7, 2024

Conversation

kwk
Copy link
Collaborator

@kwk kwk commented Aug 7, 2024

  1. Upon issue creation we create minimized comments for all chroots and
    mark them with i.e. <!--ERRORS_FOR_CHROOT/fedora-rawhide-x86_64-->.
  2. When we find an error in a chroot we update the above created comment
    and unminimize it.
  3. In case when there's no error for a chroot, we're gonna minimize the
    comment again.

Fixes #626

1. Upon issue creation we create minimized comments for all chroots and
   mark them with i.e. `<!--ERRORS_FOR_CHROOT/fedora-rawhide-x86_64-->`.
2. When we find an error in a chroot we update the above created comment
   and unminimize it.
3. In case when there's no error for a chroot, we're gonna minimize the
   comment again.

Fixes #626
@kwk kwk requested a review from nikic August 7, 2024 11:33
@kwk kwk added enhancement New feature or request python labels Aug 7, 2024
@nikic
Copy link
Collaborator

nikic commented Aug 7, 2024

It would be good to address this part of the issue at the same time:

When doing that, we should also only set the assignee after posting these comments. That way the maintainer will only receive one notification per day, rather than one per failed build.

Otherwise whoever is maintainer will get ~20 not very helpful emails for empty comments (unless GitHub suppresses them in this case?)

@kwk
Copy link
Collaborator Author

kwk commented Aug 7, 2024

It would be good to address this part of the issue at the same time:

When doing that, we should also only set the assignee after posting these comments. That way the maintainer will only receive one notification per day, rather than one per failed build.

Otherwise whoever is maintainer will get ~20 not very helpful emails for empty comments (unless GitHub suppresses them in this case?)

@nikic I've made the change as you suggested. We'll have to ask @tuliom tomorrow if he got more than one notification.

@nikic
Copy link
Collaborator

nikic commented Aug 7, 2024

Thanks! I think an adjustment to initial_comment may also be needed, as that also uses maintainer_handle ping (and thus subscribes by mail to the issue). I think it's fine to just drop the "hi" there :)

…te_or_get_todays_github_issue

Also remove the `Hello @{self.config.maintainer_handle}!` from the
initial issue comment to not automatically subscribe the maintainer to
the issue.
nikic
nikic previously approved these changes Aug 7, 2024
Copy link
Collaborator

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

snapshot_manager/snapshot_manager/github_util.py Outdated Show resolved Hide resolved
snapshot_manager/snapshot_manager/snapshot_manager.py Outdated Show resolved Hide resolved
@kwk kwk merged commit 0df2cdc into main Aug 7, 2024
5 of 7 checks passed
@kwk kwk deleted the fix-626 branch August 7, 2024 13:40
@nikic
Copy link
Collaborator

nikic commented Aug 9, 2024

I got 14 empty notifications todays :) It seems like there is a bit of a race condition on GitHub's side, so if a lot of actions are taken in a short time frame, it may end up sending notifications for stuff that happened before you subscribed to the issue.

This can probably be "fixed" just by waiting for a minute before doing the assignment...

@kwk
Copy link
Collaborator Author

kwk commented Aug 12, 2024

I got 14 empty notifications todays :) It seems like there is a bit of a race condition on GitHub's side, so if a lot of actions are taken in a short time frame, it may end up sending notifications for stuff that happened before you subscribed to the issue.

I feared this because Github certainly does so when assigning and removing labels. It groups the actions together it seemed.

This can probably be "fixed" just by waiting for a minute before doing the assignment...

How about assigning the maintainer only on the second run of the checks like done in #643? The second run is still way to early to expect any results anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change how build failure comments are generated
2 participants