Skip to content

Notify nightly failures#14049

Merged
diemol merged 8 commits intotrunkfrom
notify-nightly-failures
May 29, 2024
Merged

Notify nightly failures#14049
diemol merged 8 commits intotrunkfrom
notify-nightly-failures

Conversation

@diemol
Copy link
Member

@diemol diemol commented May 29, 2024 •
edited by qodo-code-review bot
Loading

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement, configuration changes

Description

  • Added new jobs to handle failures for Ruby, Python, Java, .NET, Grid, and JavaScript in the .github/workflows/nightly.yml file.
  • Configured Slack notifications for each failure job using the rtCamp/action-slack-notify@v2 action.
  • Set environment variables for Slack notifications including SLACK_ICON_EMOJI, SLACK_COLOR, SLACK_CHANNEL, SLACK_USERNAME, SLACK_TITLE, MSG_MINIMAL, and SLACK_WEBHOOK.

Changes walkthrough 📝

Relevant files
Configuration changes
nightly.yml
Add failure handling and Slack notifications for nightly builds.

.github/workflows/nightly.yml

  • Added new jobs to handle failures for Ruby, Python, Java, .NET, Grid,
    and JavaScript.
  • Configured Slack notifications for each failure job.
  • Set environment variables for Slack notifications including
    SLACK_ICON_EMOJI, SLACK_COLOR, SLACK_CHANNEL, SLACK_USERNAME,
    SLACK_TITLE, MSG_MINIMAL, and SLACK_WEBHOOK.
    		•  +102/-4 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @diemol diemol merged commit 92f3240 into trunk May 29, 2024
    @diemol diemol deleted the notify-nightly-failures branch May 29, 2024 13:13
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented May 29, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are primarily additions of new jobs to an existing GitHub Actions workflow. Each job follows a similar pattern, which simplifies the review process. However, the reviewer needs to ensure that the conditions and environment variables are correctly set up for each job.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The SLACK_COLOR environment variable uses needs.<job>.status which might not provide meaningful color codes for Slack. Typically, Slack expects specific color codes or names (e.g., 'danger', 'good', 'warning'), and it's unclear if the status outputs from GitHub Actions jobs align with these expectations.

    🔒 Security concerns

    No

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented May 29, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Use a reusable workflow or composite action to avoid duplicating Slack notification steps

    Use a reusable workflow or composite action to avoid duplicating the Slack notification
    steps across multiple failure jobs.

    .github/workflows/nightly.yml [48-58]

     - name: Slack Notification
-  uses: rtCamp/action-slack-notify@v2
-  env:
-    SLACK_ICON_EMOJI: ":rotating_light:"
-    SLACK_COLOR: ${{ needs.ruby.status }}
-    SLACK_CHANNEL: selenium-tlc
-    SLACK_USERNAME: GitHub Workflows
-    SLACK_TITLE: Nightly Ruby ${{ needs.ruby.result }}
-    MSG_MINIMAL: actions url
-    SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK_URL }}
+  uses: ./.github/actions/slack-notify
+  with:
+    status: ${{ needs.ruby.status }}
+    result: ${{ needs.ruby.result }}
    Suggestion importance[1-10]: 8

    Why: Using a reusable workflow or composite action for Slack notifications would significantly reduce redundancy and improve maintainability across multiple jobs.

    		8
    Possible issue
    Ensure the SLACK_WEBHOOK environment variable is always set to avoid potential issues

    To avoid potential issues with missing secrets, ensure that the SLACK_WEBHOOK environment
    variable is always set by adding a default value or a check before using it.

    .github/workflows/nightly.yml [58]

    -SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK_URL }}
+SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK_URL || 'default_webhook_url' }}
    Suggestion importance[1-10]: 7

    Why: Ensuring that the SLACK_WEBHOOK is always set can prevent failures due to missing configuration, which is crucial for the reliability of the notification system.

    		7
    Debugging
    Log the failure reason to the console before sending the Slack notification

    Add a step to log the failure reason or error message to the console before sending the
    Slack notification for easier debugging.

    .github/workflows/nightly.yml [48-50]

    +- name: Log Failure Reason
+  run: echo "Failure reason: ${{ needs.ruby.outputs.failure_reason }}"
 - name: Slack Notification
   uses: rtCamp/action-slack-notify@v2
    Suggestion importance[1-10]: 7

    Why: Logging the failure reason before sending a Slack notification would aid in debugging and provide immediate context, which is beneficial for quick issue resolution.

    		7
    Enhancement
    Add job name or ID to the Slack notification title for better context

    Consider adding a step to include the job name or ID in the Slack notification message to
    provide more context about which job failed.

    .github/workflows/nightly.yml [56]

    -SLACK_TITLE: Nightly Ruby ${{ needs.ruby.result }}
+SLACK_TITLE: Nightly Ruby ${{ needs.ruby.result }} - Job ${{ github.job }}
    Suggestion importance[1-10]: 6

    Why: Adding job name or ID to the Slack notification title would provide more context about the failure, enhancing the utility of the notification.

    		6

    @seedysoft seedysoft mentioned this pull request Jun 21, 2024
    Merged
    @MiguelPelegrina MiguelPelegrina mentioned this pull request Jul 12, 2024
    Merged
    @aravindb26 aravindb26 mentioned this pull request Jul 16, 2024
    Open
    @x-way x-way mentioned this pull request Jul 24, 2024
    Closed
    @x-way x-way mentioned this pull request Aug 10, 2024
    Closed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    @diemol @sandeepsuryaprasad
    Notify nightly failures (SeleniumHQ#14049)
    6e01eed 
    * Sending notification when workflow fails

* Making the job fail

* Adding name and fixing "needs"

* Removing unneeded SLACKIFY_MARKDOWN

* Making it fail

* Setting icon

* Making it work again

* Rotating light for all
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @diemol