feat(batches): overview of personal github apps #64105
Conversation
| @bahrmichael is it intentional this is still draft? Is it ready for review? |
I set it as a draft because it felt like a prototype that I wanted to get some early feedback on. It works well enough though, so we can just push it out if there are no objections. |
| metadata: 'read', | ||
| } | ||
| | ||
| export const UserGitHubAppsArea: FC<Props> = props => { |
There was a problem hiding this comment.
I don't think we should allow creation of GitHub apps in here, why?
Because the GitHub app will be orphaned and not tied to any Sourcegraph services.
There was a problem hiding this comment.
That's reasonable, and since we have text there indicating that github apps are for batch changes, they should find their way to the batch changes settings. I updated the code to hide the button if we're not in the siteAdmin context.
| In the video, under the Batch Changes settings page, should the copy under "Code host credentials" be changed so it no longer refers to "access tokens" specifically (now that a user can authenticate with github app)? Also, should we guide users towards github apps there? There doesn't seem to be any way to discover that they can use a github app (configured on a different page) to fulfil the credentials requirement |
| try { | ||
| const req = await fetch(`/githubapp/state?id=${app?.id}&domain=${app?.domain}`) | ||
| const req = await fetch( | ||
| `/githubapp/state?id=${app?.id}&domain=${app?.domain}&kind=${GitHubAppKind.USER_CREDENTIAL}` |
There was a problem hiding this comment.
Q: why did this change? Does adding the kind here still work for site admin setup?
| } | ||
| | ||
| export const GitHubAppsPage: React.FC<Props> = ({ batchChangesEnabled, telemetryRecorder }) => { | ||
| export const GitHubAppsPage: React.FC<Props> = ({ batchChangesEnabled, telemetryRecorder, isSiteAdmin }) => { |
There was a problem hiding this comment.
nit: non-blocking, but given the amount of branching here, it kinda feels like this should just be a separate component
There was a problem hiding this comment.
I disagree. The flag isSiteAdmin is used a couple times, but only in ternary statements. I find it still easy to read and understand how we're switching things around. If you feel strongly about this, I'm happy to change it.
| <> | ||
| Personal GitHub Apps are currently only used for Batch Changes, but this feature is not | ||
| enabled on your instance. | ||
| </> |
There was a problem hiding this comment.
If batch changes is not enabled, should we just hide the personal github apps page entirely?
There was a problem hiding this comment.
This already happens via a condition in routes.tsx. I added this text here in case the other condition breaks, and we accidentally render this page. Customers shouldn't see it, but I thought it'd be nicer to display a helpful message if that assumption breaks.
| AppID: int(id64), | ||
| Domain: domain, | ||
| BaseURL: baseURL, | ||
| Kind: kind, |
There was a problem hiding this comment.
Were we accidentally omitting kind before?
|
|
Closes SRCH-790
Screen.Recording.2024-07-26.at.15.24.55.mov
"+ Add installation" is broken because of SRCH-705
Test plan
Manual testing
Changelog