feat: introduce database fields for github apps <-> batch changes integration, and update database layer#63577
Conversation
BolajiOlajide
left a comment
BolajiOlajide
left a comment
There was a problem hiding this comment.
A couple things to take care of here @bahrmichael
We should be good to merge soon.
Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com>
…he not null constraint
| &c.ExternalServiceID, | ||
| &encryptedCredential, | ||
| &keyID, | ||
| &dbutil.NullInt{N: &c.GitHubAppID}, |
There was a problem hiding this comment.
Do we actually want to use a NullInt here? In the case that GitHubAppID is null, it will set the ID to zero. Do we check everywhere we use it that the ID is non-zero? Personally, it feels more natural to use *int as an optional int.
There was a problem hiding this comment.
From what I can see, GitHubAppId will never be zero if it's set. Checking against a zero-value feels more golang to me than having a nullable value. We have checks in place already that the id has to be something else than 0 (internal/batches/types/site_credential.go). @BolajiOlajide what do you think?
There was a problem hiding this comment.
AFAIK, both approaches work. Considering GitHubAppID is a foreign key, there'd never be a zero value in the database, so this is pretty safe. I'd be more worried if it wasn't a foreign key.
| } | ||
|
|
||
| func (uc *UserCredential) githubAppAuthenticator() (auth.Authenticator, error) { | ||
| return nil, errors.New("not implemented") |
There was a problem hiding this comment.
Will any batch changes have a github app associated with it by default when this PR is merged? Mostly, I just don't want us to merge this and start seeing "not implemented" errors unless someone tries to specifically opt in.
There was a problem hiding this comment.
With migrations/frontend/1719498091_add_github_app_id_user_credentials/up.sql and migrations/frontend/1719498032_add_github_app_batch_changes_site_credential/up.sql we introduce the new column, but don't set a value. Unless someone goes into the database and manually sets a value, there should never be one, and therefore no failure. I'm rather confident about this, and would prefer seeing a hard value on s2 if that assumption turns out to be false.
The implementation of this is going to be one of the immediate next steps, so it should never be rolled out to customers.
We prefer to ship this, so that we can base future work on it instead of piling up more work in one big PR.
There was a problem hiding this comment.
Yup. Also to add, the UI implementation for this isn't done yet so there's no way for someone to add a GitHub app yet.
BolajiOlajide
deleted the
06-28-add_github_app_id_field_to_site_credential_struct
branch
3 participants
Closes SRCH-659
This PR introduces new database fields to site_credentials and user_credentials. The goal is to enable an integration between regular batch change activity and github apps.
We have implemented db migrations to add the new fields, and updated the database layer to work with those. There's a new skipped test that can be unskipped once we implement the new could that errors out with "not implemented".
This should be safe to merge as long as no one is creating github apps and credentials directly in the database. There is no exposed feature yet through which users could do this themselves.
Test plan
Changelog