CI: Notify when CircleCI build failures for master and rel branches only

[michaeldiamant]

Suggests the following build notification failure policy modifications: Notify when release + master branches fail.

Rationale:

• Merges to master sometimes fail the build. The failure is silent until the nightly build happens.
• Prior to the PR, ad-hoc full builds triggered by a user result in a failure notification. The PR deliberately precludes custom branches from generating failure notifications.
  • If we wish to retain notifications here, I propose adding a secondary channel out-of-band to the PR.

Notes:

• PR bumps cricleci/slack orb. The associated commits and releases seem harmless: CircleCI-Public/slack-orb@v4.4.2...v4.10.1.
• PR moves notification into post-steps (https://circleci.com/docs/configuration-reference/#pre-steps-and-post-steps).
  • Rationale: Using a post-step feels to me like a cleaner separation of concerns, and hopefully reduces the chance of missing a notification on new workflow jobs.
  • For reference, here's an article I referred to while working on the PR: https://support.circleci.com/hc/en-us/articles/360047082992-How-to-send-a-slack-notification-at-end-of-workflow.
• Out-of-band to the PR, I can imagine moving all Slack notification into Actions via https://circleci.com/blog/trigger-circleci-pipeline-github-action/. It'd provide a consistent way to notify on failure. It's sufficiently involved that I'd prefer to ticket the concern rather than address now.

[codecov]

Codecov Report

Merging #4789 (fc1e458) into master (1a13168) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4789      +/-   ##
==========================================
- Coverage   54.71%   54.69%   -0.03%     
==========================================
  Files         414      414              
  Lines       53548    53548              
==========================================
- Hits        29299    29286      -13     
- Misses      21825    21835      +10     
- Partials     2424     2427       +3     

Impacted Files	Coverage Δ
crypto/merkletrie/trie.go	66.42% <0.00%> (-2.19%)	⬇️
network/wsPeer.go	66.50% <0.00%> (-1.95%)	⬇️
crypto/merkletrie/node.go	91.62% <0.00%> (-1.87%)	⬇️
ledger/tracker.go	77.87% <0.00%> (-0.86%)	⬇️
ledger/acctonline.go	78.12% <0.00%> (-0.53%)	⬇️
network/wsNetwork.go	65.62% <0.00%> (-0.19%)	⬇️
ledger/accountdb.go	72.55% <0.00%> (-0.08%)	⬇️
data/transactions/verify/txn.go	76.19% <0.00%> (ø)
ledger/acctupdates.go	69.51% <0.00%> (+0.24%)	⬆️
catchup/service.go	69.62% <0.00%> (+0.74%)	⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[michaeldiamant]

#4790 illustrates notification on failure test. See the notification channel at ~5:15PM ET for failure notifications from branch = notify_on_failure_test.

[winder]

You're adding slack contexts to more jobs. Is that going to make it more difficult to run nightly tests against forks?

[michaeldiamant]

Is that going to make it more difficult to run nightly tests against forks?

@winder Without testing it, it's unclear to me. Depends on when the filter clause is invoked.

• At worst, it means commenting out additional secrets.
• At best, it means no change to the existing workflow.

From my perspective, the PR remains worth pursuing even in the worst case scenario.

[gmalouf]

You're adding slack contexts to more jobs. Is that going to make it more difficult to run nightly tests against forks?

@winder wouldn't they not run if outside of master or rel branches?

[gmalouf]

#4790 illustrates notification on failure test. See the notification channel at ~5:15PM ET for failure notifications from branch = notify_on_failure_test.

Looked spammy in the dev-alerts channel - has that been addressed/corrected (as-is, did not seem useful)

[michaeldiamant]

Looked spammy

@gmalouf No - no attempt made to modify behavior.

• It's not a change in behavior from the nightly build.
  • If a nightly build fails to compile, it generates multiple notifications. The likelihood of merging a PR with compilation issues feels slim (happened 0 times in ~10 months).
  • I agree if a build error happens that it's too noisy. Sadly, I don't see an alternative with the toolset CircleCI provides. I'd be happy to be proven wrong.
• I think 1 way to improve behavior is to move notifications to GHA as discussed in PR description. I can ticket the concern though I don't see a reason to hold here.

[cce]

If you only want one notification on failure, you should be able to follow this advice and make a fan-in job that requires all the other jobs: https://support.circleci.com/hc/en-us/articles/360047082992-How-to-send-a-slack-notification-at-end-of-workflow

However if your workflow jobs don't run sequentially, you will need to create a new job that contains the slack notification and require that the job has run all the previous jobs.

[michaeldiamant]

make a fan-in job that requires all the other jobs:

@cce I had considered that article while working on the PR and concluded the approach only works for success notifications. Admittedly, I didn't try the approach, but looking at a sample failed workflow (https://app.circleci.com/pipelines/github/algorand/go-algorand/10598/workflows/95522810-15f8-4f97-8dc2-9c4a8e421535), we see jobs downstream from failed jobs (e.g. arm64_test_verification) are not invoked.