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

🔧 chore(noa): invoke all ticketing actions via the issue alert handlers #87372

Open
wants to merge 1 commit into
base: raj/aci/action-group
Choose a base branch
from

Conversation

iamrajjoshi
Copy link
Member

we need all of ticketing actions to flow to the issue alert handler.

this pr defines a new TicketingActionHandler ABC to handle invoking the issue alert handlers directly and updates the corresponding implementing classes to use the new ABC.

the changes are covered by this, so we should be good

@iamrajjoshi iamrajjoshi self-assigned this Mar 19, 2025
@iamrajjoshi iamrajjoshi requested a review from a team as a code owner March 19, 2025 01:33
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 19, 2025
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...low_engine/handlers/action/notification/handler.py 83.33% 2 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           raj/aci/action-group   #87372    +/-   ##
======================================================
  Coverage                 87.73%   87.74%            
======================================================
  Files                      9863     9863            
  Lines                    558658   558387   -271     
  Branches                  22024    22024            
======================================================
- Hits                     490164   489942   -222     
+ Misses                    68111    68062    -49     
  Partials                    383      383            

@iamrajjoshi iamrajjoshi requested a review from Copilot March 19, 2025 05:25
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors ticketing actions by funneling them through the issue alert handler and introduces a new TicketingActionHandler abstract base class.

  • Implements execute_via_metric_alert_handler to centralize notification handling for ticketing actions.
  • Introduces TicketingActionHandler with a dedicated configuration schema for ticketing actions.
  • Updates GitHub, GitHub Enterprise, Jira, Jira Server, and Azure Devops action handlers to extend TicketingActionHandler.
Comments suppressed due to low confidence (2)

src/sentry/workflow_engine/handlers/action/notification/handler.py:102

  • [nitpick] The configuration schema for 'target_identifier' is defined as type ["null"]. Verify if this is intentional or if a more flexible type (e.g., ["string", "null"]) is expected.
"type": ["null"],

src/sentry/workflow_engine/handlers/action/notification/handler.py:109

  • Ensure that ActionTarget is an iterable since using the splat operator (*) requires it to be unpackable.
"enum": [*ActionTarget],

# TODO(iamrajjoshi): Implement this, it should be used for the ticketing actions
pass
"""
This exists so that all ticketing actions can use the same handler as issue alerts since thats the only way we can
Copy link
Preview

Copilot AI Mar 19, 2025

Choose a reason for hiding this comment

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

Consider fixing the typo in the docstring: "thats" should be "that's".

Suggested change
This exists so that all ticketing actions can use the same handler as issue alerts since thats the only way we can
This exists so that all ticketing actions can use the same handler as issue alerts since that's the only way we can

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant