ci: send slack message on failed performance tests #1598
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to the Slack notification system related to GitHub pipeline statuses. It updates the JSON structure in Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci-cd-yt01.yml (1)
Line range hint
143-173: LGTM! Consider adjusting test duration for more reliable results.The performance test job implementation looks good with:
- Appropriate dependencies on infrastructure and app deployments
- Matrix strategy for running multiple test files
- Disabled fail-fast for comprehensive test results
However, the test duration of 30s might be too short for reliable performance metrics.
Consider increasing the duration to at least 2-3 minutes to account for warm-up periods and get more stable metrics:
- duration: 30s + duration: 180s
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/slack-templates/pipeline-failed.json(1 hunks).github/workflows/ci-cd-yt01.yml(3 hunks).github/workflows/workflow-send-ci-cd-status-slack-message.yml(3 hunks)
🔇 Additional comments (3)
.github/workflows/ci-cd-yt01.yml (1)
178-178: LGTM! Proper integration of performance tests status.
The changes correctly integrate performance test results into the notification system by:
- Adding the job dependency
- Passing the test status to the notification workflow
Also applies to: 187-187
.github/workflows/workflow-send-ci-cd-status-slack-message.yml (2)
25-28: LGTM! Well-defined input parameter.
The performance_tests_status input parameter is properly defined with:
- Consistent default value
- Clear description
- Correct type specification
76-76: LGTM! Consistent status handling implementation.
The changes properly integrate performance test status into the emoji determination and environment variables setup.
Also applies to: 93-93
| @@ -23,7 +23,7 @@ | |||
| "type": "section", | |||
| "text": { | |||
| "type": "mrkdwn", | |||
| "text": "*Job Status:*\n• Infrastructure: ${{ env.INFRA_STATUS }}\n• Apps: ${{ env.APPS_STATUS }}\n• Slack Notifier: ${{ env.SLACK_NOTIFIER_STATUS }}\n• E2E Tests: ${{ env.E2E_TESTS_STATUS }}\n• Schema NPM: ${{ env.SCHEMA_NPM_STATUS }}\n• Publish: ${{ env.PUBLISH_STATUS }}" | |||
| "text": "*Job Status:*\n• Infrastructure: ${{ env.INFRA_STATUS }}\n• Apps: ${{ env.APPS_STATUS }}\n• Slack Notifier: ${{ env.SLACK_NOTIFIER_STATUS }}\n• E2E Tests: ${{ env.E2E_TESTS_STATUS }}\n• Performance Tests: ${{ env.PERFORMANCE_TESTS_STATUS }}\n• Schema NPM: ${{ env.SCHEMA_NPM_STATUS }}\n• Publish: ${{ env.PUBLISH_STATUS }}" | |||
There was a problem hiding this comment.
💡 Codebase verification
Consider splitting the status message into columns for better readability
The status message line length (123 characters) exceeds Slack's optimal line length of 100 characters, which could cause wrapping and affect readability. Consider reformatting the message into two columns to improve the layout:
*Job Status:*
• Infrastructure: ${{ env.INFRA_STATUS }} • Schema NPM: ${{ env.SCHEMA_NPM_STATUS }}
• Apps: ${{ env.APPS_STATUS }} • Publish: ${{ env.PUBLISH_STATUS }}
• Slack Notifier: ${{ env.SLACK_NOTIFIER_STATUS }}
• E2E Tests: ${{ env.E2E_TESTS_STATUS }}
• Performance Tests: ${{ env.PERFORMANCE_TESTS_STATUS }}
🔗 Analysis chain
LGTM! Verify message formatting in Slack.
The Performance Tests status is properly integrated into the Job Status section. However, it would be good to verify the message formatting in Slack to ensure readability with the additional status line.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Slack message formatting by checking the line length
# Test: Check if the message text length might cause wrapping issues in Slack
# Get the line length
echo "Line length of the status text:"
echo "*Job Status:*\n• Infrastructure: ✓\n• Apps: ✓\n• Slack Notifier: ✓\n• E2E Tests: ✓\n• Performance Tests: ✓\n• Schema NPM: ✓\n• Publish: ✓" | wc -L
# Slack's message formatting typically starts to wrap around 100 characters
echo "Note: If the line length exceeds 100 characters, consider reformatting for better readability in Slack"
Length of output: 446
|
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)