feat(aio): implement a way to check and update a PR\'s preview visibility

Previously, `BuildCreator#changePrVisibility()` would throw an error if the PR's
visibility was already up-to-date or if the PR directory did not exist (e.g. was
removed). This method was only used from inside `BuildCreator#create()`, which
had already checked for the existence of the directories.

This commit renames `changePrVisibility()` to `updatePrVisibility()` and makes
it more "forgiving" (i.e. it will only throw if both public and non-public
directories exist). This allows it to be used on events that may or may not have
caused the PR's visibility to change (e.g. a GitHub webhook triggered whenever a
PR's labels change).

aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts

@@ -18,45 +18,17 @@ export class BuildCreator extends EventEmitter {
   }
 
   // Methods - Public
-  public changePrVisibility(pr: string, makePublic: boolean): Promise<void> {
-    const {oldPrDir, newPrDir} = this.getCandidatePrDirs(pr, makePublic);
-
-    return Promise.
-      all([this.exists(oldPrDir), this.exists(newPrDir)]).
-      then(([oldPrDirExisted, newPrDirExisted]) => {
-        if (!oldPrDirExisted) {
-          throw new UploadError(404, `Request to move non-existing directory '${oldPrDir}' to '${newPrDir}'.`);
-        } else if (newPrDirExisted) {
-          throw new UploadError(409, `Request to move '${oldPrDir}' to existing directory '${newPrDir}'.`);
-        }
-
-        return Promise.resolve().
-          then(() => shell.mv(oldPrDir, newPrDir)).
-          then(() => this.listShasByDate(newPrDir)).
-          then(shas => this.emit(ChangedPrVisibilityEvent.type, new ChangedPrVisibilityEvent(+pr, shas, makePublic))).
-          then(() => undefined);
-      }).
-      catch(err => {
-        if (!(err instanceof UploadError)) {
-          err = new UploadError(500, `Error while making PR ${pr} ${makePublic ? 'public' : 'hidden'}.\n${err}`);
-        }
-
-        throw err;
-      });
-  }
-
   public create(pr: string, sha: string, archivePath: string, isPublic: boolean): Promise<void> {
     // Use only part of the SHA for more readable URLs.
     sha = sha.substr(0, SHORT_SHA_LEN);
 
-    const {oldPrDir: otherVisPrDir, newPrDir: prDir} = this.getCandidatePrDirs(pr, isPublic);
+    const {newPrDir: prDir} = this.getCandidatePrDirs(pr, isPublic);
     const shaDir = path.join(prDir, sha);
     let dirToRemoveOnError: string;
 
     return Promise.resolve().
-      then(() => this.exists(otherVisPrDir)).
       // If the same PR exists with different visibility, update the visibility first.
-      then(otherVisPrDirExisted => (otherVisPrDirExisted && this.changePrVisibility(pr, isPublic)) as any).
+      then(() => this.updatePrVisibility(pr, isPublic)).
       then(() => Promise.all([this.exists(prDir), this.exists(shaDir)])).
       then(([prDirExisted, shaDirExisted]) => {
         if (shaDirExisted) {
@@ -84,6 +56,36 @@ export class BuildCreator extends EventEmitter {
       });
   }
 
+  public updatePrVisibility(pr: string, makePublic: boolean): Promise<boolean> {
+    const {oldPrDir: otherVisPrDir, newPrDir: targetVisPrDir} = this.getCandidatePrDirs(pr, makePublic);
+
+    return Promise.
+      all([this.exists(otherVisPrDir), this.exists(targetVisPrDir)]).
+      then(([otherVisPrDirExisted, targetVisPrDirExisted]) => {
+        if (!otherVisPrDirExisted) {
+          // No visibility change: Either the visibility is up-to-date or the PR does not exist.
+          return false;
+        } else if (targetVisPrDirExisted) {
+          // Error: Directories for both visibilities exist.
+          throw new UploadError(409, `Request to move '${otherVisPrDir}' to existing directory '${targetVisPrDir}'.`);
+        }
+
+        // Visibility change: Moving `otherVisPrDir` to `targetVisPrDir`.
+        return Promise.resolve().
+          then(() => shell.mv(otherVisPrDir, targetVisPrDir)).
+          then(() => this.listShasByDate(targetVisPrDir)).
+          then(shas => this.emit(ChangedPrVisibilityEvent.type, new ChangedPrVisibilityEvent(+pr, shas, makePublic))).
+          then(() => true);
+      }).
+      catch(err => {
+        if (!(err instanceof UploadError)) {
+          err = new UploadError(500, `Error while making PR ${pr} ${makePublic ? 'public' : 'hidden'}.\n${err}`);
+        }
+
+        throw err;
+      });
+  }
+
   // Methods - Protected
   protected exists(fileOrDir: string): Promise<boolean> {
     return new Promise(resolve => fs.access(fileOrDir, err => resolve(!err)));

aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-creator.spec.ts

@@ -43,178 +43,25 @@ describe('BuildCreator', () => {
   });
 
 
-  describe('changePrVisibility()', () => {
-    let bcEmitSpy: jasmine.Spy;
-    let bcExistsSpy: jasmine.Spy;
-    let bcListShasByDate: jasmine.Spy;
-    let shellMvSpy: jasmine.Spy;
-
-    beforeEach(() => {
-      bcEmitSpy = spyOn(bc, 'emit');
-      bcExistsSpy = spyOn(bc as any, 'exists');
-      bcListShasByDate = spyOn(bc as any, 'listShasByDate');
-      shellMvSpy = spyOn(shell, 'mv');
-
-      bcExistsSpy.and.returnValues(Promise.resolve(true), Promise.resolve(false));
-      bcListShasByDate.and.returnValue([]);
-    });
-
-
-    it('should return a promise', done => {
-      const promise = bc.changePrVisibility(pr, true);
-      promise.then(done);   // Do not complete the test (and release the spies) synchronously
-                            // to avoid running the actual `extractArchive()`.
-
-      expect(promise).toEqual(jasmine.any(Promise));
-    });
-
-
-    [true, false].forEach(makePublic => {
-      const oldPrDir = makePublic ? hiddenPrDir : publicPrDir;
-      const newPrDir = makePublic ? publicPrDir : hiddenPrDir;
-
-
-      it('should rename the directory', done => {
-        bc.changePrVisibility(pr, makePublic).
-          then(() => expect(shellMvSpy).toHaveBeenCalledWith(oldPrDir, newPrDir)).
-          then(done);
-      });
-
-
-      it('should emit a ChangedPrVisibilityEvent on success', done => {
-        let emitted = false;
-
-        bcEmitSpy.and.callFake((type: string, evt: ChangedPrVisibilityEvent) => {
-          expect(type).toBe(ChangedPrVisibilityEvent.type);
-          expect(evt).toEqual(jasmine.any(ChangedPrVisibilityEvent));
-          expect(evt.pr).toBe(+pr);
-          expect(evt.shas).toEqual(jasmine.any(Array));
-          expect(evt.isPublic).toBe(makePublic);
-
-          emitted = true;
-        });
-
-        bc.changePrVisibility(pr, makePublic).
-          then(() => expect(emitted).toBe(true)).
-          then(done);
-      });
-
-
-      it('should include all shas in the emitted event', done => {
-        const shas = ['foo', 'bar', 'baz'];
-        let emitted = false;
-
-        bcListShasByDate.and.returnValue(Promise.resolve(shas));
-        bcEmitSpy.and.callFake((type: string, evt: ChangedPrVisibilityEvent) => {
-          expect(bcListShasByDate).toHaveBeenCalledWith(newPrDir);
-
-          expect(type).toBe(ChangedPrVisibilityEvent.type);
-          expect(evt).toEqual(jasmine.any(ChangedPrVisibilityEvent));
-          expect(evt.pr).toBe(+pr);
-          expect(evt.shas).toBe(shas);
-          expect(evt.isPublic).toBe(makePublic);
-
-          emitted = true;
-        });
-
-        bc.changePrVisibility(pr, makePublic).
-          then(() => expect(emitted).toBe(true)).
-          then(done);
-      });
-
-
-      describe('on error', () => {
-
-        it('should abort and skip further operations if the old directory does not exist', done => {
-          bcExistsSpy.and.callFake((dir: string) => dir !== oldPrDir);
-          bc.changePrVisibility(pr, makePublic).catch(err => {
-            expectToBeUploadError(err, 404, `Request to move non-existing directory '${oldPrDir}' to '${newPrDir}'.`);
-            expect(shellMvSpy).not.toHaveBeenCalled();
-            expect(bcListShasByDate).not.toHaveBeenCalled();
-            expect(bcEmitSpy).not.toHaveBeenCalled();
-            done();
-          });
-        });
-
-
-        it('should abort and skip further operations if the new directory does already exist', done => {
-          bcExistsSpy.and.returnValue(true);
-          bc.changePrVisibility(pr, makePublic).catch(err => {
-            expectToBeUploadError(err, 409, `Request to move '${oldPrDir}' to existing directory '${newPrDir}'.`);
-            expect(shellMvSpy).not.toHaveBeenCalled();
-            expect(bcListShasByDate).not.toHaveBeenCalled();
-            expect(bcEmitSpy).not.toHaveBeenCalled();
-            done();
-          });
-        });
-
-
-        it('should abort and skip further operations if it fails to rename the directory', done => {
-          shellMvSpy.and.throwError('');
-          bc.changePrVisibility(pr, makePublic).catch(() => {
-            expect(shellMvSpy).toHaveBeenCalled();
-            expect(bcListShasByDate).not.toHaveBeenCalled();
-            expect(bcEmitSpy).not.toHaveBeenCalled();
-            done();
-          });
-        });
-
-
-        it('should abort and skip further operations if it fails to list the SHAs', done => {
-          bcListShasByDate.and.throwError('');
-          bc.changePrVisibility(pr, makePublic).catch(() => {
-            expect(shellMvSpy).toHaveBeenCalled();
-            expect(bcListShasByDate).toHaveBeenCalled();
-            expect(bcEmitSpy).not.toHaveBeenCalled();
-            done();
-          });
-        });
-
-
-        it('should reject with an UploadError', done => {
-          shellMvSpy.and.callFake(() => { throw 'Test'; });
-          bc.changePrVisibility(pr, makePublic).catch(err => {
-            expectToBeUploadError(err, 500, `Error while making PR ${pr} ${makePublic ? 'public' : 'hidden'}.\nTest`);
-            done();
-          });
-        });
-
-
-        it('should pass UploadError instances unmodified', done => {
-          shellMvSpy.and.callFake(() => { throw new UploadError(543, 'Test'); });
-          bc.changePrVisibility(pr, makePublic).catch(err => {
-            expectToBeUploadError(err, 543, 'Test');
-            done();
-          });
-        });
-
-      });
-
-    });
-
-  });
-
-
   describe('create()', () => {
-    let bcChangePrVisibilitySpy: jasmine.Spy;
     let bcEmitSpy: jasmine.Spy;
     let bcExistsSpy: jasmine.Spy;
     let bcExtractArchiveSpy: jasmine.Spy;
+    let bcUpdatePrVisibilitySpy: jasmine.Spy;
     let shellMkdirSpy: jasmine.Spy;
     let shellRmSpy: jasmine.Spy;
 
     beforeEach(() => {
-      bcChangePrVisibilitySpy = spyOn(bc, 'changePrVisibility');
       bcEmitSpy = spyOn(bc, 'emit');
       bcExistsSpy = spyOn(bc as any, 'exists');
       bcExtractArchiveSpy = spyOn(bc as any, 'extractArchive');
+      bcUpdatePrVisibilitySpy = spyOn(bc, 'updatePrVisibility');
       shellMkdirSpy = spyOn(shell, 'mkdir');
       shellRmSpy = spyOn(shell, 'rm');
     });
 
 
     [true, false].forEach(isPublic => {
-      const otherVisPrDir = isPublic ? hiddenPrDir : publicPrDir;
       const prDir = isPublic ? publicPrDir : hiddenPrDir;
       const shaDir = isPublic ? publicShaDir : hiddenShaDir;
 
@@ -228,20 +75,12 @@ describe('BuildCreator', () => {
       });
 
 
-      it('should not update the PR\'s visibility first if not necessary', done => {
-        bc.create(pr, sha, archive, isPublic).
-          then(() => expect(bcChangePrVisibilitySpy).not.toHaveBeenCalled()).
-          then(done);
-      });
-
-
       it('should update the PR\'s visibility first if necessary', done => {
-        bcChangePrVisibilitySpy.and.callFake(() => expect(shellMkdirSpy).not.toHaveBeenCalled());
+        bcUpdatePrVisibilitySpy.and.callFake(() => expect(shellMkdirSpy).not.toHaveBeenCalled());
-        bcExistsSpy.and.callFake((dir: string) => dir === otherVisPrDir);
 
         bc.create(pr, sha, archive, isPublic).
           then(() => {
-            expect(bcChangePrVisibilitySpy).toHaveBeenCalledWith(pr, isPublic);
+            expect(bcUpdatePrVisibilitySpy).toHaveBeenCalledWith(pr, isPublic);
             expect(shellMkdirSpy).toHaveBeenCalled();
           }).
           then(done);
@@ -286,7 +125,6 @@ describe('BuildCreator', () => {
 
         beforeEach(() => {
           existsValues = {
-            [otherVisPrDir]: false,
             [prDir]: false,
             [shaDir]: false,
           };
@@ -297,14 +135,12 @@ describe('BuildCreator', () => {
 
         it('should abort and skip further operations if changing the PR\'s visibility fails', done => {
           const mockError = new UploadError(543, 'Test');
-
+          bcUpdatePrVisibilitySpy.and.returnValue(Promise.reject(mockError));
-          existsValues[otherVisPrDir] = true;
-          bcChangePrVisibilitySpy.and.returnValue(Promise.reject(mockError));
 
           bc.create(pr, sha, archive, isPublic).catch(err => {
             expect(err).toBe(mockError);
 
-            expect(bcExistsSpy).toHaveBeenCalledTimes(1);
+            expect(bcExistsSpy).not.toHaveBeenCalled();
             expect(shellMkdirSpy).not.toHaveBeenCalled();
             expect(bcExtractArchiveSpy).not.toHaveBeenCalled();
             expect(bcEmitSpy).not.toHaveBeenCalled();
@@ -327,8 +163,10 @@ describe('BuildCreator', () => {
 
 
         it('should detect existing build directory after visibility change', done => {
-          existsValues[otherVisPrDir] = true;
+          bcUpdatePrVisibilitySpy.and.callFake(() => existsValues[prDir] = existsValues[shaDir] = true);
-          bcChangePrVisibilitySpy.and.callFake(() => existsValues[prDir] = existsValues[shaDir] = true);
+
+          expect(bcExistsSpy(prDir)).toBe(false);
+          expect(bcExistsSpy(shaDir)).toBe(false);
 
           bc.create(pr, sha, archive, isPublic).catch(err => {
             expectToBeUploadError(err, 409, `Request to overwrite existing directory: ${shaDir}`);
@@ -406,6 +244,190 @@ describe('BuildCreator', () => {
   });
 
 
+  describe('updatePrVisibility()', () => {
+    let bcEmitSpy: jasmine.Spy;
+    let bcExistsSpy: jasmine.Spy;
+    let bcListShasByDate: jasmine.Spy;
+    let shellMvSpy: jasmine.Spy;
+
+    beforeEach(() => {
+      bcEmitSpy = spyOn(bc, 'emit');
+      bcExistsSpy = spyOn(bc as any, 'exists');
+      bcListShasByDate = spyOn(bc as any, 'listShasByDate');
+      shellMvSpy = spyOn(shell, 'mv');
+
+      bcExistsSpy.and.returnValues(Promise.resolve(true), Promise.resolve(false));
+      bcListShasByDate.and.returnValue([]);
+    });
+
+
+    it('should return a promise', done => {
+      const promise = bc.updatePrVisibility(pr, true);
+      promise.then(done);   // Do not complete the test (and release the spies) synchronously
+                            // to avoid running the actual `extractArchive()`.
+
+      expect(promise).toEqual(jasmine.any(Promise));
+    });
+
+
+    [true, false].forEach(makePublic => {
+      const oldPrDir = makePublic ? hiddenPrDir : publicPrDir;
+      const newPrDir = makePublic ? publicPrDir : hiddenPrDir;
+
+
+      it('should rename the directory', done => {
+        bc.updatePrVisibility(pr, makePublic).
+          then(() => expect(shellMvSpy).toHaveBeenCalledWith(oldPrDir, newPrDir)).
+          then(done);
+      });
+
+
+      describe('when the visibility is updated', () => {
+
+        it('should resolve to true', done => {
+          bc.updatePrVisibility(pr, makePublic).
+            then(result => expect(result).toBe(true)).
+            then(done);
+        });
+
+
+        it('should rename the directory', done => {
+          bc.updatePrVisibility(pr, makePublic).
+            then(() => expect(shellMvSpy).toHaveBeenCalledWith(oldPrDir, newPrDir)).
+            then(done);
+        });
+
+
+        it('should emit a ChangedPrVisibilityEvent on success', done => {
+          let emitted = false;
+
+          bcEmitSpy.and.callFake((type: string, evt: ChangedPrVisibilityEvent) => {
+            expect(type).toBe(ChangedPrVisibilityEvent.type);
+            expect(evt).toEqual(jasmine.any(ChangedPrVisibilityEvent));
+            expect(evt.pr).toBe(+pr);
+            expect(evt.shas).toEqual(jasmine.any(Array));
+            expect(evt.isPublic).toBe(makePublic);
+
+            emitted = true;
+          });
+
+          bc.updatePrVisibility(pr, makePublic).
+            then(() => expect(emitted).toBe(true)).
+            then(done);
+        });
+
+
+        it('should include all shas in the emitted event', done => {
+          const shas = ['foo', 'bar', 'baz'];
+          let emitted = false;
+
+          bcListShasByDate.and.returnValue(Promise.resolve(shas));
+          bcEmitSpy.and.callFake((type: string, evt: ChangedPrVisibilityEvent) => {
+            expect(bcListShasByDate).toHaveBeenCalledWith(newPrDir);
+
+            expect(type).toBe(ChangedPrVisibilityEvent.type);
+            expect(evt).toEqual(jasmine.any(ChangedPrVisibilityEvent));
+            expect(evt.pr).toBe(+pr);
+            expect(evt.shas).toBe(shas);
+            expect(evt.isPublic).toBe(makePublic);
+
+            emitted = true;
+          });
+
+          bc.updatePrVisibility(pr, makePublic).
+            then(() => expect(emitted).toBe(true)).
+            then(done);
+        });
+
+      });
+
+
+      it('should do nothing if the visibility is already up-to-date', done => {
+        bcExistsSpy.and.callFake((dir: string) => dir === newPrDir);
+        bc.updatePrVisibility(pr, makePublic).
+          then(result => {
+            expect(result).toBe(false);
+            expect(shellMvSpy).not.toHaveBeenCalled();
+            expect(bcListShasByDate).not.toHaveBeenCalled();
+            expect(bcEmitSpy).not.toHaveBeenCalled();
+          }).
+          then(done);
+      });
+
+
+      it('should do nothing if the PR directory does not exist', done => {
+        bcExistsSpy.and.returnValue(false);
+        bc.updatePrVisibility(pr, makePublic).
+          then(result => {
+            expect(result).toBe(false);
+            expect(shellMvSpy).not.toHaveBeenCalled();
+            expect(bcListShasByDate).not.toHaveBeenCalled();
+            expect(bcEmitSpy).not.toHaveBeenCalled();
+          }).
+          then(done);
+      });
+
+
+      describe('on error', () => {
+
+        it('should abort and skip further operations if both directories exist', done => {
+          bcExistsSpy.and.returnValue(true);
+          bc.updatePrVisibility(pr, makePublic).catch(err => {
+            expectToBeUploadError(err, 409, `Request to move '${oldPrDir}' to existing directory '${newPrDir}'.`);
+            expect(shellMvSpy).not.toHaveBeenCalled();
+            expect(bcListShasByDate).not.toHaveBeenCalled();
+            expect(bcEmitSpy).not.toHaveBeenCalled();
+            done();
+          });
+        });
+
+
+        it('should abort and skip further operations if it fails to rename the directory', done => {
+          shellMvSpy.and.throwError('');
+          bc.updatePrVisibility(pr, makePublic).catch(() => {
+            expect(shellMvSpy).toHaveBeenCalled();
+            expect(bcListShasByDate).not.toHaveBeenCalled();
+            expect(bcEmitSpy).not.toHaveBeenCalled();
+            done();
+          });
+        });
+
+
+        it('should abort and skip further operations if it fails to list the SHAs', done => {
+          bcListShasByDate.and.throwError('');
+          bc.updatePrVisibility(pr, makePublic).catch(() => {
+            expect(shellMvSpy).toHaveBeenCalled();
+            expect(bcListShasByDate).toHaveBeenCalled();
+            expect(bcEmitSpy).not.toHaveBeenCalled();
+            done();
+          });
+        });
+
+
+        it('should reject with an UploadError', done => {
+          shellMvSpy.and.callFake(() => { throw 'Test'; });
+          bc.updatePrVisibility(pr, makePublic).catch(err => {
+            expectToBeUploadError(err, 500, `Error while making PR ${pr} ${makePublic ? 'public' : 'hidden'}.\nTest`);
+            done();
+          });
+        });
+
+
+        it('should pass UploadError instances unmodified', done => {
+          shellMvSpy.and.callFake(() => { throw new UploadError(543, 'Test'); });
+          bc.updatePrVisibility(pr, makePublic).catch(err => {
+            expectToBeUploadError(err, 543, 'Test');
+            done();
+          });
+        });
+
+      });
+
+    });
+
+  });
+
+
   // Protected methods
 
   describe('exists()', () => {

aio/aio-builds-setup/docs/overview--http-status-codes.md

@@ -42,10 +42,6 @@ with a bried explanation of what they mean:
 - **403 (Forbidden)**:
   Unable to verify build (e.g. invalid JWT token, or unable to talk to 3rd-party APIs, etc).
 
-- **404 (Not Found)**:
-  Tried to change PR visibility but the source directory did not exist.
-  (Currently, this can only happen as a rare race condition during build deployment.)
-
 - **405 (Method Not Allowed)**:
   Request method other than POST.