Skip to content
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

feat: update per API change for importing ruleset #228

Merged
merged 12 commits into from
Aug 9, 2024
34 changes: 29 additions & 5 deletions src/azure/ApiCenter/ApiCenterService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { ISubscriptionContext } from "@microsoft/vscode-azext-utils";
import { getCredentialForToken } from "../../utils/credentialUtil";
import { APICenterRestAPIs } from "./ApiCenterRestAPIs";
import { ApiCenter, ApiCenterApi, ApiCenterApiDeployment, ApiCenterApiVersion, ApiCenterApiVersionDefinition, ApiCenterApiVersionDefinitionExport, ApiCenterApiVersionDefinitionImport, ApiCenterEnvironment, ApiCenterRulesetConfig, ApiCenterRulesetExport, ApiCenterRulesetImport } from "./contracts";
import { ApiCenter, ApiCenterApi, ApiCenterApiDeployment, ApiCenterApiVersion, ApiCenterApiVersionDefinition, ApiCenterApiVersionDefinitionExport, ApiCenterApiVersionDefinitionImport, ApiCenterEnvironment, ApiCenterRulesetConfig, ApiCenterRulesetExport, ApiCenterRulesetImport, ApiCenterRulesetImportResult } from "./contracts";

export class ApiCenterService {
private susbcriptionContext: ISubscriptionContext;
Expand Down Expand Up @@ -226,16 +226,40 @@
return response.parsedBody;
}

public async importRuleset(importPayload: ApiCenterRulesetImport): Promise<HttpOperationResponse> {
public async importRuleset(importPayload: ApiCenterRulesetImport): Promise<ApiCenterRulesetImportResult> {
const creds = getCredentialForToken(await this.susbcriptionContext.credentials.getToken());
const client = new ServiceClient(creds);
const options: RequestPrepareOptions = {
let options: RequestPrepareOptions = {
method: "POST",
url: APICenterRestAPIs.ImportRuleset(this.susbcriptionContext.subscriptionId, this.resourceGroupName, this.apiCenterName, this.apiVersionPreview),
body: importPayload
};
const response = await client.sendRequest(options);
return response;
let response = await client.sendRequest(options);

if (response.status === 202) {
const location = response.headers.get("Location");

if (!location) {
return { isSuccessful: false };
formulahendry marked this conversation as resolved.
Show resolved Hide resolved
}

Check warning on line 244 in src/azure/ApiCenter/ApiCenterService.ts

View check run for this annotation

Codecov / codecov/patch

src/azure/ApiCenter/ApiCenterService.ts#L243-L244

Added lines #L243 - L244 were not covered by tests

options = {
method: "GET",
url: location,
};

const timeout = 30000; // 30 seconds in milliseconds
Copy link
Member

Choose a reason for hiding this comment

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

Do we consider use retry times like 5 times? Or the service team recommend to use timeout?

Copy link
Member Author

Choose a reason for hiding this comment

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

The 'importing spec' written by service team used this pattern, so I just follow it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just synced with Alex yesterday, he recommand to set timeout as 60 seconds, as the job may be long.

let startTime = Date.now();
do {
response = await client.sendRequest(options);
if (response.parsedBody?.status === "Succeeded") {
formulahendry marked this conversation as resolved.
Show resolved Hide resolved
return { isSuccessful: true };
Copy link
Contributor

Choose a reason for hiding this comment

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

the response no need to return?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just return ApiCenterRulesetImportResult

}
} while (Date.now() - startTime < timeout);
return { isSuccessful: false };

Check warning on line 259 in src/azure/ApiCenter/ApiCenterService.ts

View check run for this annotation

Codecov / codecov/patch

src/azure/ApiCenter/ApiCenterService.ts#L259

Added line #L259 was not covered by tests
} else {
return { isSuccessful: false, message: response.bodyAsText };
}
}

public async exportRuleset(): Promise<ApiCenterRulesetExport> {
Expand Down
5 changes: 5 additions & 0 deletions src/azure/ApiCenter/contracts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ export type ApiCenterRulesetImport = {
value: string;
};

export type ApiCenterRulesetImportResult = {
isSuccessful: boolean;
message?: string | null;
};

export type ApiCenterRulesetExport = {
format: string;
value: string;
Expand Down
4 changes: 2 additions & 2 deletions src/commands/rules/deployRules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
};
const response = await apiCenterService.importRuleset(importPayload);

if (response.status === 200) {
if (response.isSuccessful) {

Check warning on line 32 in src/commands/rules/deployRules.ts

View check run for this annotation

Codecov / codecov/patch

src/commands/rules/deployRules.ts#L32

Added line #L32 was not covered by tests
vscode.window.showInformationMessage(vscode.l10n.t(UiStrings.RulesDeployed, node.apiCenter.name));
} else {
vscode.window.showErrorMessage(vscode.l10n.t(UiStrings.FailedToDeployRules, response.bodyAsText ?? `status code ${response.status}`));
vscode.window.showErrorMessage(vscode.l10n.t(UiStrings.FailedToDeployRules, response.message ?? `Error: ${response.message}`));

Check warning on line 35 in src/commands/rules/deployRules.ts

View check run for this annotation

Codecov / codecov/patch

src/commands/rules/deployRules.ts#L35

Added line #L35 was not covered by tests
}
}
8 changes: 4 additions & 4 deletions src/commands/rules/enableRules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
const resourceGroupName = getResourceGroupFromId(node.apiCenter.id);
const apiCenterService = new ApiCenterService(node.parent?.subscription!, resourceGroupName, node.apiCenter.name);

let response = await apiCenterService.createOrUpdateApiCenterRulesetConfig(apiCenterRulesetConfig);
const response = await apiCenterService.createOrUpdateApiCenterRulesetConfig(apiCenterRulesetConfig);

if (response.status === 200) {
vscode.window.showInformationMessage((vscode.l10n.t(UiStrings.RulesEnabled, node.apiCenter.name)));
Expand All @@ -42,13 +42,13 @@
value: content,
format: "InlineZip",
};
response = await apiCenterService.importRuleset(importPayload);
const importRulesetResponse = await apiCenterService.importRuleset(importPayload);

if (response.status === 200) {
if (importRulesetResponse.isSuccessful) {
vscode.window.showInformationMessage(vscode.l10n.t(UiStrings.RulesDeployed, node.apiCenter.name));
node.updateStatusToEnable();
await node.refresh(context);
} else {
vscode.window.showErrorMessage(vscode.l10n.t(UiStrings.FailedToDeployRules, response.bodyAsText ?? `status code ${response.status}`));
vscode.window.showErrorMessage(vscode.l10n.t(UiStrings.FailedToDeployRules, importRulesetResponse.message ?? `Error: ${importRulesetResponse.message}`));

Check warning on line 52 in src/commands/rules/enableRules.ts

View check run for this annotation

Codecov / codecov/patch

src/commands/rules/enableRules.ts#L52

Added line #L52 was not covered by tests
}
}
61 changes: 61 additions & 0 deletions src/test/unit/azure/ApiCenter/ApiCenterService.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
import { HttpHeaders, HttpOperationResponse, ServiceClient } from "@azure/ms-rest-js";
import { ISubscriptionContext } from "@microsoft/vscode-azext-utils";
import * as assert from "assert";
import * as sinon from "sinon";
import { ApiCenterService } from "../../../../azure/ApiCenter/ApiCenterService";
import { ApiCenterRulesetImport } from "../../../../azure/ApiCenter/contracts";

describe("ApiCenterService", () => {
let sandbox: sinon.SinonSandbox;
let subscriptionContext: ISubscriptionContext;
before(() => {
sandbox = sinon.createSandbox();
});
beforeEach(() => {
subscriptionContext = {
credentials: {
getToken: sandbox.stub().resolves({ token: 'fake-token' })
}
} as unknown as ISubscriptionContext;
});
afterEach(() => {
sandbox.restore();
});
it("importRuleset succeeded", async () => {
const mockResponse1 = { status: 202 } as HttpOperationResponse;
mockResponse1.headers = new HttpHeaders({ Location: "fakeLocation" });
const mockResponse2 = {
status: 200,
parsedBody: { status: "Succeeded" },
} as HttpOperationResponse;
const sendRequestStub = sandbox.stub(ServiceClient.prototype, "sendRequest");
sendRequestStub.onFirstCall().resolves(mockResponse1);
sendRequestStub.onSecondCall().resolves(mockResponse2);

const apiCenterService = new ApiCenterService(subscriptionContext, "fakeResourceGroup", "fakeServiceName");
const importPayload: ApiCenterRulesetImport = {
value: "fakeValue",
format: "InlineZip",
};

const response = await apiCenterService.importRuleset(importPayload);

assert.strictEqual(response.isSuccessful, true);
});
it("importRuleset failed", async () => {
sandbox.stub(ServiceClient.prototype, "sendRequest").resolves({ status: 500, bodyAsText: "error" } as HttpOperationResponse);

const apiCenterService = new ApiCenterService(subscriptionContext, "fakeResourceGroup", "fakeServiceName");
const importPayload: ApiCenterRulesetImport = {
value: "fakeValue",
format: "InlineZip",
};

const response = await apiCenterService.importRuleset(importPayload);

assert.strictEqual(response.isSuccessful, false);
assert.strictEqual(response.message, "error");
});
});
4 changes: 2 additions & 2 deletions src/test/unit/commands/rules/enableRules.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as path from 'path';
import * as sinon from "sinon";
import * as vscode from "vscode";
import { ApiCenterService } from "../../../../azure/ApiCenter/ApiCenterService";
import { ApiCenter } from "../../../../azure/ApiCenter/contracts";
import { ApiCenter, ApiCenterRulesetImportResult } from "../../../../azure/ApiCenter/contracts";
import { enableRules } from "../../../../commands/rules/enableRules";
import { RulesTreeItem } from "../../../../tree/rules/RulesTreeItem";

Expand All @@ -34,7 +34,7 @@ describe("enableRules", () => {
sandbox.stub(path, 'join').returns(__dirname);
const showInformationMessage = sandbox.spy(vscode.window, "showInformationMessage");
sandbox.stub(ApiCenterService.prototype, "createOrUpdateApiCenterRulesetConfig").resolves({ status: 200 } as HttpOperationResponse);
sandbox.stub(ApiCenterService.prototype, "importRuleset").resolves({ status: 200 } as HttpOperationResponse);
sandbox.stub(ApiCenterService.prototype, "importRuleset").resolves({ isSuccessful: true } as ApiCenterRulesetImportResult);
await enableRules({} as IActionContext, node);
sandbox.assert.calledTwice(showInformationMessage);
assert.ok(node.isEnabled);
Expand Down
2 changes: 1 addition & 1 deletion src/uiStrings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ export class UiStrings {
static readonly FileAlreadyExists = vscode.l10n.t("The file '{0}' already exists. Please input a different name.");
static readonly NoRulesFolder = vscode.l10n.t("Rules folder '{0}' is empty. No files to deploy.");
static readonly RulesDeployed = vscode.l10n.t("Rules deployed to '{0}'.");
static readonly FailedToDeployRules = vscode.l10n.t("Failed to deploy rules. Error: {0}");
static readonly FailedToDeployRules = vscode.l10n.t("Failed to deploy rules. {0}");
static readonly RulesEnabled = vscode.l10n.t("API Analysis enabled for '{0}'.");
static readonly FailedToEnableRules = vscode.l10n.t("Failed to enable API Analysis. Error: {0}");
static readonly RulesFolderNotEmpty = vscode.l10n.t("The rules folder '{0}' is not empty. Do you want to overwrite the existing files?");
Expand Down
Loading