Skip to content

Commit

Permalink
Fixes uncaught errors with cloud self hosted integrations
Browse files Browse the repository at this point in the history
  • Loading branch information
axosoft-ramint committed Jan 21, 2025
1 parent 5747f4c commit 8394748
Show file tree
Hide file tree
Showing 10 changed files with 74 additions and 65 deletions.
4 changes: 4 additions & 0 deletions src/constants.integrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ export enum SelfHostedIntegrationId {
GitLabSelfHosted = 'gitlab-self-hosted',
}

export type CloudSelfHostedIntegrationId =
| SelfHostedIntegrationId.CloudGitHubEnterprise
| SelfHostedIntegrationId.CloudGitLabSelfHosted;

export enum IssueIntegrationId {
Jira = 'jira',
Trello = 'trello',
Expand Down
7 changes: 2 additions & 5 deletions src/git/remotes/remoteProviders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { RemotesConfig } from '../../config';
import { SelfHostedIntegrationId } from '../../constants.integrations';
import type { Container } from '../../container';
import type { ConfiguredIntegrationDescriptor } from '../../plus/integrations/authentication/models';
import { isCloudSelfHostedIntegrationId } from '../../plus/integrations/providers/models';
import { configuration } from '../../system/-webview/configuration';
import { Logger } from '../../system/logger';
import { AzureDevOpsRemote } from './azure-devops';
Expand Down Expand Up @@ -104,11 +105,7 @@ export function loadRemoteProviders(

if (configuredIntegrations?.length) {
for (const ci of configuredIntegrations) {
if (
(ci.integrationId === SelfHostedIntegrationId.CloudGitHubEnterprise ||
ci.integrationId === SelfHostedIntegrationId.CloudGitLabSelfHosted) &&
ci.domain
) {
if (isCloudSelfHostedIntegrationId(ci.integrationId) && ci.domain) {
const matcher = ci.domain.toLocaleLowerCase();
const providerCreator = (_container: Container, domain: string, path: string) =>
ci.integrationId === SelfHostedIntegrationId.CloudGitHubEnterprise
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import type { Container } from '../../../container';
import { gate } from '../../../system/decorators/-webview/gate';
import { debug, log } from '../../../system/decorators/log';
import type { DeferredEventExecutor } from '../../../system/event';
import { isSelfHostedIntegrationId, supportedIntegrationIds } from '../providers/models';
import {
isCloudSelfHostedIntegrationId,
isSelfHostedIntegrationId,
supportedIntegrationIds,
} from '../providers/models';
import type { ConfiguredIntegrationDescriptor, ProviderAuthenticationSession } from './models';
import { isSupportedCloudIntegrationId } from './models';

Expand Down Expand Up @@ -375,10 +379,7 @@ export abstract class CloudIntegrationAuthenticationProvider<
// Make an exception for GitHub because they always return 0

This comment has been minimized.

Copy link
@eamodio

eamodio Jan 21, 2025

Member

@axosoft-ramint should update the comment here, since this isn't only for GitHub now

if (
session?.expiresIn === 0 &&
(this.authProviderId === HostingIntegrationId.GitHub ||
this.authProviderId === SelfHostedIntegrationId.CloudGitHubEnterprise ||
// Note: added GitLab self managed here because the cloud token is always a PAT, and the api does not know when it expires, nor can it refresh it
this.authProviderId === SelfHostedIntegrationId.CloudGitLabSelfHosted)
(this.authProviderId === HostingIntegrationId.GitHub || isCloudSelfHostedIntegrationId(this.authProviderId))
) {
// It never expires so don't refresh it frequently:
session.expiresIn = maxSmallIntegerV8; // maximum expiration length
Expand Down
8 changes: 7 additions & 1 deletion src/plus/integrations/integration.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import type { CancellationToken, Event, MessageItem } from 'vscode';
import { EventEmitter, window } from 'vscode';
import type { AutolinkReference, DynamicAutolinkReference } from '../../autolinks';
import type { IntegrationId, IssueIntegrationId, SelfHostedIntegrationId } from '../../constants.integrations';
import type {
CloudSelfHostedIntegrationId,
IntegrationId,
IssueIntegrationId,
SelfHostedIntegrationId,
} from '../../constants.integrations';
import { HostingIntegrationId } from '../../constants.integrations';
import type { Sources } from '../../constants.telemetry';
import type { Container } from '../../container';
Expand Down Expand Up @@ -55,6 +60,7 @@ export type IntegrationResult<T> =

export type SupportedIntegrationIds = IntegrationId;
export type SupportedHostingIntegrationIds = HostingIntegrationId;
export type SupportedCloudSelfHostedIntegrationIds = CloudSelfHostedIntegrationId;
export type SupportedIssueIntegrationIds = IssueIntegrationId;
export type SupportedSelfHostedIntegrationIds = SelfHostedIntegrationId;

Expand Down
76 changes: 32 additions & 44 deletions src/plus/integrations/integrationService.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import type { AuthenticationSessionsChangeEvent, CancellationToken, Event } from 'vscode';
import { authentication, Disposable, env, EventEmitter, ProgressLocation, Uri, window } from 'vscode';
import { isWeb } from '@env/platform';
import type { IntegrationId, SupportedCloudIntegrationIds } from '../../constants.integrations';
import type {
CloudSelfHostedIntegrationId,
IntegrationId,
SupportedCloudIntegrationIds,
} from '../../constants.integrations';
import { HostingIntegrationId, IssueIntegrationId, SelfHostedIntegrationId } from '../../constants.integrations';
import type { Source } from '../../constants.telemetry';
import { sourceToContext } from '../../constants.telemetry';
Expand Down Expand Up @@ -38,12 +42,13 @@ import type {
IntegrationType,
IssueIntegration,
ResourceDescriptor,
SupportedCloudSelfHostedIntegrationIds,
SupportedHostingIntegrationIds,
SupportedIntegrationIds,
SupportedIssueIntegrationIds,
SupportedSelfHostedIntegrationIds,
} from './integration';
import { isHostingIntegrationId, isSelfHostedIntegrationId } from './providers/models';
import { isCloudSelfHostedIntegrationId, isHostingIntegrationId, isSelfHostedIntegrationId } from './providers/models';
import type { ProvidersApi } from './providers/providersApi';
import { isGitHubDotCom, isGitLabDotCom } from './providers/utils';

Expand Down Expand Up @@ -136,20 +141,22 @@ export class IntegrationService implements Disposable {

private async *getSupportedCloudIntegrations(domainsById: Map<IntegrationId, string>): AsyncIterable<Integration> {
for (const id of getSupportedCloudIntegrationIds()) {
if (
(id === SelfHostedIntegrationId.CloudGitHubEnterprise ||
id === SelfHostedIntegrationId.CloudGitLabSelfHosted) &&
!domainsById.has(id)
) {
if (isCloudSelfHostedIntegrationId(id) && !domainsById.has(id)) {
try {

This comment has been minimized.

Copy link
@eamodio

eamodio Jan 21, 2025

Member

@axosoft-ramint We should be able to lose the try/catch now

// Try getting whatever we have now because we will need to disconnect
yield this.get(id);
const integration = await this.get(id, undefined);
if (integration != null) {
yield integration;
}
} catch {
// Ignore this exception and continue,
// because we probably haven't ever had an instance of this integration
}
} else {
yield this.get(id, domainsById.get(id));
const integration = await this.get(id, domainsById.get(id));
if (integration != null) {
yield integration;
}
}
}
}
Expand Down Expand Up @@ -245,6 +252,7 @@ export class IntegrationService implements Disposable {
for (const integrationId of integrationIds) {
try {
const integration = await this.get(integrationId);
if (integration == null) continue;
if (integration.maybeConnected ?? (await integration.isConnected())) {
connectedIntegrations.add(integrationId);
}
Expand Down Expand Up @@ -368,6 +376,7 @@ export class IntegrationService implements Disposable {
if (integrationIds != null) {
for (const integrationId of integrationIds) {
const integration = await this.get(integrationId);
if (integration == null) continue;
const connected = integration.maybeConnected ?? (await integration.isConnected());
if (connected && !connectedIntegrations.has(integrationId)) {
return true;
Expand Down Expand Up @@ -455,19 +464,18 @@ export class IntegrationService implements Disposable {
return key == null ? this._connectedCache.size !== 0 : this._connectedCache.has(key);
}

get(
id:
| SupportedHostingIntegrationIds
| SelfHostedIntegrationId.CloudGitHubEnterprise
| SelfHostedIntegrationId.CloudGitLabSelfHosted,
): Promise<HostingIntegration>;
get(id: SupportedHostingIntegrationIds): Promise<HostingIntegration>;
get(id: SupportedIssueIntegrationIds): Promise<IssueIntegration>;
get(id: SupportedSelfHostedIntegrationIds, domain: string): Promise<HostingIntegration>;
get(id: SupportedIntegrationIds, domain?: string): Promise<Integration>;
get(
id: SupportedHostingIntegrationIds | SupportedCloudSelfHostedIntegrationIds,
domain?: string,
): Promise<HostingIntegration | undefined>;
get(id: SupportedSelfHostedIntegrationIds, domain: string): Promise<HostingIntegration | undefined>;
get(id: SupportedIntegrationIds, domain?: string): Promise<Integration | undefined>;
async get(
id: SupportedHostingIntegrationIds | SupportedIssueIntegrationIds | SupportedSelfHostedIntegrationIds,
domain?: string,
): Promise<Integration> {
): Promise<Integration | undefined> {
let integration = this.getCached(id, domain);
if (integration == null) {
switch (id) {
Expand Down Expand Up @@ -504,7 +512,7 @@ export class IntegrationService implements Disposable {
break;
}

throw new Error(`Domain is required for '${id}' integration`);
return undefined;
}

integration = new (
Expand Down Expand Up @@ -562,7 +570,7 @@ export class IntegrationService implements Disposable {
break;
}

throw new Error(`Domain is required for '${id}' integration`);
return undefined;
}

integration = new (
Expand Down Expand Up @@ -825,25 +833,9 @@ export class IntegrationService implements Disposable {
args: { 0: integrationIds => (integrationIds?.length ? integrationIds.join(',') : '<undefined>') },
})
async getMyCurrentAccounts(
integrationIds: (
| HostingIntegrationId
| SelfHostedIntegrationId.CloudGitHubEnterprise
| SelfHostedIntegrationId.CloudGitLabSelfHosted
)[],
): Promise<
Map<
| HostingIntegrationId
| SelfHostedIntegrationId.CloudGitHubEnterprise
| SelfHostedIntegrationId.CloudGitLabSelfHosted,
Account
>
> {
const accounts = new Map<
| HostingIntegrationId
| SelfHostedIntegrationId.CloudGitHubEnterprise
| SelfHostedIntegrationId.CloudGitLabSelfHosted,
Account
>();
integrationIds: (HostingIntegrationId | CloudSelfHostedIntegrationId)[],
): Promise<Map<HostingIntegrationId | CloudSelfHostedIntegrationId, Account>> {
const accounts = new Map<HostingIntegrationId | CloudSelfHostedIntegrationId, Account>();
await Promise.allSettled(
integrationIds.map(async integrationId => {
const integration = await this.get(integrationId);
Expand All @@ -862,11 +854,7 @@ export class IntegrationService implements Disposable {
args: { 0: integrationIds => (integrationIds?.length ? integrationIds.join(',') : '<undefined>'), 1: false },
})
async getMyPullRequests(
integrationIds?: (
| HostingIntegrationId
| SelfHostedIntegrationId.CloudGitHubEnterprise
| SelfHostedIntegrationId.CloudGitLabSelfHosted
)[],
integrationIds?: (HostingIntegrationId | CloudSelfHostedIntegrationId)[],
cancellation?: CancellationToken,
silent?: boolean,
): Promise<IntegrationResult<SearchedPullRequest[] | undefined>> {
Expand Down
6 changes: 5 additions & 1 deletion src/plus/integrations/providers/models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
GitPullRequestReviewState,
GitPullRequestState,
} from '@gitkraken/provider-apis';
import type { IntegrationId } from '../../../constants.integrations';
import type { CloudSelfHostedIntegrationId, IntegrationId } from '../../../constants.integrations';
import { HostingIntegrationId, IssueIntegrationId, SelfHostedIntegrationId } from '../../../constants.integrations';
import type { Account as UserAccount } from '../../../git/models/author';
import type { IssueMember, SearchedIssue } from '../../../git/models/issue';
Expand Down Expand Up @@ -103,6 +103,10 @@ export function isHostingIntegrationId(id: IntegrationId): id is HostingIntegrat
].includes(id as HostingIntegrationId);
}

export function isCloudSelfHostedIntegrationId(id: IntegrationId): id is CloudSelfHostedIntegrationId {
return id === SelfHostedIntegrationId.CloudGitHubEnterprise || id === SelfHostedIntegrationId.CloudGitLabSelfHosted;
}

export enum PullRequestFilter {
Author = 'author',
Assignee = 'assignee',
Expand Down
5 changes: 2 additions & 3 deletions src/plus/integrations/providers/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type { LaunchpadItem } from '../../launchpad/launchpadProvider';
import type { IssueResourceDescriptor, RepositoryDescriptor } from '../integration';
import { isIssueResourceDescriptor, isRepositoryDescriptor } from '../integration';
import type { GitConfigEntityIdentifier } from './models';
import { isCloudSelfHostedIntegrationId } from './models';

export function isGitHubDotCom(domain: string): boolean {
return equalsIgnoreCase(domain, 'github.com');
Expand Down Expand Up @@ -134,9 +135,7 @@ export function encodeIssueOrPullRequestForGitConfig(
id: entity.id,
owner: encodedOwner,
createdDate: new Date().toISOString(),
isCloudEnterprise:
entity.provider.id === SelfHostedIntegrationId.CloudGitHubEnterprise ||
entity.provider.id === SelfHostedIntegrationId.CloudGitLabSelfHosted,
isCloudEnterprise: isCloudSelfHostedIntegrationId(entity.provider.id as IntegrationId),
},
};
}
Expand Down
1 change: 1 addition & 0 deletions src/plus/launchpad/launchpad.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ export class LaunchpadCommand extends QuickCommand<State> {

private async ensureIntegrationConnected(id: IntegrationId) {
const integration = await this.container.integrations.get(id);
if (integration == null) return false;
let connected = integration.maybeConnected ?? (await integration.isConnected());
if (!connected) {
connected = await integration.connect('launchpad');
Expand Down
16 changes: 10 additions & 6 deletions src/plus/launchpad/launchpadProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { CancellationToken, ConfigurationChangeEvent } from 'vscode';
import { Disposable, env, EventEmitter, Uri, window } from 'vscode';
import { md5 } from '@env/crypto';
import { GlCommand } from '../../constants.commands';
import type { IntegrationId } from '../../constants.integrations';
import type { CloudSelfHostedIntegrationId, IntegrationId } from '../../constants.integrations';
import { HostingIntegrationId, SelfHostedIntegrationId } from '../../constants.integrations';
import type { Container } from '../../container';
import { CancellationError } from '../../errors';
Expand Down Expand Up @@ -107,11 +107,7 @@ type PullRequestsWithSuggestionCounts = {

export type LaunchpadRefreshEvent = LaunchpadCategorizedResult;

export const supportedLaunchpadIntegrations: (
| HostingIntegrationId
| SelfHostedIntegrationId.CloudGitHubEnterprise
| SelfHostedIntegrationId.CloudGitLabSelfHosted
)[] = [
export const supportedLaunchpadIntegrations: (HostingIntegrationId | CloudSelfHostedIntegrationId)[] = [
HostingIntegrationId.GitHub,
SelfHostedIntegrationId.CloudGitHubEnterprise,
HostingIntegrationId.GitLab,
Expand Down Expand Up @@ -278,6 +274,7 @@ export class LaunchpadProvider implements Disposable {
)
.map(async (id: SupportedLaunchpadIntegrationIds) => {
const integration = await this.container.integrations.get(id);
if (integration == null) return;
const searchResult = await searchIntegrationPRs(integration);
const prs = searchResult?.value;
if (prs) {
Expand Down Expand Up @@ -412,6 +409,7 @@ export class LaunchpadProvider implements Disposable {
});
if (confirm !== 'Merge') return;
const integration = await this.container.integrations.get(integrationId);
if (integration == null) return;
const pr: PullRequest = fromProviderPullRequest(item, integration);
await integration.mergePullRequest(pr);
this.refresh();
Expand Down Expand Up @@ -604,6 +602,7 @@ export class LaunchpadProvider implements Disposable {
for (const integrationId of supportedLaunchpadIntegrations) {
if (connectedIntegrations.get(integrationId)) {
const integration = await this.container.integrations.get(integrationId);
if (integration == null) continue;
const prIdentity = integration.getPullRequestIdentityFromMaybeUrl(search);
if (prIdentity) {
return prIdentity;
Expand Down Expand Up @@ -873,6 +872,7 @@ export class LaunchpadProvider implements Disposable {
async hasConnectedIntegration(): Promise<boolean> {
for (const integrationId of supportedLaunchpadIntegrations) {
const integration = await this.container.integrations.get(integrationId);
if (integration == null) continue;
if (integration.maybeConnected ?? (await integration.isConnected())) {
return true;
}
Expand All @@ -887,6 +887,10 @@ export class LaunchpadProvider implements Disposable {
await Promise.allSettled(
supportedLaunchpadIntegrations.map(async integrationId => {
const integration = await this.container.integrations.get(integrationId);
if (integration == null) {
connected.set(integrationId, false);
return;
}
const isConnected = integration.maybeConnected ?? (await integration.isConnected());
const hasAccess = isConnected && (await integration.access());
connected.set(integrationId, hasAccess);
Expand Down
5 changes: 5 additions & 0 deletions src/plus/startWork/startWork.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ export abstract class StartWorkBaseCommand extends QuickCommand<State> {

private async ensureIntegrationConnected(id: IntegrationId) {
const integration = await this.container.integrations.get(id);
if (integration == null) return false;
let connected = integration.maybeConnected ?? (await integration.isConnected());
if (!connected) {
connected = await integration.connect(this.overrides?.ownSource ?? 'startWork');
Expand Down Expand Up @@ -726,6 +727,10 @@ async function getConnectedIntegrations(container: Container): Promise<Map<Suppo
await Promise.allSettled(
supportedStartWorkIntegrations.map(async integrationId => {
const integration = await container.integrations.get(integrationId);
if (integration == null) {
connected.set(integrationId, false);
return;
}
const isConnected = integration.maybeConnected ?? (await integration.isConnected());
const hasAccess = isConnected && (await integration.access());
connected.set(integrationId, hasAccess);
Expand Down

0 comments on commit 8394748

Please sign in to comment.