-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(batches): overview of personal github apps #64105
base: main
Are you sure you want to change the base?
Changes from 7 commits
5bf273c
79e04ae
2806ce7
bc931be
5da1840
8465b6f
7de840d
1837f5a
f7056c6
cf73f1a
f44f016
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,12 @@ import { | |
| // eslint-disable-next-line no-restricted-imports | ||
| import type { BreadcrumbItem } from '@sourcegraph/wildcard/src/components/PageHeader' | ||
|
|
||
| import { GitHubAppDomain, type GitHubAppByIDResult, type GitHubAppByIDVariables } from '../../graphql-operations' | ||
| import { | ||
| GitHubAppDomain, | ||
| type GitHubAppByIDResult, | ||
| type GitHubAppByIDVariables, | ||
| GitHubAppKind, | ||
| } from '../../graphql-operations' | ||
| import { ExternalServiceNode } from '../externalServices/ExternalServiceNode' | ||
| import { ConnectionList, ConnectionSummary, SummaryContainer } from '../FilteredConnection/ui' | ||
| import { PageTitle } from '../PageTitle' | ||
|
|
@@ -44,6 +49,7 @@ interface Props extends TelemetryProps, TelemetryV2Props { | |
| * The parent breadcrumb item to show for this page in the header. | ||
| */ | ||
| headerParentBreadcrumb: BreadcrumbItem | ||
| isSiteAdmin: boolean | ||
| /** An optional annotation to show in the page header. */ | ||
| headerAnnotation?: React.ReactNode | ||
| } | ||
|
|
@@ -53,6 +59,7 @@ export const GitHubAppPage: FC<Props> = ({ | |
| telemetryRecorder, | ||
| headerParentBreadcrumb, | ||
| headerAnnotation, | ||
| isSiteAdmin, | ||
| }) => { | ||
| const { appID } = useParams() | ||
| const navigate = useNavigate() | ||
|
|
@@ -81,7 +88,9 @@ export const GitHubAppPage: FC<Props> = ({ | |
|
|
||
| const onAddInstallation = async (app: NonNullable<GitHubAppByIDResult['gitHubApp']>): Promise<void> => { | ||
| 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}` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: why did this change? Does adding the kind here still work for site admin setup? |
||
| ) | ||
| const state = await req.text() | ||
| const trailingSlash = app.appURL.endsWith('/') ? '' : '/' | ||
| window.location.assign(`${app.appURL}${trailingSlash}installations/new?state=${state}`) | ||
|
|
@@ -100,7 +109,7 @@ export const GitHubAppPage: FC<Props> = ({ | |
| {removeModalOpen && ( | ||
| <RemoveGitHubAppModal | ||
| onCancel={() => setRemoveModalOpen(false)} | ||
| afterDelete={() => navigate('/site-admin/github-apps')} | ||
| afterDelete={() => navigate(`/${isSiteAdmin ? 'site-admin' : 'user/settings'}/github-apps`)} | ||
| app={app} | ||
| /> | ||
| )} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,12 +27,13 @@ import styles from './GitHubAppsPage.module.scss' | |
|
|
||
| interface Props extends TelemetryV2Props { | ||
| batchChangesEnabled: boolean | ||
| isSiteAdmin: boolean | ||
| } | ||
|
|
||
| export const GitHubAppsPage: React.FC<Props> = ({ batchChangesEnabled, telemetryRecorder }) => { | ||
| export const GitHubAppsPage: React.FC<Props> = ({ batchChangesEnabled, telemetryRecorder, isSiteAdmin }) => { | ||
|
||
| const { data, loading, error, refetch } = useQuery<GitHubAppsResult, GitHubAppsVariables>(GITHUB_APPS_QUERY, { | ||
| variables: { | ||
| domain: GitHubAppDomain.REPOS, | ||
| domain: isSiteAdmin ? GitHubAppDomain.REPOS : GitHubAppDomain.BATCHES, | ||
| }, | ||
| }) | ||
| const gitHubApps = useMemo(() => data?.gitHubApps?.nodes ?? [], [data]) | ||
|
|
@@ -63,28 +64,45 @@ export const GitHubAppsPage: React.FC<Props> = ({ batchChangesEnabled, telemetry | |
| className={classNames(styles.pageHeader, 'mb-3')} | ||
| description={ | ||
| <> | ||
| Create and connect a GitHub App to better manage GitHub code host connections.{' '} | ||
| <Link to="/help/admin/code_hosts/github#using-a-github-app" target="_blank"> | ||
| See how GitHub App configuration works. | ||
| </Link> | ||
| {batchChangesEnabled && ( | ||
| {isSiteAdmin ? ( | ||
| <> | ||
| Create and connect a GitHub App to better manage GitHub code host connections.{' '} | ||
| <Link to="/help/admin/code_hosts/github#using-a-github-app" target="_blank"> | ||
| See how GitHub App configuration works. | ||
| </Link> | ||
| </> | ||
| ) : batchChangesEnabled ? ( | ||
| <>Use personal GitHub Apps to act on your behalf when running Batch Changes.</> | ||
| ) : ( | ||
| <> | ||
| Personal GitHub Apps are currently only used for Batch Changes, but this feature is not | ||
| enabled on your instance. | ||
| </> | ||
|
||
| )} | ||
| {batchChangesEnabled && isSiteAdmin ? ( | ||
| <> | ||
| {' '} | ||
| To create a GitHub App to sign Batch Changes commits, visit{' '} | ||
| <Link to="/site-admin/batch-changes">Batch Changes settings</Link>. | ||
| </> | ||
| ) : ( | ||
| <> To create a GitHub App to sign Batch Changes commits, ask your site admin.</> | ||
| )} | ||
| </> | ||
| } | ||
| actions={ | ||
| <ButtonLink | ||
| to="/site-admin/github-apps/new" | ||
| className="ml-auto text-nowrap" | ||
| variant="primary" | ||
| as={Link} | ||
| > | ||
| <Icon aria-hidden={true} svgPath={mdiPlus} /> Create GitHub App | ||
| </ButtonLink> | ||
| isSiteAdmin ? ( | ||
| <ButtonLink | ||
| to="/site-admin/github-apps/new" | ||
| className="ml-auto text-nowrap" | ||
| variant="primary" | ||
| as={Link} | ||
| > | ||
| <Icon aria-hidden={true} svgPath={mdiPlus} /> Create GitHub App | ||
| </ButtonLink> | ||
| ) : ( | ||
| <></> | ||
| ) | ||
| } | ||
| /> | ||
| <Container className="mb-3"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import type { FC } from 'react' | ||
|
|
||
| import { Route, Routes } from 'react-router-dom' | ||
|
|
||
| import { useQuery } from '@sourcegraph/http-client' | ||
| import type { AuthenticatedUser } from '@sourcegraph/shared/src/auth' | ||
| import type { PlatformContextProps } from '@sourcegraph/shared/src/platform/context' | ||
| import type { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService' | ||
| import { lazyComponent } from '@sourcegraph/shared/src/util/lazyComponent' | ||
| import { ErrorAlert, LoadingSpinner } from '@sourcegraph/wildcard' | ||
|
|
||
| import { type SiteExternalServiceConfigResult, type SiteExternalServiceConfigVariables } from '../../graphql-operations' | ||
| import { SITE_EXTERNAL_SERVICE_CONFIG } from '../../site-admin/backend' | ||
|
|
||
| const GitHubAppPage = lazyComponent(() => import('../../components/gitHubApps/GitHubAppPage'), 'GitHubAppPage') | ||
| const GitHubAppsPage = lazyComponent(() => import('../../components/gitHubApps/GitHubAppsPage'), 'GitHubAppsPage') | ||
|
|
||
| interface Props extends TelemetryProps, PlatformContextProps { | ||
| authenticatedUser: AuthenticatedUser | ||
| batchChangesEnabled: boolean | ||
| } | ||
|
|
||
| export const UserGitHubAppsArea: FC<Props> = props => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should allow creation of GitHub apps in here, why?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| const { data, error, loading } = useQuery<SiteExternalServiceConfigResult, SiteExternalServiceConfigVariables>( | ||
| SITE_EXTERNAL_SERVICE_CONFIG, | ||
| {} | ||
| ) | ||
|
|
||
| if (error && !loading) { | ||
| return <ErrorAlert error={error} /> | ||
| } | ||
|
|
||
| if (loading && !error) { | ||
| return <LoadingSpinner /> | ||
| } | ||
bahrmichael marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (!data) { | ||
| return null | ||
| } | ||
|
|
||
| return ( | ||
| <Routes> | ||
| <Route | ||
| index={true} | ||
| element={ | ||
| <GitHubAppsPage | ||
| batchChangesEnabled={props.batchChangesEnabled} | ||
| telemetryRecorder={props.platformContext.telemetryRecorder} | ||
| isSiteAdmin={false} | ||
| /> | ||
| } | ||
| /> | ||
|
|
||
| <Route | ||
| path=":appID" | ||
| element={ | ||
| <GitHubAppPage | ||
| headerParentBreadcrumb={{ to: '/user/github-apps', text: 'GitHub Apps' }} | ||
| telemetryRecorder={props.platformContext.telemetryRecorder} | ||
| isSiteAdmin={false} | ||
| {...props} | ||
| /> | ||
| } | ||
| /> | ||
| </Routes> | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,7 @@ func (srv *gitHubAppServer) stateHandler(w http.ResponseWriter, r *http.Request) | |
| gqlID := r.URL.Query().Get("id") | ||
| domain := r.URL.Query().Get("domain") | ||
| baseURL := r.URL.Query().Get("baseURL") | ||
| kind := r.URL.Query().Get("kind") | ||
| if gqlID == "" { | ||
| // we marshal an empty `gitHubAppStateDetails` struct because we want the values saved in the cache | ||
| // to always conform to the same structure. | ||
|
|
@@ -159,6 +160,7 @@ func (srv *gitHubAppServer) stateHandler(w http.ResponseWriter, r *http.Request) | |
| AppID: int(id64), | ||
| Domain: domain, | ||
| BaseURL: baseURL, | ||
| Kind: kind, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Were we accidentally omitting kind before? |
||
| }) | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("Unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.