First pass converting seer api calls to using signature #72486
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #72486 +/- ##
===========================================
+ Coverage 67.02% 78.10% +11.07%
===========================================
Files 6605 6601 -4
Lines 294521 294098 -423
Branches 50761 50664 -97
===========================================
+ Hits 197403 229697 +32294
+ Misses 90285 58122 -32163
+ Partials 6833 6279 -554
|
| response = requests.post( | ||
| f"{settings.SEER_AUTOFIX_URL}{path}", | ||
| data=body, | ||
| headers={ | ||
| "content-type": "application/json;charset=utf-8", | ||
| **sign_with_seer_secret( | ||
| path=path, | ||
| body=body, | ||
| ), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Do you think you're at a point where you would benefit from a small client library that packages up these common header and request serialization concerns?
There was a problem hiding this comment.
Yes, although that is a bit deeper of a change with some larger risks as well. Not all call sites behave the same way and I don't have full confidence I understand their contexts, in part because those callsites are maintained by other teams. My first thought was to get this change in safely and then consider other changes.
| "min_change()": cls.min_change, | ||
| # "trend_percentage()": 0.5, # require a minimum 50% increase |
There was a problem hiding this comment.
Was this updated in seer? Last I checked, this param required parenthesis.
There was a problem hiding this comment.
For awhile (since changes I made) it has been a working alias so that both work: https://github.com/getsentry/seer/blob/6f656ea74d695c59b504e5648e615861038e04d9/src/seer/trend_detection/trend_detector.py#L52
This change also pushes us closer to uhm, making this all implementable with gRPC 🤫 getsentry/seer#798
|
@markstory Finally ready for review if you have a chance. |
| @@ -51,15 +52,23 @@ def get_autofix_repos_from_project_code_mappings(project: Project) -> list[dict] | |||
|
|
|||
|
|
|||
| def get_autofix_state_from_pr_id(provider: str, pr_id: int) -> AutofixState | None: | |||
| path = "/v1/automation/autofix/state/pr" | |||
| body = json.dumps( | |||
There was a problem hiding this comment.
Probably, I took what was at the original callsite and used that. Should all json calls be orjson now? Is there any considersations or concerns about behavior changes?
Gotta adjust some tests.
This addresses the need to sign not just to sentry, but from sentry. Unfortunately, given the way we did signing originally, we can't reuse the existing secret due to asymmetry. However, good news is that all of this may be replaceable with grpc in the near future. This addresses the immediate security issues and puts us on track to remove most of this later.
See https://github.com/getsentry/seer/pull/774/files