Merge pull request #1535 from iclanton/ianc/fail-on-outdated-approved-packages

[rush] Make rush install error if the approved package files are out-of-date.

Author: iclanton

apps/rush-lib/src/api/ApprovedPackagesConfiguration.ts

@@ -78,17 +78,23 @@ export class ApprovedPackagesConfiguration {
     return this._itemsByName.get(packageName);
   }
 
-  public addOrUpdatePackage(packageName: string, reviewCategory: string): void {
+  public addOrUpdatePackage(packageName: string, reviewCategory: string): boolean {
+    let changed: boolean = false;
+
     let item: ApprovedPackagesItem | undefined = this._itemsByName.get(packageName);
     if (!item) {
       item = new ApprovedPackagesItem();
       item.packageName = packageName;
       this._addItem(item);
+      changed = true;
     }
 
-    if (reviewCategory) {
+    if (reviewCategory && !item.allowedCategories.has(reviewCategory)) {
       item.allowedCategories.add(reviewCategory);
+      changed = true;
     }
+
+    return changed;
   }
 
   /**

apps/rush-lib/src/api/ApprovedPackagesPolicy.ts

@@ -35,14 +35,18 @@ export class ApprovedPackagesPolicy {
     }
 
     // Load browser-approved-packages.json
-    const browserApprovedPackagesPath: string = path.join(rushConfiguration.commonRushConfigFolder,
-      RushConstants.browserApprovedPackagesFilename);
+    const browserApprovedPackagesPath: string = path.join(
+      rushConfiguration.commonRushConfigFolder,
+      RushConstants.browserApprovedPackagesFilename
+    );
     this._browserApprovedPackages = new ApprovedPackagesConfiguration(browserApprovedPackagesPath);
     this._browserApprovedPackages.tryLoadFromFile(this._enabled);
 
     // Load nonbrowser-approved-packages.json
-    const nonbrowserApprovedPackagesPath: string = path.join(rushConfiguration.commonRushConfigFolder,
-      RushConstants.nonbrowserApprovedPackagesFilename);
+    const nonbrowserApprovedPackagesPath: string = path.join(
+      rushConfiguration.commonRushConfigFolder,
+      RushConstants.nonbrowserApprovedPackagesFilename
+    );
     this._nonbrowserApprovedPackages = new ApprovedPackagesConfiguration(nonbrowserApprovedPackagesPath);
     this._nonbrowserApprovedPackages.tryLoadFromFile(this._enabled);
   }

apps/rush-lib/src/logic/ApprovedPackagesChecker.ts

@@ -9,6 +9,27 @@ import { RushConfigurationProject } from '../api/RushConfigurationProject';
 import { DependencySpecifier } from './DependencySpecifier';
 
 export class ApprovedPackagesChecker {
+  private readonly _rushConfiguration: RushConfiguration;
+  private _approvedPackagesPolicy: ApprovedPackagesPolicy;
+  private _filesAreOutOfDate: boolean;
+
+  public constructor(rushConfiguration: RushConfiguration) {
+    this._rushConfiguration = rushConfiguration;
+    this._approvedPackagesPolicy = this._rushConfiguration.approvedPackagesPolicy;
+    this._filesAreOutOfDate = false;
+
+    if (this._approvedPackagesPolicy.enabled) {
+      this._updateApprovedPackagesPolicy();
+    }
+  }
+
+  /**
+   * If true, the files on disk are out of date.
+   */
+  public get approvedPackagesFilesAreOutOfDate(): boolean {
+    return this._filesAreOutOfDate;
+  }
+
   /**
    * Examines the current dependencies for the projects specified in RushConfiguration,
    * and then adds them to the 'browser-approved-packages.json' and
@@ -17,30 +38,46 @@ export class ApprovedPackagesChecker {
    *
    * If the "approvedPackagesPolicy" feature is not enabled, then no action is taken.
    */
-  public static rewriteConfigFiles(rushConfiguration: RushConfiguration): void {
-    const approvedPackagesPolicy: ApprovedPackagesPolicy = rushConfiguration.approvedPackagesPolicy;
-    if (!approvedPackagesPolicy.enabled) {
-      return;
+  public rewriteConfigFiles(): void {
+    const approvedPackagesPolicy: ApprovedPackagesPolicy = this._rushConfiguration.approvedPackagesPolicy;
+    if (approvedPackagesPolicy.enabled) {
+      approvedPackagesPolicy.browserApprovedPackages.saveToFile();
+      approvedPackagesPolicy.nonbrowserApprovedPackages.saveToFile();
     }
+  }
 
-    for (const rushProject of rushConfiguration.projects) {
+  private _updateApprovedPackagesPolicy(): void {
+    for (const rushProject of this._rushConfiguration.projects) {
       const packageJson: IPackageJson = rushProject.packageJson;
 
-      ApprovedPackagesChecker._collectDependencies(packageJson.dependencies,
-        approvedPackagesPolicy, rushProject);
-      ApprovedPackagesChecker._collectDependencies(packageJson.optionalDependencies,
-        approvedPackagesPolicy, rushProject);
-      ApprovedPackagesChecker._collectDependencies(packageJson.devDependencies,
-        approvedPackagesPolicy, rushProject);
+      this._collectDependencies(
+        packageJson.dependencies,
+        this._approvedPackagesPolicy,
+        rushProject
+      );
+      this._collectDependencies(
+        packageJson.devDependencies,
+        this._approvedPackagesPolicy,
+        rushProject
+      );
+      this._collectDependencies(
+        packageJson.peerDependencies,
+        this._approvedPackagesPolicy,
+        rushProject
+      );
+      this._collectDependencies(
+        packageJson.optionalDependencies,
+        this._approvedPackagesPolicy,
+        rushProject
+      );
     }
-
-    approvedPackagesPolicy.browserApprovedPackages.saveToFile();
-    approvedPackagesPolicy.nonbrowserApprovedPackages.saveToFile();
   }
 
-  private static _collectDependencies(dependencies: { [key: string]: string } | undefined,
-    approvedPackagesPolicy: ApprovedPackagesPolicy, rushProject: RushConfigurationProject): void {
-
+  private _collectDependencies(
+    dependencies: { [key: string]: string } | undefined,
+    approvedPackagesPolicy: ApprovedPackagesPolicy,
+    rushProject: RushConfigurationProject
+  ): void {
     if (dependencies) {
       for (const packageName of Object.keys(dependencies)) {
 
@@ -51,8 +88,10 @@ export class ApprovedPackagesChecker {
         // "dependencies": {
         //   "alias-name": "npm:target-name@^1.2.3"
         // }
-        const dependencySpecifier: DependencySpecifier
-          = new DependencySpecifier(packageName, dependencies[packageName]);
+        const dependencySpecifier: DependencySpecifier = new DependencySpecifier(
+          packageName,
+          dependencies[packageName]
+        );
         if (dependencySpecifier.aliasTarget) {
           // Use "target-name" instead of "alias-name"
           referencedPackageName = dependencySpecifier.aliasTarget.packageName;
@@ -64,15 +103,23 @@ export class ApprovedPackagesChecker {
         if (!approvedPackagesPolicy.ignoredNpmScopes.has(scope)) {
           // Yes, add it to the list if it's not already there
 
+          let updated: boolean = false;
+
           // By default we put everything in the browser file.  But if it already appears in the
           // non-browser file, then use that instead.
           if (approvedPackagesPolicy.nonbrowserApprovedPackages.getItemByName(referencedPackageName)) {
-            approvedPackagesPolicy.nonbrowserApprovedPackages
-              .addOrUpdatePackage(referencedPackageName, rushProject.reviewCategory);
+            updated = approvedPackagesPolicy.nonbrowserApprovedPackages.addOrUpdatePackage(
+              referencedPackageName,
+              rushProject.reviewCategory
+            );
           } else {
-            approvedPackagesPolicy.browserApprovedPackages
-              .addOrUpdatePackage(referencedPackageName, rushProject.reviewCategory);
+            updated = approvedPackagesPolicy.browserApprovedPackages.addOrUpdatePackage(
+              referencedPackageName,
+              rushProject.reviewCategory
+            );
           }
+
+          this._filesAreOutOfDate = this._filesAreOutOfDate || updated;
         }
       }
     }

apps/rush-lib/src/logic/InstallManager.ts

@@ -271,8 +271,6 @@ export class InstallManager {
       // Check the policies
       PolicyValidator.validatePolicy(this._rushConfiguration, options.bypassPolicy);
 
-      ApprovedPackagesChecker.rewriteConfigFiles(this._rushConfiguration);
-
       // Git hooks are only installed if the repo opts in by including files in /common/git-hooks
       const hookSource: string = path.join(this._rushConfiguration.commonFolder, 'git-hooks');
       const hookDestination: string | undefined = Git.getHooksFolder();
@@ -300,6 +298,18 @@ export class InstallManager {
         }
       }
 
+      const approvedPackagesChecker: ApprovedPackagesChecker = new ApprovedPackagesChecker(this._rushConfiguration);
+      if (approvedPackagesChecker.approvedPackagesFilesAreOutOfDate) {
+        if (this._options.allowShrinkwrapUpdates) {
+          approvedPackagesChecker.rewriteConfigFiles();
+          console.log(colors.yellow(
+            'Approved package files have been updated. These updates should be committed to source control'
+          ));
+        } else {
+          throw new Error(`Approved packages files are out-of date. Run "rush update" to update them.`);
+        }
+      }
+
       // Ensure that the package manager is installed
       return this.ensureLocalPackageManager()
         .then(() => {

common/changes/@microsoft/rush/ianc-fail-on-outdated-approved-packages_2019-09-20-00-17.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "comment": "Rush update now prints a message when the approved packages files are out-of-date, and rush install exits with an error if they are out-of-date.",
+      "packageName": "@microsoft/rush",
+      "type": "none"
+    }
+  ],
+  "packageName": "@microsoft/rush",
+  "email": "[email protected]"
+}

common/changes/@microsoft/rush/ianc-fail-on-outdated-approved-packages_2019-09-26-22-02.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "comment": "Include peerDependencies in the approved packages files.",
+      "packageName": "@microsoft/rush",
+      "type": "none"
+    }
+  ],
+  "packageName": "@microsoft/rush",
+  "email": "[email protected]"
+}

common/reviews/api/rush-lib.api.md

@@ -10,7 +10,7 @@ import { IPackageJson } from '@microsoft/node-core-library';
 export class ApprovedPackagesConfiguration {
     constructor(jsonFilename: string);
     // (undocumented)
-    addOrUpdatePackage(packageName: string, reviewCategory: string): void;
+    addOrUpdatePackage(packageName: string, reviewCategory: string): boolean;
     clear(): void;
     // (undocumented)
     getItemByName(packageName: string): ApprovedPackagesItem | undefined;