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

refactor cleanup methods in workflows to improve error handling and c… #577

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Jan 9, 2025

User description

…ode organization


PR Type

Enhancement, Bug fix


Description

  • Refactored cleanup methods for better error handling.

  • Introduced cleanup_workflows method for reusable cleanup logic.

  • Improved logging for exceptions during cleanup operations.

  • Enhanced code organization in workflow-related classes.


Changes walkthrough 📝

Relevant files
Enhancement
teams_workflows.py
Improved error handling in `teams_workflows` cleanup method

tests_scripts/workflows/teams_workflows.py

  • Added call to cleanup_workflows in cleanup method.
  • Wrapped delete_channel_by_guid in a try-except block for error
    handling.
  • Enhanced logging for exceptions during channel deletion.
  • +5/-1     
    workflows.py
    Refactored workflow cleanup logic into reusable method     

    tests_scripts/workflows/workflows.py

  • Extracted cleanup_workflows method for reusable cleanup logic.
  • Replaced inline cleanup logic with cleanup_workflows in cleanup.
  • Improved logging for exceptions during workflow deletion.
  • +9/-6     

    💡 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 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The error handling in cleanup() silently continues after logging channel deletion errors. Consider whether errors should propagate up or if cleanup should be more resilient.

        self.delete_channel_by_guid(self.get_channel_guid_by_name(self.webhook_name))
    except Exception as e:
        Logger.logger.error(f"Failed to delete channel with name {self.webhook_name}, got exception {e}")

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Improve cleanup sequence to prevent potential dependency conflicts

    Call cleanup_workflows after the framework deletion to ensure proper cleanup order,
    as framework deletion might depend on workflow state.

    tests_scripts/workflows/teams_workflows.py [136-144]

     def cleanup(self, **kwargs):
    -    super().cleanup_workflows()
         if self.webhook_name:
             try:
                 self.delete_channel_by_guid(self.get_channel_guid_by_name(self.webhook_name))
             except Exception as e:
                 Logger.logger.error(f"Failed to delete channel with name {self.webhook_name}, got exception {e}")
         if self.fw_name:
             self.wait_for_report(report_type=self.backend.delete_custom_framework, framework_name=self.fw_name)
    +    super().cleanup_workflows()
         return super().cleanup(**kwargs)
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Moving cleanup_workflows() after framework deletion is a critical improvement as it prevents potential dependency issues where workflows might need to be cleaned up after framework-related operations are complete.

    8
    Possible issue
    Enhance error handling by validating intermediate results before proceeding with operations

    Add error handling for get_channel_guid_by_name call as it could fail before
    reaching delete_channel_by_guid. This would provide better error tracking and
    prevent potential NoneType exceptions.

    tests_scripts/workflows/teams_workflows.py [138-141]

     try:
    -    self.delete_channel_by_guid(self.get_channel_guid_by_name(self.webhook_name))
    +    channel_guid = self.get_channel_guid_by_name(self.webhook_name)
    +    if channel_guid:
    +        self.delete_channel_by_guid(channel_guid)
    +    else:
    +        Logger.logger.error(f"Could not find channel with name {self.webhook_name}")
     except Exception as e:
         Logger.logger.error(f"Failed to delete channel with name {self.webhook_name}, got exception {e}")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by separating the channel GUID retrieval from deletion and adding validation, which prevents potential NoneType exceptions and provides clearer error messages.

    7

    Copy link

    github-actions bot commented Jan 9, 2025

    Failed to generate code suggestions for PR

    @kooomix kooomix merged commit 1fc780c into master Jan 9, 2025
    2 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.

    1 participant