-
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 bundle analysis report breakdown for a given commit #339
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Codecov Report
@@ Coverage Diff @@
## main #339 +/- ##
==========================================
+ Coverage 96.03% 96.05% +0.01%
==========================================
Files 626 628 +2
Lines 16231 16307 +76
==========================================
+ Hits 15588 15664 +76
Misses 643 643
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 ✅
Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
==========================================
+ Coverage 96.03% 96.05% +0.01%
==========================================
Files 626 628 +2
Lines 16231 16307 +76
==========================================
+ Hits 15588 15664 +76
Misses 643 643
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
=======================================
+ Coverage 95.71 95.73 +0.02
=======================================
Files 741 743 +2
Lines 16747 16823 +76
=======================================
+ Hits 16028 16104 +76
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. |
|
||
|
||
@bundle_analysis_report_bindable.field("sizeTotal") | ||
def resolve_size_total(bundles_analysis_report: BundleAnalysisReport, info): |
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.
Could we add return types for these resolvers pls?
return bundles_analysis_report.bundles | ||
|
||
|
||
@bundle_report_bindable.field("name") |
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.
Wdyt about separating bundle analysis reports and bundles into their own folders? Rn they're similar but we could eventually make that difference more pronounced, so might make sense to separate that into different folders
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.
I don't see there being an entry point into bundles without going through bundle analysis though. If/when that does come up in the future, we can separate it, but for now I think the bundle analysis report and bundle analysis comparison folders separation is intuitive.
graphql_api/types/commit/commit.py
Outdated
@@ -160,6 +163,12 @@ def resolve_bundle_analysis_compare_with_parent(commit: Commit, info, **kwargs): | |||
return load_bundle_analysis_comparison(base_commit, commit) | |||
|
|||
|
|||
@commit_bindable.field("bundleAnalysisReport") | |||
@sync_to_async | |||
def resolve_bundle_analysis_report(commit: Commit, info, **kwargs): |
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.
Also return type here please 😬
report_type=CommitReport.ReportType.BUNDLE_ANALYSIS, commit=commit | ||
).first() | ||
if report is None: | ||
return MissingHeadReport() |
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.
Could this fn ever load a non-head report? Trying to see if it's better to say MissingReport as opposed of the head
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.
Doesn't appear like we have anything more suitable. FWIW we do use MissingHeadReport
on non comparison like Commits.pathContents
self.report = report | ||
self.report_bundles = [] | ||
self.total_size = 0 | ||
for bundle in self.report.bundle_reports(): |
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.
TODO as we spoke previously:
- add functionality to calculate total BundleAnalysisReport size in shared
- rethink how to ensure we close/start all db connections in shared as well, fine here for now
- replace BundleReport to use the SharedBundleReport to avoid disconnect bw data here and data from shared
(could you make an issue capturing this when you get a chance please?)
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.
codecov/engineering-team#1067
codecov/engineering-team#1068
Yep will refactor this code next when tackling these two tickets.
Purpose/Motivation
What is the feature? Why is this being done?
GQL: Add a resolver in Commit type, that gets the bundle report of the commit, that displays the total size of all the bundles and size of each individual bundle
Links to relevant tickets
codecov/engineering-team#1027
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.