Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50307 closed enhancement (wontfix)

Disable Slack notifications for non-version branch builds

Reported by: desrosj's profile desrosj Owned by: desrosj's profile desrosj
Milestone: Priority: lowest
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

Currently, failed pull request builds are posted into the #core Slack room.

It looks like the configuration is set correctly to disable these based on the documentation, but they still seem to be coming through.

GitHub pull requests are often used to try out potential solutions or work through problems over time, and are also a newly embraced way to contribute to Trac tickets.

This will cause quite a bit of noise as more and more contributors use GitHub PRs to contribute, and also raises false alarms.

Attachments (4)

50307.diff (999 bytes) - added by jorbin 4 years ago.
50307.2.diff (960 bytes) - added by jorbin 4 years ago.
50307.3.diff (990 bytes) - added by jorbin 4 years ago.
50307.4.diff (992 bytes) - added by jorbin 4 years ago.

Download all attachments as: .zip

Change History (21)

#1 follow-up: @ocean90
4 years ago

  • Keywords close added

I'm assuming you created the ticket based on these notifications? There are a few open and closed PRs with a failed build status which didn't generate a notification so I guess it was just a temporary glitch.

@jorbin
4 years ago

#2 in reply to: ↑ 1 @desrosj
4 years ago

Replying to ocean90:

I'm assuming you created the ticket based on these notifications? There are a few open and closed PRs with a failed build status which didn't generate a notification so I guess it was just a temporary glitch.

Correct. It did seem weird, but wanted to have a ticket in case to track. I'll do some testing to try and trigger the notifications.

#3 @jorbin
4 years ago

Some other proejects such as https://github.com/apache/openwhisk/blob/master/.travis.yml#L37-L39

have the order different. I wonder if travis is encountering the on_failures: always and following that and never seeing the pull request part. My patch changes that. Worth a try?

#4 @desrosj
4 years ago

  • Keywords has-patch added; close removed

@jorbin that crossed my mind as a possibility as well. I don't think it hurts to try it.

#5 @TimothyBlynJacobs
4 years ago

I think that only happens on pull requests that are from a branch on the repository itself, not from a different repository.

#6 @ocean90
4 years ago

@TimothyBlynJacobs Good catch! So it was the notification for the branch and not the PR.

50307.diff doesn't make a difference in that case.

#7 @jorbin
4 years ago

whomp whomp

@jorbin
4 years ago

#9 @ocean90
4 years ago

We should make sure to send the notifications also for the release branches (and tags?).

@jorbin
4 years ago

#10 @desrosj
4 years ago

  • Owner set to desrosj
  • Status changed from new to reviewing
  • Summary changed from Disable Slack notifications for pull request builds to Disable Slack notifications for non-version branch builds

#11 @desrosj
4 years ago

Whoops, I didn't mean to assign as reviewing. But since I did, this looks OK to try to me. I'll commit it and test.

I'm also thinking of ways to advise committers to not push branches directly on the mirror because they will disappear. We can start with mentioning it in the handbook and go from there.

@jorbin
4 years ago

#12 @jorbin
4 years ago

Updated the regex a bit. See https://regex101.com/r/ba8tB2/1 as examples

#13 @desrosj
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 47901:

Build/Test Tools: Disable Travis CI build Slack notifications for non-official branches.

When branches are unintentionally pushed to the GitHub wordpress-develop mirror by committers, a build is triggered in Travis and the result is reported in Slack if the criteria defined is met.

Though this is not the desired workflow (any modifications made to the mirror are erased when the repository is synced from SVN), this can cause a lot of noise if several pushes are made and raise false alarms.

This change limits builds only to the master branch, and branches meeting the X.X pattern to match each branched version.

Props desrosj, jorbin, ocean90, TimothyBlynJacobs.
Fixes #50307.

#14 @desrosj
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#15 @desrosj
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 47905:

Build/Test Tools: Revert [47901].

This is not matching branches as desired.

Unrops desrosj, jorbin, ocean90, TimothyBlynJacobs.
Fixes #50307.

#16 @desrosj
4 years ago

  • Milestone WordPress.org deleted

I'm going to close this out as a wontfix. The regex in [47901] didn't work as expected. Thinking about it more, the chance this happens is relatively small.

Only committers can cause this to happen by pushing directly to the mirror. When this does happen, it could be a good red flag to the committer that they should be pushing to their fork instead. If they do not notice, another contributor can reach out with the suggestion. They won't actually lose their work because they should still retain a local copy of the branch.

If anyone feels strongly and has another idea for a solution, feel free to reopen and try it out.

Version 0, edited 4 years ago by desrosj (next)

#17 @ocean90
4 years ago

  • Resolution changed from fixed to wontfix
Note: See TracTickets for help on using tickets.