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.

Sign up for GitHub

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

Jump to bottom

Post error test results to Slack #4404

Merged
merged 15 commits into from
Feb 26, 2025
Merged

Post error test results to Slack #4404

merged 15 commits into from
Feb 26, 2025

Conversation

tonidero
Copy link
Contributor

@tonidero tonidero commented Oct 24, 2024 •
edited
Loading

Description

This adds a slack notification to all CircleCI jobs that error out. Currently posts to the #feed-circleci-ios-failures

vegaro
vegaro reviewed Oct 30, 2024
View reviewed changes
@tonidero tonidero force-pushed the post-test-results-slack branch from cb2b1d2 to fe6bc4d Compare February 4, 2025 08:31
@tonidero tonidero marked this pull request as ready for review February 4, 2025 08:37
@tonidero tonidero requested review from a team and vegaro February 4, 2025 08:37
@tonidero
Copy link
Contributor Author

tonidero commented Feb 4, 2025

So I believe this might be quite noisy, but it's working for now so I thought we can merge and tweak as needed.

vegaro
vegaro reviewed Feb 4, 2025
View reviewed changes
vegaro
vegaro requested changes Feb 5, 2025
View reviewed changes
.circleci/config.yml Outdated
@@ -461,6 +472,7 @@ jobs:
- run:
name: Check pods and deployment targets
command: bundle exec fastlane check_pods
- slack-notify
Copy link
Contributor

Choose a reason for hiding this comment

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

This also runs on PRs, we don't need it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm pod-lib-lint is executed also in main right? I'm planning to only remove the ones for jobs that are not supposed to run on main never

.circleci/config.yml Outdated
@@ -1080,6 +1111,7 @@ jobs:
- run:
name: Prepare next version
command: bundle exec fastlane prepare_next_version
- slack-notify
Copy link
Contributor

Choose a reason for hiding this comment

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

I love we are going to get notified by this

.circleci/config.yml Outdated
@@ -1188,6 +1224,7 @@ jobs:
path: fastlane/test_output
- store_artifacts:
path: fastlane/test_output
- slack-notify
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't run this on lint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually runs on main. I think the chances of merging something breaking this would be low, but better to keep it. Lmk if you think otherwise!

@tonidero tonidero force-pushed the post-test-results-slack branch from e04cde2 to cd3c29a Compare February 5, 2025 11:51
@tonidero tonidero requested a review from vegaro February 5, 2025 11:53
JayShortway
JayShortway reviewed Feb 6, 2025
View reviewed changes
Copy link
Member

@JayShortway JayShortway left a comment

Choose a reason for hiding this comment

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

Instead of adding a slack-notify step to every job, can we add a slack-notify job to certain workflows? That way we can more easily make it notify on main only.

We can make the job only run when the test job failed, for instance like so:

- slack-notify:
    requires:
       - lint: failed

https://circleci.com/docs/configuration-reference/#requires

@tonidero
Copy link
Contributor Author

tonidero commented Feb 6, 2025

Hmm that's interesting... I can try that, thanks! I guess we might lose the data of what jobs failed in Slack though, not sure if it's that important though.

@tonidero
Copy link
Contributor Author

tonidero commented Feb 14, 2025 •
edited
Loading

So I was checking that alternative, however, I don't think Slack's orb officially supports that use case... The orb docs have a use case that looks like the current solution: https://circleci.com/developer/orbs/orb/circleci/slack#usage-notify_on_fail_with_template

I saw this: https://support.circleci.com/hc/en-us/articles/360047082992-How-to-send-a-slack-notification-at-end-of-workflow, but that would mean we need to add a job to run on failures as a post_step on each job. The only advantage of that would be that we could call this from the workflow instead of inside each job, but I don't know if it's worth going against the recommendations from the Slack orb for this. Wdyt? @JayShortway

Well, I think we can make it, only that it won't be as personalized, as I mentioned in my previous comment. Nvm then!

@tonidero tonidero force-pushed the post-test-results-slack branch from cd3c29a to 8edc6c2 Compare February 14, 2025 15:13
@tonidero
Post error test results to Slack
0d1473b
@tonidero
To revert: Add linter error
2c73fe1
@tonidero
Test different context
ef4dcdb
@tonidero
Revert "To revert: Add linter error"
5a38eb0 
This reverts commit 2177c48.
@tonidero
Post only on main
da92632
@tonidero
Remove notify from danger job
b34eeb1
@tonidero
To revert: testing branch specific notifications
bcf5234
@tonidero
Revert "To revert: testing branch specific notifications"
4d373da 
This reverts commit 5602857.
@tonidero
Only run slack-notify command on jobs running in main
c87bfc9
@tonidero
Change to have a separate job for failures instead of running as a st…
0fe54a4 
…ep on each job
@tonidero
Reapply "To revert: Add linter error"
5c3eef4 
This reverts commit 2a40047.
@tonidero
Revert "Reapply "To revert: Add linter error""
57a5819 
This reverts commit 5c3eef4.
@tonidero
Revert "Change to have a separate job for failures instead of running…
c63d92c 
… as a step on each job"

This reverts commit 0fe54a4.
@tonidero tonidero force-pushed the post-test-results-slack branch from 8edc6c2 to c63d92c Compare February 25, 2025 14:53
@tonidero
Copy link
Contributor Author

An update here. We haven't had time to dedicate to this yet, so we're planning on merging the approach of a notification as a step of each job, instead of a separate job in the workflow for now. We can revisit this later, when we can.

@tonidero tonidero requested review from JayShortway and a team February 25, 2025 15:11
JayShortway
JayShortway approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@JayShortway JayShortway left a comment

Choose a reason for hiding this comment

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

Great to have this! Just a naming suggestion.

@tonidero tonidero enabled auto-merge (squash) February 26, 2025 07:51
@tonidero tonidero merged commit b5e1fcb into main Feb 26, 2025
9 of 10 checks passed
@tonidero tonidero deleted the post-test-results-slack branch February 26, 2025 08:06
@RCGitBot RCGitBot mentioned this pull request Feb 26, 2025
Closed
@RCGitBot RCGitBot mentioned this pull request Mar 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vegaro vegaro vegaro approved these changes

@ajpallares ajpallares ajpallares approved these changes

@JayShortway JayShortway JayShortway approved these changes

Assignees
No one assigned
Labels
pr:other
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tonidero @vegaro @ajpallares @JayShortway