WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 18 months ago

Last modified 18 months ago

#50307 closed enhancement (wontfix)

Disable Slack notifications for non-version branch builds

Reported by: desrosj Owned by: 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 18 months ago.
50307.2.diff (960 bytes) - added by jorbin 18 months ago.
50307.3.diff (990 bytes) - added by jorbin 18 months ago.
50307.4.diff (992 bytes) - added by jorbin 18 months ago.

Download all attachments as: .zip

Change History (21)

#1 follow-up: @ocean90
18 months 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
18 months ago

#2 in reply to: ↑ 1 @desrosj
18 months 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
18 months 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
18 months 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
18 months 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
18 months 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
18 months ago

whomp whomp

@jorbin
18 months ago

#9 @ocean90
18 months ago

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

@jorbin
18 months ago

#10 @desrosj
18 months 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
18 months 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
18 months ago

#12 @jorbin
18 months ago

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

#13 @desrosj
18 months 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
18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#15 @desrosj
18 months 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
18 months 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 18 months ago by desrosj (next)

#17 @ocean90
18 months ago

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