Remove Impact Analysis from API #1154
Conversation
michelletran-sentry
force-pushed
the
2833_remove_impact_analysis
branch
from
dc8410b to
2dc4a2d
Compare
michelletran-sentry
force-pushed
the
2833_remove_impact_analysis
branch
from
2dc4a2d to
ed73845
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
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 #1154 +/- ##
==========================================
- Coverage 96.07% 96.07% -0.01%
==========================================
Files 838 836 -2
Lines 19779 19723 -56
==========================================
- Hits 19002 18948 -54
+ Misses 777 775 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
michelletran-sentry
temporarily deployed
to
staging
GitHub Actions
Inactive
michelletran-sentry
force-pushed
the
2833_remove_impact_analysis
branch
from
ed73845 to
7b53859
Compare
adrian-codecov
left a comment
adrian-codecov
left a comment
There was a problem hiding this comment.
Lgtm, although I'd confirm w/ someone from the apps team if they're okay w/ the stubbing. Not sure if there's already someone working on Gazebo to stop querying those fields
Probably not. I looked into doing it myself, but there were a lot of references and I am but a lowly backend dev. I might pick it up again, or create a ticket for the Apps Team to clean up. But in the meantime, I want to at least clean up API without affecting Gazebo. |
| def resolve_is_critical_file(data, info): | ||
| critical_filenames = info.context["profiling_summary"].critical_filenames | ||
| return data.get("path") in critical_filenames | ||
| """DEPRECATED. Returning dummy value""" |
There was a problem hiding this comment.
one thing you can do on the related .graphql files is also tag these fields with @deprecated and say something like "impact analysis removed, these are just dummy values" or something, until we actually remove those fields
There was a problem hiding this comment.
This seems to be the only example in API atm
| command = info.context["executor"].get_command("repository") | ||
| return command.get_repository_token(repository, token_type="profiling") | ||
| """DEPRECATED""" | ||
| return "" |
There was a problem hiding this comment.
this one isn't being referenced in gazebo but is in a query here: https://github.com/codecov/gazebo/blob/0899f21d7fe6eae4248cce5fdd65c7043f4e247f/src/services/repo/useRepoSettings.tsx#L60
There was a problem hiding this comment.
I think it's probably safer for me to leave it stubbed until it's completely removed from Gazebo.
|
|
||
| @path_content_file_bindable.field("isCriticalFile") | ||
| @sync_to_async | ||
| def resolve_is_critical_file(item: Union[File, Dir], info) -> bool: |
There was a problem hiding this comment.
this one's used a lot, will require some surgery in Gazebo
| @sentry_sdk.trace | ||
| @impacted_file_bindable.field("isCriticalFile") | ||
| @sync_to_async | ||
| def resolve_is_critical_file(impacted_file: ImpactedFile, info) -> bool: |
There was a problem hiding this comment.
same story for this being used a lot and needs surgery
ajay-sentry
left a comment
ajay-sentry
left a comment
There was a problem hiding this comment.
couple thoughts / comments after doing a little gazebo searching but changes look good as is if we wanna ship
I've stubbed out Impact Analysis fields from the API, as it would take a lot of work to remove them from Gazebo first. So, just returning a bunch of dummy values for now.
michelletran-sentry
force-pushed
the
2833_remove_impact_analysis
branch
from
ea83341 to
fb40914
Compare
This is not being used in Gazebo, so removing it.
This is to make clear that these fields are actually deprecated.
michelletran-sentry
force-pushed
the
2833_remove_impact_analysis
branch
from
fb40914 to
3286c26
Compare
|
@ajay-sentry Updated based on comments. I've removed |
|
@michelletran-codecov Thanks for doing that! Lgtm ✅ |
Purpose/Motivation
Removes the Impact Analysis API endpoints and data from API.
I've stubbed out Impact Analysis fields from the GraphQL API, as it would take a lot of work to remove them from Gazebo first. So, just returning a bunch of dummy values for now.
Links to relevant tickets
codecov/engineering-team#2833
Notes to Reviewer
We've already given notice to users that Impact Analysis is deprecated as of January 31 2025. There's still quite a few calls to the endpoint, but most of them 401. There's a total of 2 successful calls to the API in the past 2 weeks. The feature itself has been broken for months, so removing the endpoints is probably fine.
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.