Logic to remove checks from team plan customer #463
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #463 +/- ##
=======================================
Coverage 97.34% 97.34%
=======================================
Files 409 409
Lines 33898 33952 +54
=======================================
+ Hits 32997 33051 +54
Misses 901 901
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
=======================================
Coverage 97.34% 97.34%
=======================================
Files 409 409
Lines 33898 33952 +54
=======================================
+ Hits 32997 33051 +54
Misses 901 901
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #463 +/- ##
=======================================
Coverage 97.34% 97.34%
=======================================
Files 409 409
Lines 33898 33952 +54
=======================================
+ Hits 32997 33051 +54
Misses 901 901
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
=======================================
Coverage 97.37% 97.37%
=======================================
Files 441 441
Lines 34653 34707 +54
=======================================
+ Hits 33743 33797 +54
Misses 910 910
Flags with carried forward coverage won't be shown. Click here to find out more.
|
services/notification/__init__.py
Outdated
| continue | ||
|
|
||
| # We always send statuses, so there's currently no case of checks without status. | ||
| if should_use_status_notifiers: |
There was a problem hiding this comment.
for readability maybe the logic following here should be encapsulated in a function
services/notification/__init__.py
Outdated
| ) | ||
|
|
||
| # Introduced to gate team plan not having statuses nor checks based on conditionals. | ||
| if not should_use_status_notifiers and not should_use_checks_notifier: |
There was a problem hiding this comment.
i'm wondering if this is meant to skip the comment and slack notifiers, if not, or if they will be skipped anyways, then we can remove this if statement
There was a problem hiding this comment.
We should have the comment, clarifying about the slack one, but how would removing this help?
To clarify the logic here, everything remains the same, the only change introduced by this ticket is "if you're in a team plan, and you have a "changes" or "project" status checks, we don't show them, we only show "patch" specific comments" (the ticket was originally "skip everything" but its now "skip some things". There was a fn _should_use_checks_notifier that has some logic in it already that decides whether you show checks or not - I added a small case inside to do the team plan changes. We also always sent statuses, which changes with this ticket, so I added a fn _should_use_status_notifier which just adds the team plan related logic to it, otherwise returns true.
So I don't see a way to get rid of this if ^ here.
Customers with team plan only get patch status/checks, regardless of their yaml settings. This PR is to add this logic as it was never done when first creating the team plan. Ticket here codecov/engineering-team#1660.
I introduced 2 new variables that indicate whether we need a status and a check. We are reusing
_should_use_checks_notifierto check for status I just added a small piece to make it work with this logic. We are adding_should_use_status_notifierwhich returns true unless it's a team plan without apatchstatus/check - we've always returned status otherwise. Let me know if there's any confusion with this approachLegal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.