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

feat: extend ConnitNotification #225

Merged
merged 3 commits into from
May 27, 2024

Conversation

giovanni-guidini
Copy link
Contributor

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

ticket: codecov/engineering-team#1737

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.

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

ticket: codecov/engineering-team#1737
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.
Copy link
Contributor

@michelletran-codecov michelletran-codecov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@giovanni-guidini giovanni-guidini added this pull request to the merge queue May 27, 2024
Merged via the queue into main with commit 7a7847a May 27, 2024
6 checks passed
@giovanni-guidini giovanni-guidini deleted the gio/extend-commit-notification branch May 27, 2024 14:11
RulaKhaled pushed a commit that referenced this pull request 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.
giovanni-guidini added a commit that referenced this pull request Jun 7, 2024
I broke the GH fallback system again 😅
Fixes this issue: https://l.codecov.dev/dEsZeK

The problem is that since #225
(technically codecov/worker#470 actually) we include 1 extra piece of information in `GitHubAppInstallationInfo`, the `GitHubAppInstallation.id`.

This is breaking the function that gets tokens because we are destructuring the info dict
into the kwargs for the function (with the extra, unexpected `id` one).

To fix that we just pop it before calling the function.
github-merge-queue bot pushed a commit that referenced this pull request Jun 10, 2024
I broke the GH fallback system again 😅
Fixes this issue: https://l.codecov.dev/dEsZeK

The problem is that since #225
(technically codecov/worker#470 actually) we include 1 extra piece of information in `GitHubAppInstallationInfo`, the `GitHubAppInstallation.id`.

This is breaking the function that gets tokens because we are destructuring the info dict
into the kwargs for the function (with the extra, unexpected `id` one).

To fix that we just pop it before calling the function.
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

Successfully merging this pull request may close these issues.

None yet

2 participants