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.

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

Post error test results to Slack #4404

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

tonidero
Copy link
Contributor

@tonidero tonidero commented Oct 24, 2024

Description

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

@@ -411,6 +417,7 @@ jobs:
name: SPM Release Build
command: swift build -c release --target RevenueCat
no_output_timeout: 30m
- 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 would generally prefer these to be part of fastlane, but I think in this case, since there's an orb that gets updates and automatically post failures (right?), I like it here

- api-tests
- backend-integration-tests-SK1:
context:
- slack-secrets-ios
Copy link
Contributor

Choose a reason for hiding this comment

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

to bad there is not a way to pass this to the workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I tried but no luck 😢. It's one of the things I liked the least of this approach...

@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.

.circleci/config.yml Outdated Show resolved Hide resolved
@@ -1313,6 +1353,7 @@ jobs:
git remote set-url origin https://github.com/RevenueCat/purchases-ios-spm.git
git push origin
git push --tags
- 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.

deploy-to-spm runs on PRs so we don't want it here or it will be noisy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right... I actually added a branch pattern parameter to main in: https://github.com/RevenueCat/purchases-ios/pull/4404/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47R98. This way we won't be notified in branches. While I agree this job wouldn't be called in main, I thought it might be better to have it in all jobs so we do remember to have this as a step on all future jobs as well...

Wdyt? I guess I can remove it from all jobs that won't be run on main to be more specific...

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I see... I missed that

hmmmmm. I think it would keep the config.yml cleaner to remove it from the ones that we are not running in main tbh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will remove those 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did those changes in: cd3c29a

@@ -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

@@ -590,6 +607,7 @@ jobs:
- store_artifacts:
path: fastlane/test_output
destination: scan-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.

This also runs on every PR

@@ -623,6 +641,7 @@ jobs:
- store_artifacts:
path: fastlane/test_output
destination: scan-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.

same about running on every PR

@@ -698,7 +718,7 @@ jobs:
- store_artifacts:
path: fastlane/test_output/xctest
destination: scan-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.

this also runs on every PR

@@ -1056,6 +1086,7 @@ jobs:
name: Deploy new version
command: bundle exec fastlane push_revenuecat_pod
no_output_timeout: 30m
- 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.

Also runs on PRs

@@ -1068,7 +1099,7 @@ jobs:
name: Deploy new version
command: bundle exec fastlane push_revenuecatui_pod
no_output_timeout: 30m

- 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.

Also runs on PRs

@@ -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

@@ -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!

@@ -1214,7 +1250,7 @@ jobs:
- run:
name: Tag branch
command: bundle exec fastlane tag_current_branch

- 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.

same

@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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants