chore: address some review feedback

Author: codebytere

src/build-images-handler.ts

@@ -15,7 +15,26 @@ const files = [
   '.devcontainer/docker-compose.yml',
 ];
 
-export async function getPreviousTargetOid(payload: RegistryPackagePublishedEvent) {
+export async function shouldUpdateFiles(octokit: Octokit, oid: string) {
+  const { data: file } = await octokit.rest.repos.getContent({
+    ...REPOS.electron,
+    path: files[0],
+  });
+
+  if (!('content' in file)) {
+    throw new Error(`Incorrectly received array when fetching content for ${files[0]}`);
+  }
+
+  const fileContent = Buffer.from(file.content, 'base64').toString('utf-8');
+  const match = fileContent.match(oid);
+  if (match?.[0] === oid) {
+    return false;
+  }
+
+  return true;
+}
+
+export async function getPreviousOid(payload: RegistryPackagePublishedEvent) {
   const { registry_package, organization } = payload;
   const { target_oid, name } = registry_package.package_version;
 
@@ -42,15 +61,14 @@ export async function getPreviousTargetOid(payload: RegistryPackagePublishedEven
 
 export async function updateFilesWithNewOid(
   octokit: Octokit,
-  filePaths: string[],
-  previousTargetOid: string,
+  previousOid: string,
   targetOid: string,
   branchName: string,
 ) {
   const d = debug(`roller/github:updateFilesWithNewOid`);
   let updatedAny = false;
 
-  for (const filePath of filePaths) {
+  for (const filePath of files) {
     try {
       const { data: file } = await octokit.rest.repos.getContent({
         ...REPOS.electron,
@@ -62,20 +80,14 @@ export async function updateFilesWithNewOid(
       }
 
       const fileContent = Buffer.from(file.content, 'base64').toString('utf-8');
-      const match = fileContent.match(previousTargetOid);
+      const match = fileContent.match(previousOid);
       if (!match) {
         d(`No match found for ${filePath}`);
         continue;
       }
 
-      const currentSha = match[0];
-      if (currentSha === targetOid) {
-        d(`No update needed for ${filePath}`);
-        continue;
-      }
-
-      d(`Updating ${filePath} from ${currentSha} to ${targetOid}`);
-      const newContent = fileContent.replace(currentSha, targetOid);
+      d(`Updating ${filePath} from ${match[0]} to ${targetOid}`);
+      const newContent = fileContent.replace(match[0], targetOid);
       await octokit.rest.repos.createOrUpdateFileContents({
         ...REPOS.electron,
         path: filePath,
@@ -125,46 +137,75 @@ export async function handleBuildImagesCheck(payload: RegistryPackagePublishedEv
   const d = debug(`roller/github:handleBuildImagesCheck`);
   const octokit = await getOctokit();
 
-  const { target_oid } = payload.registry_package.package_version;
-  const previousTargetOid = await getPreviousTargetOid(payload);
-  console.log('Previous target OID:', previousTargetOid);
+  const { target_oid: targetOid } = payload.registry_package.package_version;
+  const previousOid = await getPreviousOid(payload);
 
-  if (!previousTargetOid) {
+  if (!previousOid) {
     d('No previous target OID found, cannot proceed with updates');
     return;
   }
 
   const branchName = `roller/build-images/${MAIN_BRANCH}`;
-  const { ref, sha } = await prepareGitBranch(octokit, branchName, MAIN_BRANCH);
-
-  d(`Preparing to update files with new OID: ${target_oid}`);
-  const updatedFiles = await updateFilesWithNewOid(
-    octokit,
-    files,
-    previousTargetOid,
-    target_oid,
-    branchName,
-  );
-
-  if (!updatedFiles) {
-    d('No files were updated, skipping PR creation');
-    return;
-  }
 
-  d(`Creating ref=${ref} at sha=${sha}`);
-  await octokit.rest.git.createRef({ ...REPOS.electron, ref, sha });
-
-  d(`Raising a PR for ${branchName} to ${MAIN_BRANCH}`);
-  const pr = await octokit.rest.pulls.create({
+  // Look for a pre-existing PR that targets this branch to see if we can update that.
+  const { data: prs } = await octokit.rest.pulls.list({
     ...REPOS.electron,
-    base: MAIN_BRANCH,
     head: `${REPOS.electron.owner}:${branchName}`,
-    title: `build: update build-images to ${target_oid.slice(0, 7)}`,
-    body: `This PR updates the build-images references from ${previousTargetOid.slice(
-      0,
-      7,
-    )} to ${target_oid.slice(0, 7)}.`,
+    state: 'open',
   });
 
-  d(`New PR: ${pr.data.html_url}`);
+  if (prs.length > 0) {
+    const pr = prs[0];
+    const oid = targetOid.slice(0, 7);
+
+    d(`Found existing PR: #${pr.number} opened by ${pr.user.login} - updating`);
+
+    d(`Preparing to update files with new OID: ${targetOid}`);
+    const updatedFiles = await updateFilesWithNewOid(octokit, previousOid, targetOid, branchName);
+
+    if (!updatedFiles) {
+      d('No files were updated, skipping PR update');
+      return;
+    }
+
+    await octokit.rest.pulls.update({
+      ...REPOS.electron,
+      pull_number: pr.number,
+      title: `chore: bump build image tag to ${oid}`,
+      body: `This PR updates the build-images references from ${previousOid.slice(
+        0,
+        7,
+      )} to ${oid}.`,
+    });
+    return;
+  } else {
+    d(`No existing PR found for ${branchName} - creating a new one`);
+    const { ref, sha } = await prepareGitBranch(octokit, branchName, MAIN_BRANCH);
+
+    const shouldUpdate = await shouldUpdateFiles(octokit, targetOid);
+    if (!shouldUpdate) {
+      d('Build images are up to date - skipping PR creation');
+      return;
+    }
+
+    d(`Creating ref=${ref} at sha=${sha}`);
+    await octokit.rest.git.createRef({ ...REPOS.electron, ref, sha });
+
+    d(`Preparing to update files with new OID: ${targetOid}`);
+    await updateFilesWithNewOid(octokit, previousOid, targetOid, branchName);
+
+    d(`Raising a PR for ${branchName} to ${MAIN_BRANCH}`);
+    const pr = await octokit.rest.pulls.create({
+      ...REPOS.electron,
+      base: MAIN_BRANCH,
+      head: `${REPOS.electron.owner}:${branchName}`,
+      title: `build: update build-images to ${targetOid.slice(0, 7)}`,
+      body: `This PR updates the build-images references from ${previousOid.slice(
+        0,
+        7,
+      )} to ${targetOid.slice(0, 7)}.`,
+    });
+
+    d(`New PR: ${pr.data.html_url}`);
+  }
 }

tests/build-images.test.ts

@@ -5,6 +5,7 @@ import { getOctokit } from '../src/utils/octokit';
 
 import { Probot, ProbotOctokit } from 'probot';
 import { MAIN_BRANCH } from '../src/constants';
+import { randomBytes } from 'node:crypto';
 const nock = require('nock');
 
 const GH_API = 'https://api.github.com';
@@ -99,6 +100,9 @@ describe('build-images', () => {
         },
         pulls: {
           create: vi.fn(),
+          list: vi.fn().mockResolvedValue({
+            data: [],
+          }),
         },
       },
     };
@@ -143,7 +147,7 @@ describe('build-images', () => {
     const payload = JSON.parse(JSON.stringify(payloadJson));
     payload.registry_package.package_version.target_oid = 'newtargetoid123';
 
-    const previousTargetOid = await buildImagesHandler.getPreviousTargetOid(payload);
+    const previousTargetOid = await buildImagesHandler.getPreviousOid(payload);
     expect(previousTargetOid).toBe('424eedbf277ad9749ffa9219068aa72ed4a5e373');
   });
 
@@ -222,14 +226,13 @@ describe('build-images', () => {
 
       const result = await buildImagesHandler.updateFilesWithNewOid(
         mockOctokit,
-        ['.github/workflows/linux-publish.yml'],
         'oldsha123',
         'newsha456',
         branchName,
       );
 
       expect(result).toBe(true);
-      expect(mockOctokit.rest.repos.getContent).toHaveBeenCalledTimes(1);
+      expect(mockOctokit.rest.repos.getContent).toHaveBeenCalledTimes(7);
       expect(mockOctokit.rest.repos.createOrUpdateFileContents).toHaveBeenCalledTimes(1);
       expect(mockOctokit.rest.repos.createOrUpdateFileContents).toHaveBeenCalledWith(
         expect.objectContaining({
@@ -251,7 +254,6 @@ describe('build-images', () => {
 
       const result = await buildImagesHandler.updateFilesWithNewOid(
         mockOctokit,
-        ['.github/workflows/linux-publish.yml'],
         'oldsha123',
         'newsha456',
         branchName,
@@ -269,6 +271,8 @@ describe('build-images', () => {
         data: { html_url: 'https://github.com/electron/electron/pull/123' },
       });
 
+      mockOctokit.rest.pulls.list.mockResolvedValue({ data: [] });
+
       mockOctokit.rest.repos.getContent.mockImplementation(() => {
         return {
           data: {
@@ -305,7 +309,14 @@ describe('build-images', () => {
     });
 
     it('skips PR creation when no files need updating', async () => {
-      vi.spyOn(buildImagesHandler, 'getPreviousTargetOid').mockResolvedValue('oldsha123');
+      const newSha = randomBytes(20).toString('hex');
+
+      mockOctokit.rest.pulls.list.mockResolvedValue({ data: [] });
+      mockOctokit.rest.repos.getContent.mockResolvedValue({
+        data: { content: Buffer.from(newSha).toString('base64') },
+      });
+
+      vi.spyOn(buildImagesHandler, 'getPreviousOid').mockResolvedValue('oldsha123');
 
       vi.spyOn(buildImagesHandler, 'prepareGitBranch').mockResolvedValue({
         ref: `refs/heads/${branchName}`,
@@ -317,7 +328,7 @@ describe('build-images', () => {
       vi.spyOn(buildImagesHandler, 'updateFilesWithNewOid').mockResolvedValue(false);
 
       const payload = JSON.parse(JSON.stringify(payloadJson));
-      payload.registry_package.package_version.target_oid = 'newsha456';
+      payload.registry_package.package_version.target_oid = newSha;
 
       await buildImagesHandler.handleBuildImagesCheck(payload);