-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: Add a coverage enabled column to repos #340
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
=======================================
Coverage 96.06% 96.06%
=======================================
Files 623 624 +1
Lines 16160 16171 +11
=======================================
+ Hits 15524 15535 +11
Misses 636 636
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report
@@ Coverage Diff @@
## main #340 +/- ##
=======================================
Coverage 96.06% 96.06%
=======================================
Files 623 624 +1
Lines 16160 16171 +11
=======================================
+ Hits 15524 15535 +11
Misses 636 636
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 ✅
Changes have been made to critical files, which contain lines commonly executed in production. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
=====================================
Coverage 95.69 95.69
=====================================
Files 738 739 +1
Lines 16675 16687 +12
=====================================
+ Hits 15956 15968 +12
Misses 719 719
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
@@ -338,6 +338,11 @@ def resolve_bundle_analysis_enabled(repository: Repository, info) -> bool: | |||
return repository.bundle_analysis_enabled | |||
|
|||
|
|||
@repository_bindable.field("coverageEnabled") | |||
def resolve_coverage_enabled(repository: Repository, info) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the return type is optional cause it can be null, can we add Optional to the return type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -130,6 +130,7 @@ class Languages(models.TextChoices): | |||
activated = models.BooleanField(null=True, default=False) | |||
deleted = models.BooleanField(default=False) | |||
bundle_analysis_enabled = models.BooleanField(default=False, null=True) | |||
coverage_enabled = models.BooleanField(default=False, null=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this won't have a DB default right, is that what you intend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, only ORM default, just to keep it consistent with the related fields (bundle_analysis_enabled, active, activated).
name="coverage_enabled", | ||
field=models.BooleanField(default=False, null=True), | ||
), | ||
RiskyRunSQL("UPDATE repos SET coverage_enabled=true WHERE active=true;"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you confirm these run sequentially? Would bork the DB if you rand the update before the field is in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep it runs sequentially on my local migrations run, I hope the ordering is not non-deterministic, that would be crazy. Didn't find in the Django docs that mentioned ordering, but there's many instances in the code that would imply this runs sequentially.
Purpose/Motivation
What is the feature? Why is this being done?
Create a column to determine if a user has coverage running. This differentiates from
active
because that is if any commit report has been uploaded, we need this to differentiate between coverage and bundle analysis.This copies the current
active
values to the new column, can do this because there is no upload for bundle analysis yet.Links to relevant tickets
codecov/engineering-team#1033
Legal 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.