feat: send helpful email when strip prefix is incorrect (#144)

kormide · web-flow · commit a1f788792e4c · 2024-02-07T19:17:16.000-08:00
diff --git a/e2e/__snapshots__/e2e.spec.ts.snap b/e2e/__snapshots__/e2e.spec.ts.snap
@@ -54,6 +54,15 @@ modules/no-prefix/metadata.json
 "
 `;
 
+exports[`e2e tests [snapshot] error message for incorrect strip prefix 1`] = `
+"Failed to publish entry for testorg/versioned@v1.0.0 to the Bazel Central Registry.
+
+Could not find MODULE.bazel in release archive at ./versioned-1.0.0/MODULE.bazel.
+Is the strip prefix in source.template.json correct? (currently it's 'versioned-1.0.0')
+
+"
+`;
+
 exports[`e2e tests [snapshot] missing strip prefix 1`] = `
 "----------------------------------------------------
 modules/empty-prefix/1.0.0/MODULE.bazel
diff --git a/e2e/e2e.spec.ts b/e2e/e2e.spec.ts
@@ -149,7 +149,10 @@ describe("e2e tests", () => {
     const installationId = fakeGitHub.mockAppInstallation(testOrg, repo);
     fakeGitHub.mockAppInstallation(testOrg, "bazel-central-registry");
 
-    const releaseArchive = await makeReleaseTarball(repo, "zero-versioned-1.0.0");
+    const releaseArchive = await makeReleaseTarball(
+      repo,
+      "zero-versioned-1.0.0"
+    );
     await fakeGitHub.mockReleaseArchive(
       `/${testOrg}/${repo}/archive/refs/tags/${tag}.tar.gz`,
       releaseArchive
@@ -466,8 +469,14 @@ describe("e2e tests", () => {
     const installationId = fakeGitHub.mockAppInstallation(testOrg, repo);
     fakeGitHub.mockAppInstallation(testOrg, "bazel-central-registry");
 
-    const releaseArchive1 = await makeReleaseTarball(repo, "module-1.0.0");
-    const releaseArchive2 = await makeReleaseTarball(repo, "submodule-1.0.0");
+    const releaseArchive1 = await makeReleaseTarball(
+      repo,
+      "multi-module-1.0.0"
+    );
+    const releaseArchive2 = await makeReleaseTarball(
+      repo,
+      "multi-module-1.0.0"
+    );
 
     await fakeGitHub.mockReleaseArchive(
       `/${testOrg}/${repo}/releases/download/${tag}/module-${tag}.tar.gz`,
@@ -735,6 +744,48 @@ describe("e2e tests", () => {
     );
     expect(logs.latest?.author_name).toEqual("publish-to-bcr-bot");
   });
+
+  test("[snapshot] error message for incorrect strip prefix", async () => {
+    const repo = Fixture.Versioned;
+    const tag = "v1.0.0";
+    await setupLocalRemoteRulesetRepo(repo, tag, releaser);
+
+    fakeGitHub.mockUser(releaser);
+    fakeGitHub.mockRepository(testOrg, repo);
+    fakeGitHub.mockRepository(
+      testOrg,
+      "bazel-central-registry",
+      "bazelbuild",
+      "bazel-central-registry"
+    );
+    const rulesetInstallationId = fakeGitHub.mockAppInstallation(testOrg, repo);
+    fakeGitHub.mockAppInstallation(testOrg, "bazel-central-registry");
+
+    // Strip prefix in release archive doesn't match source.template.json
+    const releaseArchive = await makeReleaseTarball(repo, "invalid-prefix");
+    await fakeGitHub.mockReleaseArchive(
+      `/${testOrg}/${repo}/archive/refs/tags/${tag}.tar.gz`,
+      releaseArchive
+    );
+
+    const response = await publishReleaseEvent(
+      cloudFunctions.getBaseUrl(),
+      secrets.webhookSecret,
+      rulesetInstallationId,
+      {
+        owner: testOrg,
+        repo,
+        tag,
+        releaser,
+      }
+    );
+
+    expect(response.status).toEqual(200);
+
+    const messages = await fetchEmails(emailClient);
+    expect(messages.length).toEqual(1);
+    expect(messages[0].text).toMatchSnapshot();
+  });
 });
 
 const testReleaseArchives: string[] = [];
diff --git a/e2e/fixtures/multi-module/.bcr/source.template.json b/e2e/fixtures/multi-module/.bcr/source.template.json
@@ -1,5 +1,5 @@
 {
   "integrity": "",
-  "strip_prefix": "{REPO}-{VERSION}",
+  "strip_prefix": "{REPO}-{VERSION}/submodule",
   "url": "https://github.com/{OWNER}/{REPO}/releases/download/{TAG}/module-{TAG}.tar.gz"
 }
diff --git a/e2e/helpers/email.ts b/e2e/helpers/email.ts
@@ -41,17 +41,10 @@ export async function fetchEmails(
     )) {
       messages.push(await mailparser.simpleParser(message.source, {}));
     }
+    await emailClient.messageFlagsAdd({ seq: "1:*", seen: false }, ["\\Seen"]);
   } finally {
     await emailClient.mailboxClose();
   }
 
   return messages;
 }
-
-export async function deleteAllMail(emailClient: ImapFlow): Promise<void> {
-  try {
-    await emailClient.messageDelete("1:*");
-  } finally {
-    await emailClient.mailboxClose();
-  }
-}
diff --git a/src/domain/create-entry.spec.ts b/src/domain/create-entry.spec.ts
@@ -40,6 +40,7 @@ jest.mock("node:fs");
 
 const mockedFileReads: { [path: string]: string } = {};
 const EXTRACTED_MODULE_PATH = "/fake/path/to/MODULE.bazel";
+let mockReleaseArchive: ReleaseArchive;
 
 beforeEach(() => {
   jest.clearAllMocks();
@@ -79,14 +80,15 @@ beforeEach(() => {
     delete mockedFileReads[key];
   }
 
-  mocked(ReleaseArchive.fetch).mockImplementation(async () => {
-    return {
-      extractModuleFile: jest.fn(async () => {
-        return new ModuleFile(EXTRACTED_MODULE_PATH);
-      }),
-      diskPath: path.join(os.tmpdir(), "archive.tar.gz"),
-    } as unknown as ReleaseArchive;
-  });
+  mockReleaseArchive = {
+    extractModuleFile: jest.fn(async () => {
+      return new ModuleFile(EXTRACTED_MODULE_PATH);
+    }),
+    diskPath: path.join(os.tmpdir(), "archive.tar.gz"),
+    cleanup: jest.fn(),
+  } as Partial<ReleaseArchive> as ReleaseArchive;
+
+  mocked(ReleaseArchive.fetch).mockResolvedValue(mockReleaseArchive);
 
   mockGitClient = mocked(new GitClient());
   mockBcrForkGitHubClient = mocked(new GitHubClient({} as any));
@@ -185,6 +187,18 @@ describe("createEntryFiles", () => {
     );
   });
 
+  test("cleans up the release archive extraction", async () => {
+    mockRulesetFiles();
+
+    const tag = "v1.2.3";
+    const rulesetRepo = await RulesetRepository.create("repo", "owner", tag);
+    const bcrRepo = CANONICAL_BCR;
+
+    await createEntryService.createEntryFiles(rulesetRepo, bcrRepo, tag, ".");
+
+    expect(mockReleaseArchive.cleanup).toHaveBeenCalled();
+  });
+
   test("creates the required entry files for a different module root", async () => {
     mockRulesetFiles({
       moduleRoot: "sub/dir",
diff --git a/src/domain/create-entry.ts b/src/domain/create-entry.ts
@@ -58,50 +58,55 @@ export class CreateEntryService {
       sourceTemplate.url,
       sourceTemplate.stripPrefix
     );
-    const integrityHash = computeIntegrityHash(releaseArchive.diskPath);
-    sourceTemplate.setIntegrityHash(integrityHash);
 
-    const moduleFile = await releaseArchive.extractModuleFile(moduleRoot);
+    try {
+      const integrityHash = computeIntegrityHash(releaseArchive.diskPath);
+      sourceTemplate.setIntegrityHash(integrityHash);
 
-    const bcrEntryPath = path.resolve(
-      bcrRepo.diskPath,
-      "modules",
-      moduleFile.moduleName
-    );
-    const bcrVersionEntryPath = path.join(bcrEntryPath, version);
+      const moduleFile = await releaseArchive.extractModuleFile(moduleRoot);
 
-    if (!fs.existsSync(bcrEntryPath)) {
-      fs.mkdirSync(bcrEntryPath);
-    }
+      const bcrEntryPath = path.resolve(
+        bcrRepo.diskPath,
+        "modules",
+        moduleFile.moduleName
+      );
+      const bcrVersionEntryPath = path.join(bcrEntryPath, version);
 
-    const metadataTemplate = rulesetRepo.metadataTemplate(moduleRoot);
+      if (!fs.existsSync(bcrEntryPath)) {
+        fs.mkdirSync(bcrEntryPath);
+      }
 
-    updateMetadataFile(metadataTemplate, bcrEntryPath, version);
+      const metadataTemplate = rulesetRepo.metadataTemplate(moduleRoot);
 
-    fs.mkdirSync(bcrVersionEntryPath);
+      updateMetadataFile(metadataTemplate, bcrEntryPath, version);
 
-    this.addPatches(
-      rulesetRepo,
-      sourceTemplate,
-      moduleFile,
-      bcrVersionEntryPath,
-      moduleRoot
-    );
+      fs.mkdirSync(bcrVersionEntryPath);
 
-    this.patchModuleVersionIfMismatch(
-      moduleFile,
-      version,
-      sourceTemplate,
-      bcrVersionEntryPath
-    );
+      this.addPatches(
+        rulesetRepo,
+        sourceTemplate,
+        moduleFile,
+        bcrVersionEntryPath,
+        moduleRoot
+      );
 
-    sourceTemplate.save(path.join(bcrVersionEntryPath, "source.json"));
-    moduleFile.save(path.join(bcrVersionEntryPath, "MODULE.bazel"));
+      this.patchModuleVersionIfMismatch(
+        moduleFile,
+        version,
+        sourceTemplate,
+        bcrVersionEntryPath
+      );
 
-    fs.copyFileSync(
-      rulesetRepo.presubmitPath(moduleRoot),
-      path.join(bcrVersionEntryPath, "presubmit.yml")
-    );
+      sourceTemplate.save(path.join(bcrVersionEntryPath, "source.json"));
+      moduleFile.save(path.join(bcrVersionEntryPath, "MODULE.bazel"));
+
+      fs.copyFileSync(
+        rulesetRepo.presubmitPath(moduleRoot),
+        path.join(bcrVersionEntryPath, "presubmit.yml")
+      );
+    } finally {
+      releaseArchive.cleanup();
+    }
   }
 
   public async commitEntryToNewBranch(
@@ -232,7 +237,7 @@ export class CreateEntryService {
     if (moduleFile.version !== version) {
       console.log(
         `Archived MODULE.bazel version ${moduleFile.version} does not match release version ${version}.`,
-        "Creating a version patch.",
+        "Creating a version patch."
       );
       const patchFileName = "module_dot_bazel_version.patch";
       const existingContent = moduleFile.content;
diff --git a/src/domain/release-archive.spec.ts b/src/domain/release-archive.spec.ts
@@ -1,6 +1,5 @@
 import axios from "axios";
 import axiosRetry from "axios-retry";
-import extractZip from "extract-zip";
 import { mocked } from "jest-mock";
 import fs, { WriteStream } from "node:fs";
 import os from "node:os";
@@ -10,6 +9,7 @@ import { fakeModuleFile } from "../test/mock-template-files";
 import { expectThrownError } from "../test/util";
 import {
   ArchiveDownloadError,
+  MissingModuleFileError,
   ReleaseArchive,
   UnsupportedArchiveFormat,
 } from "./release-archive";
@@ -24,6 +24,7 @@ jest.mock("extract-zip");
 const RELEASE_ARCHIVE_URL = "https://foo.bar/rules-foo-v1.2.3.tar.gz";
 const STRIP_PREFIX = "rules-foo";
 const TEMP_DIR = "/tmp";
+const EXTRACT_DIR = `${TEMP_DIR}/archive-1234`;
 
 beforeEach(() => {
   jest.clearAllMocks();
@@ -50,6 +51,9 @@ beforeEach(() => {
   );
 
   mocked(os.tmpdir).mockReturnValue(TEMP_DIR);
+  mocked(fs.mkdtempSync).mockReturnValue(EXTRACT_DIR);
+
+  mocked(fs.existsSync).mockReturnValue(true); // Existence check on MODULE.bazel
 });
 
 describe("fetch", () => {
@@ -73,7 +77,7 @@ describe("fetch", () => {
   test("saves the archive to disk", async () => {
     await ReleaseArchive.fetch(RELEASE_ARCHIVE_URL, STRIP_PREFIX);
 
-    const expectedPath = path.join(TEMP_DIR, "rules-foo-v1.2.3.tar.gz");
+    const expectedPath = path.join(EXTRACT_DIR, "rules-foo-v1.2.3.tar.gz");
     expect(fs.createWriteStream).toHaveBeenCalledWith(expectedPath, {
       flags: "w",
     });
@@ -94,7 +98,7 @@ describe("fetch", () => {
       STRIP_PREFIX
     );
 
-    const expectedPath = path.join(TEMP_DIR, "rules-foo-v1.2.3.tar.gz");
+    const expectedPath = path.join(EXTRACT_DIR, "rules-foo-v1.2.3.tar.gz");
     expect(releaseArchive.diskPath).toEqual(expectedPath);
   });
 
@@ -161,42 +165,7 @@ describe("extractModuleFile", () => {
     expect(tar.x).toHaveBeenCalledWith({
       cwd: path.dirname(releaseArchive.diskPath),
       file: releaseArchive.diskPath,
-      strip: 1,
-    });
-  });
-
-  test("extracts a tarball when the strip_prefix is empty", async () => {
-    const releaseArchive = await ReleaseArchive.fetch(
-      "https://foo.bar/rules-foo-v1.2.3.tar.gz",
-      ""
-    );
-    await releaseArchive.extractModuleFile(".");
-
-    expect(tar.x).toHaveBeenCalledWith({
-      cwd: path.dirname(releaseArchive.diskPath),
-      file: releaseArchive.diskPath,
-      strip: 0,
-    });
-  });
-
-  test("extracts the full zip archive next to the zip archive", async () => {
-    const releaseArchive = await ReleaseArchive.fetch(
-      "https://foo.bar/rules-foo-v1.2.3.zip",
-      STRIP_PREFIX
-    );
-    await releaseArchive.extractModuleFile(".");
-
-    expect(extractZip).toHaveBeenCalledWith(releaseArchive.diskPath, {
-      dir: path.dirname(releaseArchive.diskPath),
     });
-    expect(fs.copyFileSync).toHaveBeenCalledWith(
-      path.join(
-        path.dirname(releaseArchive.diskPath),
-        STRIP_PREFIX,
-        "MODULE.bazel"
-      ),
-      path.join(path.dirname(releaseArchive.diskPath), "MODULE.bazel")
-    );
   });
 
   test("loads the extracted MODULE.bazel file", async () => {
@@ -208,6 +177,7 @@ describe("extractModuleFile", () => {
 
     const expectedPath = path.join(
       path.dirname(releaseArchive.diskPath),
+      STRIP_PREFIX,
       "MODULE.bazel"
     );
     expect(fs.readFileSync).toHaveBeenCalledWith(expectedPath, "utf8");
@@ -222,6 +192,7 @@ describe("extractModuleFile", () => {
 
     const expectedPath = path.join(
       path.dirname(releaseArchive.diskPath),
+      STRIP_PREFIX,
       "sub",
       "dir",
       "MODULE.bazel"
@@ -239,4 +210,17 @@ describe("extractModuleFile", () => {
     expect(moduleFile.moduleName).toEqual("rules_foo");
     expect(moduleFile.version).toEqual("1.2.3");
   });
+
+  test("throws when MODULE.bazel cannot be found in the release archive", async () => {
+    const releaseArchive = await ReleaseArchive.fetch(
+      RELEASE_ARCHIVE_URL,
+      STRIP_PREFIX
+    );
+
+    mocked(fs.existsSync).mockReturnValue(false);
+
+    await expect(releaseArchive.extractModuleFile(".")).rejects.toThrow(
+      MissingModuleFileError
+    );
+  });
 });
diff --git a/src/domain/release-archive.ts b/src/domain/release-archive.ts

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`{`
`2`	`2`	`"integrity": "",`
`3`		`- "strip_prefix": "{REPO}-{VERSION}",`
	`3`	`+ "strip_prefix": "{REPO}-{VERSION}/submodule",`
`4`	`4`	`"url": "https://github.com/{OWNER}/{REPO}/releases/download/{TAG}/module-{TAG}.tar.gz"`
`5`	`5`	`}`
Original file line number	Diff line number	Diff line change
`@@ -41,17 +41,10 @@ export async function fetchEmails(`
`41`	`41`	`)) {`
`42`	`42`	`messages.push(await mailparser.simpleParser(message.source, {}));`
`43`	`43`	`}`
	`44`	`+ await emailClient.messageFlagsAdd({ seq: "1:*", seen: false }, ["\\Seen"]);`
`44`	`45`	`} finally {`
`45`	`46`	`await emailClient.mailboxClose();`
`46`	`47`	`}`
`47`	`48`
`48`	`49`	`return messages;`
`49`	`50`	`}`
`50`		`-`
`51`		`-export async function deleteAllMail(emailClient: ImapFlow): Promise<void> {`
`52`		`- try {`
`53`		`- await emailClient.messageDelete("1:*");`
`54`		`- } finally {`
`55`		`- await emailClient.mailboxClose();`
`56`		`- }`
`57`		`-}`