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

adding kdr slack and teams tests #436

Merged
merged 6 commits into from
Aug 5, 2024
Merged

adding kdr slack and teams tests #436

merged 6 commits into from
Aug 5, 2024

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Aug 5, 2024

PR Type

Tests, Enhancement


Description

  • Added new test configurations for Slack and Teams alerts in runtime_tests.py.
  • Updated default paths for notifications and KDR configurations in statics.py.
  • Introduced ResourceFieldEncoder and verify_application_profiles in base_helm.py.
  • Created IncidentsAlerts class for handling incident alerts and enriching notifications in alerts.py.
  • Removed redundant ResourceFieldEncoder and verify_application_profiles in incidents.py.
  • Updated policy validation to exclude notifications in policies.py.
  • Enhanced alert channel creation and Kubescape installation in alert_notifications.py.

Changes walkthrough 📝

Relevant files
Tests
runtime_tests.py
Add Slack and Teams alert test configurations                       

configurations/system/tests_cases/runtime_tests.py

  • Added new test configurations for Slack and Teams alerts.
  • Updated import statements to include new alert notification functions.
  • Introduced kdr_teams_alerts and kdr_slack_alerts methods.
  • +30/-3   
    Enhancement
    statics.py
    Update default paths for notifications and KDR configurations

    systest_utils/statics.py

  • Added default paths for KDR configurations.
  • Updated notifications paths.
  • +5/-0     
    base_helm.py
    Add ResourceFieldEncoder and application profiles verification

    tests_scripts/helm/base_helm.py

  • Added ResourceFieldEncoder class for JSON encoding.
  • Introduced verify_application_profiles method.
  • +30/-0   
    alerts.py
    Implement incident alerts handling and notification enrichment

    tests_scripts/runtime/alerts.py

  • Created new IncidentsAlerts class for handling incident alerts.
  • Added methods for enriching alert notifications for Slack and Teams.
  • Implemented alert notification and runtime policy creation logic.
  • +164/-0 
    incidents.py
    Remove redundant ResourceFieldEncoder and application profiles
    verification

    tests_scripts/runtime/incidents.py

  • Removed ResourceFieldEncoder class.
  • Removed verify_application_profiles method.
  • +0/-24   
    policies.py
    Update policy validation to exclude notifications               

    tests_scripts/runtime/policies.py

  • Updated validate_new_policy method to remove notifications from
    properties check.
  • +2/-2     
    alert_notifications.py
    Enhance alert channel creation and Kubescape installation

    tests_scripts/users_notifications/alert_notifications.py

  • Added optional parameters to create_alert_channel and
    get_alert_channel_payload methods.
  • Modified install_kubescape method to accept helm_kwargs.
  • Added condition to check fw_name before deleting custom framework.
  • +9/-7     

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Duplication
    The methods kdr_teams_alerts and kdr_slack_alerts in runtime_tests.py share a lot of similar code, especially in the configuration setup. Consider refactoring to reduce duplication and improve maintainability.

    Error Handling
    The method verify_application_profiles in base_helm.py uses assertions for error handling which might not be suitable for production code. Consider using more robust error handling mechanisms.

    Hardcoded Values
    The IncidentsAlerts class in alerts.py contains hardcoded values within the helm_kwargs dictionary. These should be configurable externally to allow for flexibility in different environments.

    Copy link

    github-actions bot commented Aug 5, 2024

    Preparing PR description...

    Copy link

    codiumai-pr-agent-free bot commented Aug 5, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Ensure all variables are defined to prevent runtime errors

    There is a potential issue with an undefined variable DEFAULT_K8S_PATHS used in the
    os.path.join function. Ensure that DEFAULT_K8S_PATHS is defined or imported
    correctly in this file.

    systest_utils/statics.py [76]

    +# Ensure DEFAULT_K8S_PATHS is defined or corrected as needed
     DEFAULT_KDR_DEPLOYMENT_PATH = os.path.join(DEFAULT_K8S_PATHS, 'deployments')
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a potential bug that could cause runtime errors if DEFAULT_K8S_PATHS is not defined or imported correctly, which is crucial for the functionality of the code.

    9
    Enhancement
    Implement serialization logic within the ResourceField class for better encapsulation and clarity

    To ensure that the ResourceField objects are serialized correctly, it's better to
    handle the serialization logic within the ResourceField class itself by implementing
    the repr or str method, rather than using a custom encoder.

    tests_scripts/helm/base_helm.py [19-23]

    -class ResourceFieldEncoder(json.JSONEncoder):
    -    def default(self, obj):
    -        if isinstance(obj, ResourceField):
    -            return obj.__dict__
    -        return json.JSONEncoder.default(self, obj)
    +class ResourceField:
    +    def __repr__(self):
    +        return str(self.__dict__)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion enhances the code by encapsulating the serialization logic within the ResourceField class, improving clarity and maintainability. This is a good practice for object-oriented design.

    8
    Maintainability
    Use dictionary unpacking for clearer and more maintainable test configuration setup

    To improve the readability and maintainability of the test configuration, consider
    using a dictionary unpacking approach for the TestConfiguration parameters instead
    of setting them directly in the constructor.

    configurations/system/tests_cases/runtime_tests.py [34-41]

    -return TestConfiguration(
    -    name=inspect.currentframe().f_code.co_name,
    -    test_obj=IncidentsAlerts,
    -    deployments=join(DEFAULT_DEPLOYMENT_PATH, "redis_sleep_long"),
    -    getMessagesFunc=get_messages_from_teams_channel,
    -    enrichAlertChannelFunc=enrich_teams_alert_notifications,
    -)
    +config_params = {
    +    "name": inspect.currentframe().f_code.co_name,
    +    "test_obj": IncidentsAlerts,
    +    "deployments": os.path.join(DEFAULT_DEPLOYMENT_PATH, "redis_sleep_long"),
    +    "getMessagesFunc": get_messages_from_teams_channel,
    +    "enrichAlertChannelFunc": enrich_teams_alert_notifications,
    +}
    +return TestConfiguration(**config_params)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves readability and maintainability by using dictionary unpacking, which makes the code cleaner and easier to manage. However, it does not address a critical issue.

    7

    Copy link

    github-actions bot commented Aug 5, 2024

    Preparing review...

    "config-service"
    ],
    "description": "Test kdr incidents is being sent to slack",
    "skip_on_environment": "",
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    R U sure you want them to run on PROD?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    For now yes

    @kooomix kooomix merged commit c0aa079 into master Aug 5, 2024
    3 checks passed
    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.

    2 participants