Post-merge review of #3356

[eamodio]

Review of #3356

• Can we consolidate the following to hide that complexity inside the integration.connect() call?

  vscode-gitlens/src/commands/remoteProviders.ts

  Lines 99 to 115 in 2f9750e

  	let connected = await integration.connect();
  	if (!connected) {
  	if (isSupportedCloudIntegrationId(integration.id)) {
  	await this.container.integrations.manageCloudIntegrations(
  	{ integrationId: integration.id, skipIfConnected: true },
  	{
  	source: 'remoteProvider',
  	detail: {
  	action: 'connect',
  	integration: integration.id,
  	},
  	},
  	);
  	}
  	connected = await integration.connect();

  vscode-gitlens/src/plus/focus/focus.ts

  Lines 162 to 177 in 2f9750e

  	let connected = integration.maybeConnected ?? (await integration.isConnected());
  	if (!connected) {
  	if (isSupportedCloudIntegrationId(integration.id)) {
  	await this.container.integrations.manageCloudIntegrations(
  	{ integrationId: integration.id },
  	{
  	source: 'launchpad',
  	detail: {
  	action: 'connect',
  	integration: integration.id,
  	},
  	},
  	);
  	}
  	connected = await integration.connect();
  	}

• Suggest changing the notion of "secret" to "session"/"stored session"/"auth":

  vscode-gitlens/src/plus/integrations/authentication/integrationAuthentication.ts

  Lines 71 to 96 in 305d406

  	protected async deleteSecret(key: SecretKeys) {
  	await this.container.storage.deleteSecret(key);
  	}
  	protected async writeSecret(key: SecretKeys, session: AuthenticationSession | StoredSession) {
  	await this.container.storage.storeSecret(key, JSON.stringify(session));
  	}
  	protected async readSecret(key: SecretKeys, ignoreErrors: boolean): Promise<StoredSession | undefined> {
  	let storedSession: StoredSession | undefined;
  	try {
  	const sessionJSON = await this.container.storage.getSecret(key);
  	if (sessionJSON) {
  	storedSession = JSON.parse(sessionJSON);
  	}
  	} catch (ex) {
  	try {
  	await this.deleteSecret(key);
  	} catch {}
  	if (ignoreErrors) {
  	throw ex;
  	}
  	}
  	return storedSession;
  	}

• Restrict the key params to be a subset of SecretKeys to avoid something else getting in there

  vscode-gitlens/src/plus/integrations/authentication/integrationAuthentication.ts

  Lines 71 to 96 in 305d406

  	protected async deleteSecret(key: SecretKeys) {
  	await this.container.storage.deleteSecret(key);
  	}
  	protected async writeSecret(key: SecretKeys, session: AuthenticationSession | StoredSession) {
  	await this.container.storage.storeSecret(key, JSON.stringify(session));
  	}
  	protected async readSecret(key: SecretKeys, ignoreErrors: boolean): Promise<StoredSession | undefined> {
  	let storedSession: StoredSession | undefined;
  	try {
  	const sessionJSON = await this.container.storage.getSecret(key);
  	if (sessionJSON) {
  	storedSession = JSON.parse(sessionJSON);
  	}
  	} catch (ex) {
  	try {
  	await this.deleteSecret(key);
  	} catch {}
  	if (ignoreErrors) {
  	throw ex;
  	}
  	}
  	return storedSession;
  	}

• Generally avoid Promise.all and use Promose.allSettled when we want to wait for everything -- as Promise.all will throw on the first rejected promise

https://gitkraken.atlassian.net/browse/GLVSC-591

[sergeibbb]

@eamodio

Suggest changing the notion of "secret" to "session"/"stored session"/"auth":

I'm taking "auth". I pick anything but "session", because there is a layer of "[actOn]Session" methods that call to "[actOn]Secret" methods (that are kind of a lower level), that's why I took a different word for that.

[sergeibbb]

@eamodio

Restrict the key params to be a subset of SecretKeys to avoid something else getting in there

Do you mean adding a check on its value at runtime?

[eamodio]

@eamodio

Restrict the key params to be a subset of SecretKeys to avoid something else getting in there

Do you mean adding a check on its value at runtime?

More to just create a restricted type, e.g.

export type SecretKeys =
	| `gitlens.${AIProviders}.key`
	| `gitlens.plus.auth:${Environment}`
	| IntegrationAuthenticationKeys;

export type IntegrationAuthenticationKeys =
	| `gitlens.integration.auth:${IntegrationId}|${string}`
	| `gitlens.integration.auth.cloud:${IntegrationId}|${string}`;

And then use that new type

[sergeibbb]

More to just create a restricted type

ok. that's clear

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.