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

Notify nightly failures #14049

Merged
merged 8 commits into from
May 29, 2024
Merged

Notify nightly failures #14049

merged 8 commits into from
May 29, 2024

Conversation

diemol
Copy link
Member

@diemol diemol commented May 29, 2024

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
    3 checks passed
    @diemol diemol deleted the notify-nightly-failures branch May 29, 2024 13:13
    Copy link
    Contributor

    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

    Copy link
    Contributor

    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

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

    Successfully merging this pull request may close these issues.

    1 participant