Track whether GitHub App is installed on org #2947
Conversation
|
I agree it should be a provider setting and not this, but oh well. |
|
I seem to recall that some (many?) of the GitHub operations had endpoints that worked for both users and orgs. I'm assuming that packages aren't one of those endpoints, given the other limitations. 😢 |
|
|
||
| BEGIN; | ||
|
|
||
| ALTER TABLE provider_github_app_installations RENAME COLUMN organization_id TO owner_id; |
There was a problem hiding this comment.
So, renaming a column like this will cause a brief outage between the migration apply and the updated code.
I'm not sure we need to worry about this yet, but it feels like the safer thing to do would be to add a new column (owner_id) and copy all the values (possibly with a default) into it if we need to change the name.
I'm not blocking this PR, but I'd like us to start thinking more about how to make sure our migrations are downtime-free given that we've had a few misses in the last month.
There was a problem hiding this comment.
Thanks for pointing that out. I can remove the rename, it's not necessary.
| BEGIN; | ||
|
|
||
| ALTER TABLE provider_github_app_installations RENAME COLUMN organization_id TO owner_id; | ||
| ALTER TABLE provider_github_app_installations ADD COLUMN is_org BOOLEAN NOT NULL DEFAULT FALSE; |
There was a problem hiding this comment.
This looks safe to me because the default should apply for existing binaries that don't know about the column, allowing them to pass the NOT NULL constraint.
That's right. Packages only have per-user or per-org endpoints. |
eleftherias
force-pushed
the
is-org-github-app
branch
from
77cccb1
to
414564b
Compare
There was a problem hiding this comment.
The code makes sense to me and I think @evankanderson 's suggestions have been addressed. I think this PR is ready to be approved once rebased.
| @@ -58,6 +58,9 @@ type ProviderService interface { | |||
| DeleteProvider(ctx context.Context, provider *db.Provider) error | |||
| } | |||
|
|
|||
| // TypeGitHubOrganization is the type returned from the GitHub API when the owner is an organization | |||
| const TypeGitHubOrganization = "Organization" | |||
There was a problem hiding this comment.
Nit: doesn't have to be exported
(not needed in this PR even if you agree the const doesn't have to be exported)
eleftherias
force-pushed
the
is-org-github-app
branch
from
414564b
to
7e39dbf
Compare
Summary
In preparation for https://github.com/stacklok/epics/issues/280, we need to know whether an app was installed on an organization or a personal user account, to know which API to call.
This adds an
isOrgcolumn to the installations DB table, similar to what we have for access tokens.Note: I don't think keeping track of the owner / org in the credentials is the right place. Eventually it could move to a GitHub metadata table that is specific to GitHub providers. This warrants further discussion and a design document. For now, I've followed the same pattern we're already using for access tokens.
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: