feat: allow config for multi-apps #421

giovanni-guidini · 2024-02-27T12:42:29Z

These changes introduce OwnerInstallationNameToUseForTask model.

This model allows an Owner to configure a installation_name to be
used for a given task_name. This can only be configured manually for now.

That is on purpose as we want to test these changes first.

context: codecov/engineering-team#1146

⚠️ These changes include a migration

Migration creates table for OwnerInstallationNameToUseForTask.
Should not be disruptive when deployed.

codecov-staging · 2024-02-27T12:52:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

codecov-qa · 2024-02-27T12:52:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.95%. Comparing base (ab757ca) to head (efdb97f).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #421   +/-   ##
=======================================
  Coverage   95.95%   95.95%           
=======================================
  Files         642      643    +1     
  Lines       16978    16999   +21     
=======================================
+ Hits        16291    16312   +21     
  Misses        687      687

Flag	Coverage Δ
unit	`95.95% <100.00%> (+<0.01%)`	⬆️
unit-latest-uploader	`95.95% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codecov-public-qa · 2024-02-27T12:52:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ab757ca) 95.95% compared to head (efdb97f) 95.95%.

@@           Coverage Diff           @@
##             main     #421   +/-   ##
=======================================
  Coverage   95.95%   95.95%           
=======================================
  Files         642      643    +1     
  Lines       16978    16999   +21     
=======================================
+ Hits        16291    16312   +21     
  Misses        687      687

Flag	Coverage Δ
unit	`95.95% <100.00%> (+<0.01%)`	⬆️
unit-latest-uploader	`95.95% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...0053_ownerinstallationnametousefortask_and_more.py	`100.00% <100.00%> (ø)`
codecov_auth/models.py	`98.97% <100.00%> (+0.02%)`	⬆️
webhook_handlers/views/github.py	`99.02% <100.00%> (+0.01%)`	⬆️

These changes introduce `OwnerInstallationNameToUseForTask` model. This model allows an `Owner` to configure a `installation_name` to be used for a given `task_name`. This can only be configured manually for now. That is on purpose as we want to test these changes first. context: codecov/engineering-team#1146 > ⚠️ **These changes include a migration** > > Migration creates table for `OwnerInstallationNameToUseForTask`. > Should not be disruptive when deployed.

trent-codecov · 2024-02-28T14:20:35Z

codecov_auth/migrations/0053_ownerinstallationnametousefortask_and_more.py

+                ("external_id", models.UUIDField(default=uuid.uuid4, editable=False)),
+                ("created_at", models.DateTimeField(auto_now_add=True)),
+                ("updated_at", models.DateTimeField(auto_now=True)),
+                ("installation_name", models.TextField()),


This just uses the name out of the app installations table? Any reason not to use a fk id?

The reason might be ignorance on my part of how FKs work... 😅

My understanding is that it would connect an instance of OwnerInstallationName... to 1 row in GithubAppInstallation. That's not what I want.

But there can be many OwnerInstallationName... that use the same installation_name and one installation_name can be used in multiple GithubAppInstallation.

So there's no direct connection between this row of OwnerInstallationName... and another row of GithubAppInstallation

This is what I was describing just to clarify. The way I understand it, a given task for an owner will only ever be assigned one GithubAppInstallation, either the default or a specific entry in your new table.

But there can be many OwnerInstallationName... that use the same installation_name and one installation_name can be used in multiple GithubAppInstallation.

I don't quite follow this statement. Happy to slack huddle too if you want to talk it out.

Yes that makes perfect sense. That's not exactly the model I was working on though.

The difference is subtle, but the implication is essentially that it allows us to have this piece of code

It might be an overkill. But then you have "classes of apps" essentially (under a same name).
And they are all considered pretty equally when you select them for some task.

am i understanding your idea right?

partner creates a GitHub app named "Codecov Notifications"

partner creates an OwnerInstallationNameToUseForTask which says tasks.Notify should use the app called "Codecov Notifications"

they wind up hitting the rate limit even on their dedicated notifications app

they create a distinct app, but they give it the same exact name "Codecov Notifications"

now we will automatically choose from 2 "Codecov Notifications" apps without them having to configure another OwnerInstallationNameToUseForTask

if that's right, it does make the UX smoother for users when they are so big that a dedicated app for a task is still not enough and they need to add a second, third, fourth app

i don't have a strong opinion on whether the slightly-weird design here is worth it for the improvement. the alternative UX is not that bad (same steps + configuring another OwnerInstallationToUseForTask), and afaik the need for this should be pretty rare, but if we add comments explaining this then the design isn't that strange

Your understanding is correct Matt

I think this implementation will work. Thanks for explaining it.

matt-codecov · 2024-02-28T16:25:27Z

codecov_auth/migrations/0053_ownerinstallationnametousefortask_and_more.py

+            name="OwnerInstallationNameToUseForTask",
+            fields=[
+                ("id", models.BigAutoField(primary_key=True, serialize=False)),
+                ("external_id", models.UUIDField(default=uuid.uuid4, editable=False)),


remind me what external_id means, i know we use it elsewhere too

All codecov base models include id, external_id, created_at and updated_at.
It is basically an ID for the object that we can share with users. A way to reference it that is not the actual ID

matt-codecov · 2024-02-28T19:03:04Z

codecov_auth/migrations/0053_ownerinstallationnametousefortask_and_more.py

+                ("external_id", models.UUIDField(default=uuid.uuid4, editable=False)),
+                ("created_at", models.DateTimeField(auto_now_add=True)),
+                ("updated_at", models.DateTimeField(auto_now=True)),
+                ("installation_name", models.TextField()),


am i understanding your idea right?

partner creates a GitHub app named "Codecov Notifications"

partner creates an OwnerInstallationNameToUseForTask which says tasks.Notify should use the app called "Codecov Notifications"

they wind up hitting the rate limit even on their dedicated notifications app

they create a distinct app, but they give it the same exact name "Codecov Notifications"

now we will automatically choose from 2 "Codecov Notifications" apps without them having to configure another OwnerInstallationNameToUseForTask

if that's right, it does make the UX smoother for users when they are so big that a dedicated app for a task is still not enough and they need to add a second, third, fourth app

i don't have a strong opinion on whether the slightly-weird design here is worth it for the improvement. the alternative UX is not that bad (same steps + configuring another OwnerInstallationToUseForTask), and afaik the need for this should be pretty rare, but if we add comments explaining this then the design isn't that strange

codecov · 2024-03-04T16:04:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.82%. Comparing base (ab757ca) to head (5f941e0).

❗ Current head 5f941e0 differs from pull request most recent head efdb97f. Consider uploading reports for the commit efdb97f to get more accurate results

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #421     +/-   ##
=======================================
+ Coverage   95.67   95.82   +0.15     
=======================================
  Files        764     758      -6     
  Lines      17564   17427    -137     
=======================================
- Hits       16803   16699    -104     
+ Misses       761     728     -33

Flag	Coverage Δ
unit	`96.11% <100.00%> (+0.16%)`	⬆️
unit-latest-uploader	`96.11% <100.00%> (+0.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

giovanni-guidini requested a review from a team as a code owner February 27, 2024 12:42

giovanni-guidini mentioned this pull request Feb 27, 2024

feat: Configure multi apps per task per owner codecov/worker#279

Merged

giovanni-guidini force-pushed the gio/owner2ghapp2use4task branch from ee853c2 to c27eede Compare February 27, 2024 13:01

Merge branch 'main' into gio/owner2ghapp2use4task

5f941e0

trent-codecov reviewed Feb 28, 2024

View reviewed changes

matt-codecov reviewed Feb 28, 2024

View reviewed changes

giovanni-guidini requested review from trent-codecov and matt-codecov March 1, 2024 14:41

trent-codecov approved these changes Mar 4, 2024

View reviewed changes

Merge branch 'main' into gio/owner2ghapp2use4task

efdb97f

giovanni-guidini merged commit bd7cf76 into main Mar 4, 2024
17 of 18 checks passed

giovanni-guidini deleted the gio/owner2ghapp2use4task branch March 4, 2024 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow config for multi-apps #421

feat: allow config for multi-apps #421

giovanni-guidini commented Feb 27, 2024

codecov-staging bot commented Feb 27, 2024 •

edited

codecov-qa bot commented Feb 27, 2024 •

edited

codecov-public-qa bot commented Feb 27, 2024 •

edited

trent-codecov Feb 28, 2024

giovanni-guidini Feb 28, 2024

trent-codecov Feb 28, 2024

giovanni-guidini Feb 28, 2024

matt-codecov Feb 28, 2024 •

edited

giovanni-guidini Feb 28, 2024

trent-codecov Feb 29, 2024

matt-codecov Feb 28, 2024

giovanni-guidini Feb 29, 2024

matt-codecov Feb 28, 2024 •

edited

codecov bot commented Mar 4, 2024

feat: allow config for multi-apps #421

feat: allow config for multi-apps #421

Conversation

giovanni-guidini commented Feb 27, 2024

codecov-staging bot commented Feb 27, 2024 • edited

Codecov Report

codecov-qa bot commented Feb 27, 2024 • edited

Codecov Report

codecov-public-qa bot commented Feb 27, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-codecov Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-codecov Feb 28, 2024 • edited

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2024

Codecov Report

codecov-staging bot commented Feb 27, 2024 •

edited

codecov-qa bot commented Feb 27, 2024 •

edited

codecov-public-qa bot commented Feb 27, 2024 •

edited

matt-codecov Feb 28, 2024 •

edited

matt-codecov Feb 28, 2024 •

edited