Skip to content
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

Add ability to pin a GH app to a given commit for notifications #1737

Closed
trent-codecov opened this issue May 14, 2024 · 1 comment
Closed
Assignees

Comments

@trent-codecov
Copy link
Contributor

trent-codecov commented May 14, 2024

When a comment/check is made by a specific GH app on a commit, cache which app was used. Use this app on subsequent requests if possible.

@giovanni-guidini
Copy link

giovanni-guidini commented May 21, 2024

The current idea is to cache the app in Redis. This has some benefits as it's fast to access and shared among the different worker nodes.

The thing is that we problem should treat Redis as a permanent store and give the keys some expiry date. This could be very long, but that might not be ideal. Maybe 15 days is enough time for most cases. This believes in a certain amount of "time locality" in commit notifications that is quite reasonable.

However there are many commits in a Codecov instance over 15 days. We need to make sure that we can hold this much extra data in the instance.

As a "permanent" solution to go with the Redis one we can possibly extend the CommitNotification object (see here)
We can save the GitHub app used for the notification and pull that back up if the redis cache is empty.

There's yet another alternative that is to query github directly. Their API returns the app used to send a check (not status) OR comment.

giovanni-guidini added a commit to codecov/shared that referenced this issue May 22, 2024
These changes extend the `CommitNotification` model to include an optional
field: the github app that emitted a check / comment.

ticket: codecov/engineering-team#1737
giovanni-guidini added a commit to codecov/shared that referenced this issue May 22, 2024
These changes extend the `CommitNotification` model to include an optional
field: the github app that emitted a check / comment.

ticket: codecov/engineering-team#1737
github-merge-queue bot pushed a commit to codecov/shared that referenced this issue May 27, 2024
* feat: extend ConnitNotification

These changes extend the `CommitNotification` model to include an optional
field: the github app that emitted a check / comment.

ticket: codecov/engineering-team#1737

* improve README with instructions on how to run migrations

* chore: include instance ID in GithubInstallationInfo

Up to this point we didn't pass the GithubAppInstallation ID to
the Torngit adapter. This was in part because the info was not needed,
and in part because `Owner.integration_id` doesn't have an ID.

It would be useful to have this info now, though. As it will allow
us to pin down an app to a specific app installation more easily.
RulaKhaled pushed a commit to codecov/shared that referenced this issue May 27, 2024
* feat: extend ConnitNotification

These changes extend the `CommitNotification` model to include an optional
field: the github app that emitted a check / comment.

ticket: codecov/engineering-team#1737

* improve README with instructions on how to run migrations

* chore: include instance ID in GithubInstallationInfo

Up to this point we didn't pass the GithubAppInstallation ID to
the Torngit adapter. This was in part because the info was not needed,
and in part because `Owner.integration_id` doesn't have an ID.

It would be useful to have this info now, though. As it will allow
us to pin down an app to a specific app installation more easily.
@codecov-hooky codecov-hooky bot closed this as completed Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants