Merge pull request #20 from microsoft/issue1374

[BUG FIX] Variable group is created too late in spk setup command

src/commands/project/create-variable-group.ts

@@ -11,7 +11,6 @@ import {
   validateForRequiredValues,
 } from "../../lib/commandBuilder";
 import { PROJECT_PIPELINE_FILENAME } from "../../lib/constants";
-import { AzureDevOpsOpts } from "../../lib/git";
 import { addVariableGroup } from "../../lib/pipelines/variableGroup";
 import {
   hasValue,

src/commands/setup.test.ts

@@ -15,6 +15,8 @@ import * as promptInstance from "../lib/setup/prompt";
 import * as scaffold from "../lib/setup/scaffold";
 import * as setupLog from "../lib/setup/setupLog";
 import * as azureStorage from "../lib/setup/azureStorage";
+import * as variableGroup from "../lib/setup/variableGroup";
+import * as shell from "../lib/shell";
 import { deepClone } from "../lib/util";
 import { ConfigYaml } from "../types";
 import {
@@ -23,6 +25,7 @@ import {
   execute,
   getAPIClients,
   getErrorMessage,
+  isAzCLIInstall,
 } from "./setup";
 import * as setup from "./setup";
 
@@ -38,6 +41,8 @@ const mockRequestContext: RequestContext = {
   workspace: WORKSPACE,
 };
 
+jest.spyOn(variableGroup, "setupVariableGroup").mockResolvedValue();
+
 describe("test createSPKConfig function", () => {
   it("positive test", () => {
     const tmpFile = path.join(createTempDir(), "config.yaml");
@@ -420,3 +425,25 @@ describe("test getAPIClients function", () => {
     );
   });
 });
+
+describe("test isAzCLIInstall function", () => {
+  it("positive test", async () => {
+    const version = JSON.stringify({
+      "azure-cli": "2.0.79",
+    });
+    jest.spyOn(shell, "exec").mockResolvedValueOnce(version);
+    await isAzCLIInstall();
+  });
+  it("negative test: version is not returned", async () => {
+    jest.spyOn(shell, "exec").mockResolvedValueOnce("");
+    await expect(isAzCLIInstall()).rejects.toThrow(
+      errorMessage("setup-cmd-az-cli-err")
+    );
+  });
+  it("negative test: az is not installed", async () => {
+    jest.spyOn(shell, "exec").mockRejectedValueOnce(Error("test"));
+    await expect(isAzCLIInstall()).rejects.toThrow(
+      errorMessage("setup-cmd-az-cli-err")
+    );
+  });
+});

src/commands/setup.ts

@@ -38,6 +38,7 @@ import {
   manifestRepo,
 } from "../lib/setup/scaffold";
 import { create as createSetupLog } from "../lib/setup/setupLog";
+import { setupVariableGroup } from "../lib/setup/variableGroup";
 import { logger } from "../logger";
 import decorator from "./setup.decorator.json";
 import { createStorage } from "../lib/setup/azureStorage";
@@ -188,6 +189,7 @@ export const createAppRepoTasks = async (
       rc.acrName,
       RESOURCE_GROUP_LOCATION
     );
+    await setupVariableGroup(rc);
     await helmRepo(gitAPI, rc);
     await appRepo(gitAPI, rc);
     await createLifecyclePipeline(buildAPI, rc);
@@ -266,6 +268,7 @@ export const execute = async (
     const { coreAPI, gitAPI, buildAPI } = await getAPIClients();
 
     await createProjectIfNotExist(coreAPI, rc);
+    await setupVariableGroup(rc);
     await hldRepo(gitAPI, rc);
     await manifestRepo(gitAPI, rc);
     await createHLDtoManifestPipeline(buildAPI, rc);

src/lib/azure/keyvault.test.ts

@@ -1,12 +1,10 @@
-import { PipelineOptions } from "@azure/core-http";
 import { KeyVaultSecret } from "@azure/keyvault-secrets";
 import uuid from "uuid/v4";
 import { disableVerboseLogging, enableVerboseLogging } from "../../logger";
 import { getErrorMessage } from "../errorBuilder";
 import * as azurecredentials from "./azurecredentials";
 import { getClient, getSecret, setSecret } from "./keyvault";
 import * as keyvault from "./keyvault";
-import { TokenCredential } from "azure-storage";
 
 const keyVaultName = uuid();
 const mockedName = uuid();

src/lib/fileutils.test.ts

@@ -24,7 +24,7 @@ import {
   VM_IMAGE,
   BEDROCK_FILENAME,
 } from "../lib/constants";
-import { disableVerboseLogging, enableVerboseLogging, logger } from "../logger";
+import { disableVerboseLogging, enableVerboseLogging } from "../logger";
 import {
   createTestComponentYaml,
   createTestHldAzurePipelinesYaml,

src/lib/i18n.json

@@ -30,6 +30,14 @@
     "setup-cmd-git-api-err": "Could not get Git API client. Check the Azure DevOps credential.",
     "setup-cmd-build-api-err": "Could not get Build API client. Check the Azure DevOps credential.",
     "setup-cmd-cannot-locate-pr-for-approval": "Could not locate pull request for approval",
+    "setup-cmd-create-variable-group-missing-pat": "Could not create variable group because Personal Access Token was missing.",
+    "setup-cmd-create-variable-group-missing-acr-name": "Could not create variable group because ACR name was missing.",
+    "setup-cmd-create-variable-group-missing-sp-id": "Could not create variable group because service principal id was missing.",
+    "setup-cmd-create-variable-group-missing-sp-pwd": "Could not create variable group because service principal password was missing.",
+    "setup-cmd-create-variable-group-missing-tenant-id": "Could not create variable group because service principal tenant id was missing.",
+    "setup-cmd-create-variable-group-missing-storage-access-key": "Could not create variable group because storage account access key was missing.",
+    "setup-cmd-create-variable-group-missing-storage-account-name": "Could not create variable group because storage account name was missing.",
+    "setup-cmd-create-variable-group-missing-storage-table-name": "Could not create variable group because storage table name was missing.",
 
     "spk-config-yaml-err-readyaml": "Could not load file, {0}.",
     "spk-config-yaml-var-undefined": "Environment variable needs to be defined for {0} since it's referenced in the config file.",

src/lib/setup/prompt.test.ts

@@ -14,7 +14,6 @@ import {
   validationServicePrincipalInfoFromFile,
 } from "./prompt";
 import * as promptInstance from "./prompt";
-import * as gitService from "./gitService";
 import * as servicePrincipalService from "../azure/servicePrincipalService";
 import * as subscriptionService from "../azure/subscriptionService";
 

src/lib/setup/prompt.ts

@@ -16,11 +16,9 @@ import {
 import {
   ACR_NAME,
   DEFAULT_PROJECT_NAME,
-  HLD_REPO,
   RequestContext,
   WORKSPACE,
 } from "./constants";
-import { getAzureRepoUrl } from "./gitService";
 import {
   azCLILogin,
   createWithAzCLI,

src/lib/setup/scaffold.test.ts

@@ -2,12 +2,10 @@
 import * as fs from "fs-extra";
 import * as path from "path";
 import simpleGit from "simple-git/promise";
-import * as cmdCreateVariableGroup from "../../commands/project/create-variable-group";
 import * as projectInit from "../../commands/project/init";
+import * as cmdCreateVariableGroup from "../../commands/project/create-variable-group";
 import * as createService from "../../commands/service/create";
 import * as fileutils from "../../lib/fileutils";
-import * as variableGroup from "../../lib/pipelines/variableGroup";
-import * as sVariableGroup from "../setup/variableGroup";
 import { createTempDir } from "../ioUtil";
 import {
   APP_REPO,
@@ -23,7 +21,6 @@ import {
   hldRepo,
   initService,
   manifestRepo,
-  setupVariableGroup,
 } from "./scaffold";
 import * as scaffold from "./scaffold";
 
@@ -160,11 +157,16 @@ describe("test appRepo function", () => {
     const git = simpleGit();
     git.init = jest.fn();
 
-    jest.spyOn(scaffold, "setupVariableGroup").mockResolvedValueOnce();
     jest.spyOn(scaffold, "initService").mockResolvedValueOnce();
     jest.spyOn(projectInit, "initialize").mockImplementationOnce(async () => {
       fs.createFileSync("README.md");
     });
+    jest
+      .spyOn(cmdCreateVariableGroup, "setVariableGroupInBedrockFile")
+      .mockReturnValueOnce();
+    jest
+      .spyOn(cmdCreateVariableGroup, "updateLifeCyclePipeline")
+      .mockReturnValueOnce();
 
     await appRepo({} as any, createRequestContext(tempDir));
     const folder = path.join(tempDir, APP_REPO);
@@ -175,19 +177,6 @@ describe("test appRepo function", () => {
     jest.spyOn(createService, "createService").mockResolvedValueOnce();
     await initService(createRequestContext("test"), "test");
   });
-  it("sanity test on setupVariableGroup", async () => {
-    jest
-      .spyOn(variableGroup, "deleteVariableGroup")
-      .mockResolvedValueOnce(true);
-    jest.spyOn(sVariableGroup, "create").mockResolvedValueOnce();
-    jest
-      .spyOn(cmdCreateVariableGroup, "setVariableGroupInBedrockFile")
-      .mockReturnValueOnce();
-    jest
-      .spyOn(cmdCreateVariableGroup, "updateLifeCyclePipeline")
-      .mockReturnValueOnce();
-    await setupVariableGroup(createRequestContext("/dummy"));
-  });
   it("negative test", async () => {
     const tempDir = createTempDir();
     jest.spyOn(gitService, "createRepoInAzureOrg").mockImplementation(() => {

src/lib/setup/scaffold.ts

@@ -11,9 +11,6 @@ import { initialize as projectInitialize } from "../../commands/project/init";
 import { createService } from "../../commands/service/create";
 import { RENDER_HLD_PIPELINE_FILENAME } from "../../lib/constants";
 import { appendVariableGroupToPipelineYaml } from "../../lib/fileutils";
-import { AzureDevOpsOpts } from "../../lib/git";
-import { deleteVariableGroup } from "../../lib/pipelines/variableGroup";
-import { create as createVariableGroup } from "../../lib/setup/variableGroup";
 import { logger } from "../../logger";
 import {
   APP_REPO,
@@ -187,21 +184,6 @@ export const helmRepo = async (
   }
 };
 
-export const setupVariableGroup = async (rc: RequestContext): Promise<void> => {
-  const accessOpts: AzureDevOpsOpts = {
-    orgName: rc.orgName,
-    personalAccessToken: rc.accessToken,
-    project: rc.projectName,
-  };
-
-  await deleteVariableGroup(accessOpts, VARIABLE_GROUP);
-  await createVariableGroup(rc, VARIABLE_GROUP);
-  logger.info(`Successfully created variable group, ${VARIABLE_GROUP}`);
-
-  setVariableGroupInBedrockFile(".", VARIABLE_GROUP);
-  updateLifeCyclePipeline(".");
-};
-
 export const initService = async (
   rc: RequestContext,
   repoName: string
@@ -251,7 +233,8 @@ export const appRepo = async (
     );
 
     await projectInitialize(".", { defaultRing: "master" }); //How is master set normally?
-    await setupVariableGroup(rc);
+    setVariableGroupInBedrockFile(".", VARIABLE_GROUP);
+    updateLifeCyclePipeline(".");
     await initService(rc, repoName);
     await git.add("./*");
 

src/lib/setup/variableGroup.test.ts

@@ -1,7 +1,12 @@
-import { create, createVariableData } from "./variableGroup";
+import * as variableGroup from "../../lib/pipelines/variableGroup";
 import * as service from "../pipelines/variableGroup";
+import * as sVariableGroup from "../setup/variableGroup";
 import { RequestContext } from "./constants";
-import { deepClone } from "../util";
+import {
+  create,
+  createVariableData,
+  setupVariableGroup,
+} from "./variableGroup";
 
 const mockRequestContext: RequestContext = {
   orgName: "dummy",
@@ -28,26 +33,25 @@ describe("test createVariableData function", () => {
   it("sanity test", () => {
     createVariableData(mockRequestContext);
   });
-  it("negative test", () => {
-    const properties = [
-      "orgName",
-      "projectName",
-      "accessToken",
-      "acrName",
-      "servicePrincipalId",
-      "servicePrincipalPassword",
-      "servicePrincipalTenantId",
-      "storageAccountAccessKey",
-      "storageAccountName",
-      "storageTableName",
-    ];
-    properties.forEach((prop) => {
+  it("sanity test, only pat", () => {
+    createVariableData({
+      accessToken: "accessToken",
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      const clone = deepClone(mockRequestContext) as any;
-      delete clone[prop];
-      expect(() => {
-        createVariableData(clone);
-      }).toThrow();
-    });
+    } as any);
   });
+  it("negative test: with storageAccountAccessKey and missing sp", () => {
+    expect(() => {
+      createVariableData({
+        accessToken: "accessToken",
+        storageAccountAccessKey: "storageAccountAccessKey",
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      } as any);
+    }).toThrow();
+  });
+});
+
+it("sanity test on setupVariableGroup", async () => {
+  jest.spyOn(variableGroup, "deleteVariableGroup").mockResolvedValueOnce(true);
+  jest.spyOn(sVariableGroup, "create").mockResolvedValueOnce();
+  await setupVariableGroup(mockRequestContext);
 });