-
Notifications
You must be signed in to change notification settings - Fork 10
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 functions to get/set apps to commits #469
feat: Add functions to get/set apps to commits #469
Conversation
Adds `set_github_app_for_commit` and `get_github_app_for_commit` functions. These let us set and later get a GithubAppInstallation.id that's related to a Commit.id
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: services/github.py
Did you find this useful? React with a 👍 or 👎 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #469 +/- ##
========================================
Coverage 97.25% 97.25%
========================================
Files 409 410 +1
Lines 33976 34082 +106
========================================
+ Hits 33042 33148 +106
Misses 934 934
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 #469 +/- ##
========================================
Coverage 97.25% 97.25%
========================================
Files 409 410 +1
Lines 33976 34082 +106
========================================
+ Hits 33042 33148 +106
Misses 934 934
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 #469 +/- ##
==========================================
+ Coverage 97.28% 97.30% +0.02%
==========================================
Files 441 441
Lines 34731 34939 +208
==========================================
+ Hits 33788 33999 +211
+ Misses 943 940 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes This change has been scanned for critical changes. Learn more |
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.
Generally LGTM except for a few comments!
services/github.py
Outdated
redis.set( | ||
COMMIT_GHAPP_KEY_NAME(commit.id), str(installation_id), ex=(60 * 60 * 2) | ||
) # 2h |
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.
For readability, let's extract (60 * 60 * 2)
as a constant as well.
|
||
|
||
def set_github_app_for_commit( | ||
installation_id: str | int | None, commit: Commit |
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.
Hmm, installation_id
has a lot of different types. In what circumstances should we be expecting a str
vs int
vs None
type? Should this be documented?
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.
LGTM!
Adds
set_github_app_for_commit
andget_github_app_for_commit
functions. These let us set and later get a GithubAppInstallation.id that's related to a Commit.idticket: codecov/engineering-team#1737
👀 This commit is part 1/4 of a bigger change that is actually done, but I decided to break it up into multiple PRs so it's easier to review. See how it plays with the rest here: main...gio/pin-commit-to-ghapp
(I'll try to leave the original branch up to date best as I can)