Skip to content

chore(Coder plugin): update all Backstage code to use preview SDK #131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
a27e95e
chore: add vendored version of experimental Coder SDK
Parkreiner May 24, 2024
9c958d3
chore: update CoderClient class to use new SDK
Parkreiner May 24, 2024
4979067
chore: delete mock SDK
Parkreiner May 24, 2024
5e7e01f
fix: improve data hiding for CoderSdk
Parkreiner May 24, 2024
937f6f5
docs: update typo
Parkreiner May 24, 2024
7e84d00
Merge branch 'mes/vendored-sdk' into mes/vendored-sdk-integration
Parkreiner May 24, 2024
294572d
wip: commit progress on updating Coder client
Parkreiner May 24, 2024
d9626a0
wip: commit more progress on updating types
Parkreiner May 24, 2024
1dcc13b
chore: remove valibot type definitions from global constants file
Parkreiner May 24, 2024
692a763
chore: rename mocks file
Parkreiner May 24, 2024
28accc8
fix: update type mismatches
Parkreiner May 24, 2024
d032768
wip: commit more update progress
Parkreiner May 24, 2024
a76db16
wip: commit progress on updating client/SDK integration
Parkreiner May 24, 2024
d22bc20
fix: get all tests passing for CoderClient
Parkreiner May 24, 2024
08cd049
fix: update UrlSync updates
Parkreiner May 24, 2024
2eb4987
fix: get all tests passing
Parkreiner May 24, 2024
37645f4
chore: update all mock data to use Coder core entity mocks
Parkreiner May 24, 2024
864357d
fix: add extra helpers to useCoderSdk
Parkreiner May 28, 2024
977b2eb
fix: add additional properties to hide from SDK
Parkreiner May 31, 2024
a9b24aa
Merge branch 'mes/vendored-sdk' into mes/vendored-sdk-integration
Parkreiner May 31, 2024
259702e
Merge branch 'main' into mes/vendored-sdk-integration
Parkreiner May 31, 2024
09240cc
fix: shrink down the API of useCoderSdk
Parkreiner May 31, 2024
3a8accb
update method name for clarity
Parkreiner Jun 3, 2024
a67fbcf
chore: removal vestigal endpoint properties
Parkreiner Jun 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 6 additions & 54 deletions plugins/backstage-plugin-coder/src/api/CoderClient.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
import {
CODER_AUTH_HEADER_KEY,
CoderClient,
disabledClientError,
} from './CoderClient';
import { CODER_AUTH_HEADER_KEY, CoderClient } from './CoderClient';
import type { IdentityApi } from '@backstage/core-plugin-api';
import { UrlSync } from './UrlSync';
import { rest } from 'msw';
Expand All @@ -12,8 +8,8 @@ import { delay } from '../utils/time';
import {
mockWorkspacesList,
mockWorkspacesListForRepoSearch,
} from '../testHelpers/mockCoderAppData';
import type { Workspace, WorkspacesResponse } from '../typesConstants';
} from '../testHelpers/mockCoderPluginData';
import type { Workspace, WorkspacesResponse } from './vendoredSdk';
import {
getMockConfigApi,
getMockDiscoveryApi,
Expand Down Expand Up @@ -100,50 +96,6 @@ describe(`${CoderClient.name}`, () => {
});
});

describe('cleanupClient functionality', () => {
it('Will prevent any new SDK requests from going through', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted because the cleanup method no longer exists

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!!

const client = new CoderClient({ apis: getConstructorApis() });
client.cleanupClient();

// Request should fail, even though token is valid
await expect(() => {
return client.syncToken(mockCoderAuthToken);
}).rejects.toThrow(disabledClientError);

await expect(() => {
return client.sdk.getWorkspaces({
q: 'owner:me',
limit: 0,
});
}).rejects.toThrow(disabledClientError);
});

it('Will abort any pending requests', async () => {
const client = new CoderClient({
initialToken: mockCoderAuthToken,
apis: getConstructorApis(),
});

// Sanity check to ensure that request can still go through normally
const workspacesPromise1 = client.sdk.getWorkspaces({
q: 'owner:me',
limit: 0,
});

await expect(workspacesPromise1).resolves.toEqual<WorkspacesResponse>({
workspaces: mockWorkspacesList,
count: mockWorkspacesList.length,
});

const workspacesPromise2 = client.sdk.getWorkspaces({
q: 'owner:me',
limit: 0,
});
client.cleanupClient();
await expect(() => workspacesPromise2).rejects.toThrow();
});
});

// Eventually the Coder SDK is going to get too big to test every single
// function. Focus tests on the functionality specifically being patched in
// for Backstage
Expand Down Expand Up @@ -180,10 +132,10 @@ describe(`${CoderClient.name}`, () => {
});

const { urlSync } = apis;
const apiEndpoint = await urlSync.getApiEndpoint();
const assetsEndpoint = await urlSync.getAssetsEndpoint();

const allWorkspacesAreRemapped = !workspaces.some(ws =>
ws.template_icon.startsWith(apiEndpoint),
const allWorkspacesAreRemapped = workspaces.every(ws =>
ws.template_icon.startsWith(assetsEndpoint),
);
Comment on lines +135 to +138
Copy link
Member Author

@Parkreiner Parkreiner May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo – didn't realize when I wrote the tests that I was using the wrong endpoint type


expect(allWorkspacesAreRemapped).toBe(true);
Expand Down
81 changes: 32 additions & 49 deletions plugins/backstage-plugin-coder/src/api/CoderClient.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
import globalAxios, {
import {
AxiosError,
type AxiosInstance,
type InternalAxiosRequestConfig as RequestConfig,
} from 'axios';
import { type IdentityApi, createApiRef } from '@backstage/core-plugin-api';
import {
type Workspace,
CODER_API_REF_ID_PREFIX,
WorkspacesRequest,
WorkspacesResponse,
User,
} from '../typesConstants';
import { CODER_API_REF_ID_PREFIX } from '../typesConstants';
import type { UrlSync } from './UrlSync';
import type { CoderWorkspacesConfig } from '../hooks/useCoderWorkspacesConfig';
import { CoderSdk } from './MockCoderSdk';
import {
type CoderSdk,
type User,
type Workspace,
type WorkspacesRequest,
type WorkspacesResponse,
makeCoderSdk,
} from './vendoredSdk';

export const CODER_AUTH_HEADER_KEY = 'Coder-Session-Token';
const DEFAULT_REQUEST_TIMEOUT_MS = 20_000;
Expand All @@ -39,11 +39,6 @@ type CoderClientApi = Readonly<{
* Return value indicates whether the token is valid.
*/
syncToken: (newToken: string) => Promise<boolean>;

/**
* Cleans up a client instance, removing its links to all external systems.
*/
cleanupClient: () => void;
}>;

const sharedCleanupAbortReason = new DOMException(
Expand All @@ -59,19 +54,30 @@ export const disabledClientError = new Error(
);

type ConstructorInputs = Readonly<{
/**
* initialToken is strictly for testing, and is basically limited to making it
* easier to test API logic.
*
* If trying to test UI logic that depends on CoderClient, it's probably
* better to interact with CoderClient indirectly through the auth components,
* so that React state is aware of everything.
*/
initialToken?: string;
requestTimeoutMs?: number;

requestTimeoutMs?: number;
apis: Readonly<{
urlSync: UrlSync;
identityApi: IdentityApi;
}>;
}>;

type RequestInterceptor = (
config: RequestConfig,
) => RequestConfig | Promise<RequestConfig>;

Comment on lines +74 to +76
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted this out to make addRequestInterceptor have slightly less wonky Prettier formatting

export class CoderClient implements CoderClientApi {
private readonly urlSync: UrlSync;
private readonly identityApi: IdentityApi;
private readonly axios: AxiosInstance;

private readonly requestTimeoutMs: number;
private readonly cleanupController: AbortController;
Expand All @@ -82,33 +88,28 @@ export class CoderClient implements CoderClientApi {

constructor(inputs: ConstructorInputs) {
const {
apis,
initialToken,
apis: { urlSync, identityApi },
requestTimeoutMs = DEFAULT_REQUEST_TIMEOUT_MS,
} = inputs;
const { urlSync, identityApi } = apis;

this.urlSync = urlSync;
this.identityApi = identityApi;
this.axios = globalAxios.create();

this.loadedSessionToken = initialToken;
this.requestTimeoutMs = requestTimeoutMs;

this.cleanupController = new AbortController();
this.trackedEjectionIds = new Set();

this.sdk = this.getBackstageCoderSdk(this.axios);
this.sdk = this.createBackstageCoderSdk();
this.addBaseRequestInterceptors();
}

private addRequestInterceptor(
requestInterceptor: (
config: RequestConfig,
) => RequestConfig | Promise<RequestConfig>,
requestInterceptor: RequestInterceptor,
errorInterceptor?: (error: unknown) => unknown,
): number {
const ejectionId = this.axios.interceptors.request.use(
const axios = this.sdk.getAxiosInstance();
const ejectionId = axios.interceptors.request.use(
requestInterceptor,
errorInterceptor,
);
Expand All @@ -120,7 +121,8 @@ export class CoderClient implements CoderClientApi {
private removeRequestInterceptorById(ejectionId: number): boolean {
// Even if we somehow pass in an ID that hasn't been associated with the
// Axios instance, that's a noop. No harm in calling method no matter what
this.axios.interceptors.request.eject(ejectionId);
const axios = this.sdk.getAxiosInstance();
axios.interceptors.request.eject(ejectionId);

if (!this.trackedEjectionIds.has(ejectionId)) {
return false;
Expand Down Expand Up @@ -179,10 +181,8 @@ export class CoderClient implements CoderClientApi {
this.addRequestInterceptor(baseRequestInterceptor, baseErrorInterceptor);
}

private getBackstageCoderSdk(
axiosInstance: AxiosInstance,
): BackstageCoderSdk {
const baseSdk = new CoderSdk(axiosInstance);
private createBackstageCoderSdk(): BackstageCoderSdk {
const baseSdk = makeCoderSdk();

const getWorkspaces: (typeof baseSdk)['getWorkspaces'] = async request => {
const workspacesRes = await baseSdk.getWorkspaces(request);
Expand Down Expand Up @@ -335,23 +335,6 @@ export class CoderClient implements CoderClientApi {
this.removeRequestInterceptorById(validationId);
}
};

cleanupClient = (): void => {
this.trackedEjectionIds.forEach(id => {
this.axios.interceptors.request.eject(id);
});

this.trackedEjectionIds.clear();
this.cleanupController.abort(sharedCleanupAbortReason);
this.loadedSessionToken = undefined;

// Not using this.addRequestInterceptor, because we don't want to track this
// interceptor at all. It should never be ejected once the client has been
// disabled
this.axios.interceptors.request.use(() => {
throw disabledClientError;
});
};
}

function appendParamToQuery(
Expand Down
48 changes: 0 additions & 48 deletions plugins/backstage-plugin-coder/src/api/MockCoderSdk.ts

This file was deleted.

8 changes: 4 additions & 4 deletions plugins/backstage-plugin-coder/src/api/UrlSync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import {
getMockConfigApi,
getMockDiscoveryApi,
mockBackstageAssetsEndpoint,
mockBackstageApiEndpoint,
mockBackstageUrlRoot,
mockBackstageApiEndpointWithoutSdkPath,
} from '../testHelpers/mockBackstageData';

// Tests have to assume that DiscoveryApi and ConfigApi will always be in sync,
Expand All @@ -23,7 +23,7 @@ describe(`${UrlSync.name}`, () => {
const cachedUrls = urlSync.getCachedUrls();
expect(cachedUrls).toEqual<UrlSyncSnapshot>({
baseUrl: mockBackstageUrlRoot,
apiRoute: mockBackstageApiEndpoint,
apiRoute: mockBackstageApiEndpointWithoutSdkPath,
assetsRoute: mockBackstageAssetsEndpoint,
});
});
Expand All @@ -50,7 +50,7 @@ describe(`${UrlSync.name}`, () => {

expect(newSnapshot).toEqual<UrlSyncSnapshot>({
baseUrl: 'blah',
apiRoute: 'blah/coder/api/v2',
apiRoute: 'blah/coder',
assetsRoute: 'blah/coder',
});
});
Expand All @@ -76,7 +76,7 @@ describe(`${UrlSync.name}`, () => {

expect(onChange).toHaveBeenCalledWith({
baseUrl: 'blah',
apiRoute: 'blah/coder/api/v2',
apiRoute: 'blah/coder',
assetsRoute: 'blah/coder',
} satisfies UrlSyncSnapshot);

Expand Down
10 changes: 2 additions & 8 deletions plugins/backstage-plugin-coder/src/api/UrlSync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,10 @@ const PROXY_URL_KEY_FOR_DISCOVERY_API = 'proxy';

type UrlPrefixes = Readonly<{
proxyPrefix: string;
apiRoutePrefix: string;
assetsRoutePrefix: string;
}>;

export const defaultUrlPrefixes = {
proxyPrefix: `/api/proxy`,
apiRoutePrefix: '/api/v2',
assetsRoutePrefix: '', // Deliberately left as empty string
} as const satisfies UrlPrefixes;

export type UrlSyncSnapshot = Readonly<{
Expand Down Expand Up @@ -104,12 +100,10 @@ export class UrlSync implements UrlSyncApi {
}

private prepareNewSnapshot(newProxyUrl: string): UrlSyncSnapshot {
const { assetsRoutePrefix, apiRoutePrefix } = this.urlPrefixes;

return {
baseUrl: newProxyUrl.replace(proxyRouteReplacer, ''),
assetsRoute: `${newProxyUrl}${CODER_PROXY_PREFIX}${assetsRoutePrefix}`,
apiRoute: `${newProxyUrl}${CODER_PROXY_PREFIX}${apiRoutePrefix}`,
assetsRoute: `${newProxyUrl}${CODER_PROXY_PREFIX}`,
apiRoute: `${newProxyUrl}${CODER_PROXY_PREFIX}`,
};
}

Expand Down
Loading
Loading