Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#52644 closed task (blessed) (fixed)

Configure Slack notifications for GitHub Action workflows

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

Description

Follow up to #50401.

TravisCI used to report changes in build status to Slack in the #core channel.

The following would be reported:

  • Every build that failed.
  • The first passing build in a branch after a failed build.

Some additional ideas for expanding this:

  • Posting every build result to the #core-firehose, or a new #core-ci-firehose

@helen had created GH-808 to explore setting this up.

Attachments (1)

Screen Shot 2021-08-04 at 3.16.05 pm.png (144.6 KB) - added by peterwilsoncc 3 years ago.

Download all attachments as: .zip

Change History (26)

This ticket was mentioned in Slack in #core by chaion07. View the logs.


4 years ago

#2 @peterwilsoncc
4 years ago

Hi @desrosj,

This was discussed in a 5.8 bug scrubbing session today.

As this doesn't touch production files, it was left on the milestone but feel free to change the milestone if you don't have bandwidth for this release cycle.

Thanks for your contributions to WordPress!

#3 @desrosj
4 years ago

  • Milestone changed from 5.8 to 5.9

I'm going to punt this to 5.9 as I won't have the time to address this in the weeks leading up to 5.8.

This ticket was mentioned in PR #1529 on WordPress/wordpress-develop by desrosj.


3 years ago
#4

  • Keywords has-patch added

#5 @desrosj
3 years ago

In 51511:

Build/Test Tools: Post a message to #core in Slack when a workflow fails.

This adds an additional step to each GitHub Action workflow file that posts a message to #core in Slack every time a workflow run fails.

A minor test and spacing change is included in this commit in order to that messages are posted correctly and will be reverted after testing.

See #52644.

#6 @desrosj
3 years ago

In 51512:

Build/Test Tools: Revert the test and coding standards changes in [51511].

These were temporary for testing Slack messages when GitHub Action workflows fail.

See #52644.

#7 follow-up: @desrosj
3 years ago

Looks like [51511] is doing the trick!

Some notes:

  • This will post every non-pull request workflow failure to #core.
  • Unfortunately, because this requires the job to actually run, a notification will not be posted if the job times out or fails due to a failure at the service level, such as a GitHub Action outage, or a runner that hangs and times out.
  • In Travis, there was a setting to post the first successful job following a failure to Slack. There is no straightforward way to accomplish this in GHA. It could be accomplished with a combination of REST API calls, but haven't figured out the best way for this yet.
  • We could also post all workflow results regardless of outcome to the #core-firehose.
  • We could also create a #core-ci channel that also receives a post for all workflow results regardless of outcome.
  • This will need to be backported to all branches so that Slack notifications are posted when old branches have failed workflow runs.

This ticket was mentioned in PR #1539 on WordPress/wordpress-develop by desrosj.


3 years ago
#8

Trac ticket:

#9 @desrosj
3 years ago

In 51535:

Build/Test Tools: Expand Slack notifications for GitHub Actions.

This expands Slack notifications to include success, cancelled, and “fixed” GitHub Action workflow run outcomes in addition to failures.

A “fixed” outcome occurs when the previous run for a workflow failed and the current one succeeds. This matches the behavior that was native to TravisCI by setting on_success for notifications to change.

The message details and where each outcome is posted is controlled by Slack workflows.

The Slack notification logic has also been pulled into a separate workflow to prevent repeating code in every workflow.

See #52644.

#10 @desrosj
3 years ago

In 51536:

Build/Test Tools: Correct invalid JSON in Slack payload.

The outer marks are not required and produce invalid JSON.

See #52644.

#11 @desrosj
3 years ago

In 51537:

Build/Test Tools: Revert changes only included for testing purposes.

Follow up to [51535-51536].

See #52644.

#12 in reply to: ↑ 7 @desrosj
3 years ago

Some revised notes after [51535-51537]:

  • Slack notifications are now controlled by a separate workflow that runs on the workflow_run event when a listed workflow is completed. I took this route to prevent repeating large amounts of code across all workflows where Slack notifications are desired. But ideally, this workflow would be published as an action that could be defined in the uses: for a step.
  • The first successful job following a failure will now be posted as a "fixed" notice to #core. There are some potential edge cases with the current implementation where re-running workflows could result in the wrong workflow run being used for comparison.
  • All workflow results regardless of outcome are posted to to the #core-firehose.
  • All workflow results regardless of outcome are posted to the new #core-ci-firehose channel.
  • Since these notifications are controlled in a separate workflow initiated on the workflow_run event, this does not technically need to be backported (all workflow_run events are created against the primary branch). But, [51511-51512,51535-51537] could be backported just to have consistent contents of the .github/workflows directory.
  • The messages themselves are configured within Slack Workflows.

I'm going to leave this open in case any adjustments are needed.

#13 @peterwilsoncc
3 years ago

Cool screenshot, peterwilsoncc.

Everyone else can ignore it as I've found the setting in Slack to prevent actions expanding -- and used web developer tools to read the truncated tex

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#15 @desrosj
3 years ago

In 51555:

Build/Test Tools: Correctly check for the trigger event when running the Slack notifications workflow.

This updates the conditional check for the Slack notifications workflow to check the event of the original workflow and not the one requested through the workflow_run:completed event.

The run triggering the event is accessible at github.event.workflow_run.

Props Clorith.
See #52644.

desrosj commented on PR #1539:


3 years ago
#16

This was merged into Core in https://core.trac.wordpress.org/changeset/51511.

#17 @desrosj
3 years ago

In 51558:

Build/Test Tools: Add branch filtering for Slack notifications workflow.

Since branch filtering happens prior to the workflow run being created, filtering the branches that workflow_run will fire on for this workflow should cut down on the number of skipped “Slack Notifications” runs listed in the Actions section of the repository.

This also removes the check for a skipped outcome in the requesting workflow. Workflows for push events resulting from WordPress Core commits are never skipped.

See #52644.

#18 @desrosj
3 years ago

In 51647:

Build/Test Tools: Include the commit short summary in Slack messages.

This gives more context to GitHub Action related Slack messages without the need to click the workflow URL, or unfurl the links (which shows lots of unnecessary information, such as the repository description and image).

Props earnjam.
See #52644.

#19 @desrosj
3 years ago

It looks like in some situations, null is being found for the previous conclusion. See https://github.com/WordPress/wordpress-develop/runs/3436640677?check_suite_focus=true#step:3:2.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 years ago

#21 @johnbillion
3 years ago

Sergey spotted that the commit message in the failure message posted to Slack has content between backticks stripped.

A cursory look suggests this could be the culprit: https://github.com/johnbillion/wordpress-develop/blob/00d248e699deb0b4336010316b5feb2763de8e87/.github/workflows/slack-notifications.yml#L70

#22 @desrosj
3 years ago

In 51679:

Build/Test Tools: Preserve text within backticks in Slack notifications.

Props SergeyBiryukov, johnbillion, earnjam.
See #52644.

#23 @desrosj
3 years ago

Looks like " within the first line of the commit are not being double escaped causing them to be interpreted incorrectly and producing invalid JSON. See https://github.com/WordPress/wordpress-develop/runs/3478978811?check_suite_focus=true#step:2:2.

#24 @desrosj
3 years ago

In 51709:

Build/Test Tools: Double escape quotation marks() for Slack “messages”.

This prevents quotation marks from producing invalid JSON errors.

See #52644.

#25 @desrosj
3 years ago

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

I'd forgotten that this more specific ticket was still open. Here are some additional changesets that are related to Slack notifications:

[51921-51922,51924-51925,51934,51937,51952-51954,52002].

I think this is pretty stable for now, so going to close this out.

One note for context going forward. The changes in [51921] and up making the Slack workflow a callable one is nice in several ways, but mainly in that it eliminates all of the additional "Slack Notifications" workflows that run for older branches. This creates quite a bit of noise, and I'd like to backport these changes to older branches once this gets a bit more testing.

One great thing about this is we can call the workflow from the primary branch, so any needed adjustments will not need to be backported after the initial change.

Note: See TracTickets for help on using tickets.