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

Reduce notification service delay for first scan in workflow scripts #530

Closed
wants to merge 1 commit into from

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Dec 11, 2024

PR Type

enhancement


Description

  • Reduced the notification service delay for the first scan from 7 minutes to 2 minutes.
  • Updated Slack workflows to use the reduced delay for the first scan.
  • Removed unused imports and cleaned up import statements for better readability in Jira, Slack, and Teams workflow scripts.

Changes walkthrough 📝

Relevant files
Enhancement
jira_workflows.py
Clean up imports in Jira workflows script                               

tests_scripts/workflows/jira_workflows.py

  • Removed unused import NOTIFICATIONS_SVC_DELAY_FIRST_SCAN.
  • Cleaned up imports for better readability.
  • +0/-2     
    slack_workflows.py
    Update Slack workflows to reduce first scan delay               

    tests_scripts/workflows/slack_workflows.py

  • Replaced NOTIFICATIONS_SVC_DELAY with
    NOTIFICATIONS_SVC_DELAY_FIRST_SCAN for first scan delay.
  • Removed unused import NOTIFICATIONS_SVC_DELAY.
  • Cleaned up imports for better readability.
  • +1/-3     
    teams_workflows.py
    Clean up imports in Teams workflows script                             

    tests_scripts/workflows/teams_workflows.py

  • Removed unused import NOTIFICATIONS_SVC_DELAY.
  • Cleaned up imports for better readability.
  • +0/-3     
    utils.py
    Reduce notification service delay for first scan                 

    tests_scripts/workflows/utils.py

  • Reduced NOTIFICATIONS_SVC_DELAY_FIRST_SCAN from 7 minutes to 2
    minutes.
  • +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Timing Validation
    Verify that 2 minutes delay is sufficient for the notification service to process and save the first scan results reliably across different environments and load conditions

    Configuration Change
    Validate that reducing NOTIFICATIONS_SVC_DELAY_FIRST_SCAN from 7 minutes to 2 minutes won't cause flaky tests or reliability issues in different test environments

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    A significantly reduced service delay might cause race conditions in notification processing

    The delay time for first scan notification has been significantly reduced from 7
    minutes to 2 minutes. This might be too aggressive and could lead to race conditions
    if the notification service needs more time to process and save the first scan.
    Consider validating this timing through extensive testing or implementing a polling
    mechanism instead of a fixed delay.

    tests_scripts/workflows/utils.py [16]

    -NOTIFICATIONS_SVC_DELAY_FIRST_SCAN = 2 * 60
    +NOTIFICATIONS_SVC_DELAY_FIRST_SCAN = 4 * 60  # 4 minutes delay for first scan
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The significant reduction in delay time from 7 minutes to 2 minutes could cause test flakiness and race conditions. The suggestion to increase the delay or implement a polling mechanism is crucial for test reliability.

    8
    General
    Missing error handling for scan creation can lead to unnecessary waiting time and unclear test failures

    The sleep delay is added after triggering the first scan but there's no error
    handling if the scan creation fails. Add error handling to ensure the scan was
    successfully initiated before waiting.

    tests_scripts/workflows/slack_workflows.py [96-97]

    -self.backend.create_kubescape_job_request(cluster_name=self.cluster, framework_list=[self.fw_name])
    -TestUtil.sleep(NOTIFICATIONS_SVC_DELAY_FIRST_SCAN, "waiting for first scan to be saved in notification service")
    +response = self.backend.create_kubescape_job_request(cluster_name=self.cluster, framework_list=[self.fw_name])
    +if response:
    +    TestUtil.sleep(NOTIFICATIONS_SVC_DELAY_FIRST_SCAN, "waiting for first scan to be saved in notification service")
    +else:
    +    raise RuntimeError("Failed to initiate first scan")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding error handling for the scan creation is important for test robustness and clearer failure messages, preventing unnecessary waiting time when the scan fails to initiate.

    7

    Copy link

    Failed to generate code suggestions for PR

    @kooomix kooomix closed this Jan 2, 2025
    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