feat(validate): only trigger tests from impacted packages (#39)

* feat(validate): only trigger tests for impacted packages

Only trigger tests for impacted packages, other packages will be installed for synchronizing the
org. If the same org is retried, it will be skipped

BREAKING CHANGE: Changes the earlier behaviour across all validation modes

fixes #5

.github/workflows/sfp-build-docker.yml

@@ -110,20 +110,9 @@ jobs:
          repo: ${{ inputs.repo }}
          image: ${{ inputs.image }}
          existing-tag: ${{ env.PKG_VERSION }}-${{ inputs.suffix-tag }}
-         new-tag: ${{ github.ref_name }}
+         new-tag: development
          registry: ghcr.io
          username:   ${{ secrets.username }}
          token: ${{ secrets.token }}
 
 
-   #Tag the image as develop
-    - name: 'Tag Docker'
-      uses: ./.github/actions/tagDocker
-      with:
-         repo: ${{ inputs.repo }}
-         image: ${{ inputs.image }}
-         existing-tag: ${{ env.PKG_VERSION }}-${{ inputs.suffix-tag }}
-         new-tag:  ${{ env.RELEASE_NAME }}
-         registry: ghcr.io
-         username:   ${{ secrets.username }}
-         token: ${{ secrets.token }}

decision records/validate/005-validate-to-skip-testing.md

@@ -0,0 +1,36 @@
+# Trigger tests only for impacted packages during validate
+
+* Status: Approved
+* Deciders: @azlam-abdulsalam, @nbatterham, @Rocko1204
+* Issue: #5
+
+## Context and Problem Statement
+
+In the context of validating a change in CI/review orgs, sfp currently triggers all Apex tests for changed packages. However, these changes might not always be directly introduced by a developer from a branch. They could also result from syncing an upstream change or the org being a few commits behind.
+
+See the original issue  by @petter-eikeland
+
+i.e. given that validate orgs were created at midnight, and 2 PRs have been merged to master/main changing package A and package B, respectively, both of which containing Apex classes and tests. When creating a 3rd PR, only changing package C (also with Apex classes and tests), validate would pick up changes in all 3 packages (A, B and C). In this scenario, validate deploys package A, and then executes the corresponding Apex tests. Then the same for package B and then finally package C. Ideally, validate would only deploy the (already validated) changes in package A and B (to update to validate org to what's on master/main), and only execute the Apex tests for package C.
+E.g. if 'git diff HEAD origin/master --quiet ' + package["path"] + '/*' --> then run Apex tests. If not, skip Apex tests for the package.
+
+This additional execution of Apex tests for already validated changes has an especially significant impact on our current cadence given Salesforce's infrastructure issues during release windows.
+
+This model introduces considerable delay when PRs are synchronized in sfops style `Review` environments, as the 
+environment has a life cycle till the PR is addressed
+
+## Decision
+
+After reviewing multiple orgs, we've determined there's no added value in triggering Apex tests for packages that are synchronized rather than directly impacted. The changes in such packages are minimal since the PR doesn't introduce any new modifications to the out-of-sync packages. Therefore, it's more efficient to only trigger tests for the directly impacted packages.
+
+Initially, we considered adding a new flag and specific mode for this feature. However, to simplify adoption and because we foresee no significant impacts, we've decided against introducing a new mode. Instead, we will add flags to capture the SHA ref of the head of the incoming branch and the SHA ref of the base branch. It's crucial to ensure these refs are accurate and do not reflect any temporary detached commit IDs created by CI/CD systems such as GitHub.
+
+This also means the following changes to behavior of how validate works
+
+- Differentiate between packages that need to be synchronized vs validated
+- Allow validate to commit the changed packages in to the target org, reverting earlier change to disableArtifactUpdate,so
+  users can control the behavior required, Ideally disableArtifactUpdate will be set to false, so retries to the same
+  review org within a range of commits will save further time
+- Only commit the packages to an org, if the test pass for validate packages, however commit packages to org immediately for packages that are to be synchronized, when the deployment is succesful
+- Package Diff comparison logic need to be updated to ensure, that if the commit id in the org is incorrect, assume the package is never built
+
+

packages/sfp-cli/messages/validate.json

@@ -7,7 +7,8 @@
     "logsGroupSymbolFlagDescription": "Symbol used by CICD platform to group/collapse logs in the console. Provide an opening group, and an optional closing group symbol.",
     "deleteScratchOrgFlagDescription": "Delete scratch-org validation environment, after the command has finished running",
     "keysFlagDescription": "Keys to be used while installing any managed package dependencies. Required format is a string of key-value pairs separated by spaces e.g. packageA:pw123 packageB:pw123 packageC:pw123",
-    "baseBranchFlagDescription": "The pull request base branch",
+    "baseRefFlagDescription": "The sha/ref to the base commit against which this ref will be merged into, In CI/CD platforms please pass in the full sha as opposed to branch name",
+    "refFlagDescription": "The sha/ref that need to be validated, this should not be the merge ref in some ci/cd systems, rather the head ref of the branch that is proposed to be merged",
     "tagFlagDescription": "Tag the build with a label, useful to identify in metrics",
     "disableDiffCheckFlagDescription": "Disables diff check while validating, this will validate all the packages in the repository",
     "disableArtifactUpdateFlagDescription": "Do not update information about deployed artifacts to the org",

packages/sfp-cli/messages/validateAgainstOrg.json

@@ -5,7 +5,8 @@
     "logsGroupSymbolFlagDescription": "Symbol used by CICD platform to group/collapse logs in the console. Provide an opening group, and an optional closing group symbol.",
     "diffCheckFlagDescription": "Only build the packages which have changed by analyzing previous tags",
     "disableArtifactUpdateFlagDescription": "Do not update information about deployed artifacts to the org",
-    "baseBranchFlagDescription": "The pull request base branch",
+    "baseRefFlagDescription": "The sha/ref to the base commit against which this ref will be merged into, In CI/CD platforms please pass in the full sha as opposed to branch name",
+    "refFlagDescription": "The sha/ref that need to be validated, this should not be the merge ref in some ci/cd systems, rather the head ref of the branch that is proposed to be merged",
     "fastfeedbackFlagDescription": "Enable validation in fast feedback mode, In fast feedback mode, validation will only do selective deployment of changed components and selective tests",
     "releaseConfigFileFlagDescription":"(Required if the release modes are ff-relese-config or thorough-release-config), Path to the config file which determines which impacted domains need to be validated",
     "devhubAliasFlagDescription": "Provide the alias of the devhub previously authenticated, used for installing or updating dependency in individual/thorough modes",

packages/sfp-cli/package.json

@@ -1,7 +1,7 @@
 {
     "name": "@flxbl-io/sfp",
     "description": "sfp is a CLI tool to help you manage your Salesforce projects in an artifact centric model",
-    "version": "37.1.4",
+    "version": "38.2.10",
     "license": "MIT",
     "author": "flxblio",
     "release": "March 24",

packages/sfp-cli/src/commands/impact/package.ts

@@ -4,11 +4,11 @@ import { Stage } from '../../impl/Stage';
 import SFPLogger, { COLOR_KEY_MESSAGE, ConsoleLogger } from '@flxbl-io/sfp-logger';
 import { Flags } from '@oclif/core';
 import { loglevel } from '../../flags/sfdxflags';
-import { ZERO_BORDER_TABLE } from '../../ui/TableConstants';
 import ImpactedPackageResolver, { ImpactedPackageProps } from '../../impl/impact/ImpactedPackagesResolver';
-const Table = require('cli-table');
 import path from 'path';
 import * as fs from 'fs-extra';
+import ImpactedPackagesDisplayer from '../../core/display/ImpactedPackagesDisplayer';
+import { LoggerLevel } from '@flxbl-io/sfp-logger';
 
 
 Messages.importMessagesDirectory(__dirname);
@@ -41,13 +41,10 @@ export default class Package extends SfpCommand {
         const impactedPackageResolver = new ImpactedPackageResolver(this.props, new ConsoleLogger());
 
         let packagesToBeBuiltWithReasons = await impactedPackageResolver.getImpactedPackages();
-        let packageDiffTable = this.createDiffPackageScheduledDisplayedAsATable(packagesToBeBuiltWithReasons);
         const packagesToBeBuilt = Array.from(packagesToBeBuiltWithReasons.keys());
-
-        //Log Packages to be built
-        SFPLogger.log(COLOR_KEY_MESSAGE('Packages impacted...'));
-        SFPLogger.log(packageDiffTable.toString());
-
+        SFPLogger.log(COLOR_KEY_MESSAGE('Packages impacted...'),LoggerLevel.INFO,new ConsoleLogger());  
+        ImpactedPackagesDisplayer.displayImpactedPackages(packagesToBeBuiltWithReasons, new ConsoleLogger());
+
 
         const outputPath = path.join(process.cwd(), 'impacted-package.json');
         if (packagesToBeBuilt && packagesToBeBuilt.length > 0)
@@ -59,22 +56,7 @@ export default class Package extends SfpCommand {
        return packagesToBeBuilt;
     }
 
-    private createDiffPackageScheduledDisplayedAsATable(packagesToBeBuilt: Map<string, any>) {
-        let tableHead = ['Package', 'Reason', 'Last Known Tag'];
-        let table = new Table({
-            head: tableHead,
-            chars: ZERO_BORDER_TABLE,
-        });
-        for (const pkg of packagesToBeBuilt.keys()) {
-            let item = [
-                pkg,
-                packagesToBeBuilt.get(pkg).reason,
-                packagesToBeBuilt.get(pkg).tag ? packagesToBeBuilt.get(pkg).tag : '',
-            ];
-            table.push(item);
-        }
-        return table;
-    }
+
 
 
 }

packages/sfp-cli/src/commands/validate/org.ts

@@ -40,15 +40,18 @@ export default class ValidateAgainstOrg extends SfpCommand {
             default: false,
         }),
         disableartifactupdate: Flags.boolean({
-            deprecated: {
-              message: "--disableartifactupdate flag is deprecated, Artifacts used for validation are never recorded in the org "
-            },
             description: messages.getMessage('disableArtifactUpdateFlagDescription'),
             default: false,
         }),
         logsgroupsymbol,
-        basebranch: Flags.string({
-            description: messages.getMessage('baseBranchFlagDescription'),
+        ref: Flags.string({
+            aliases: ['branch'],
+            dependsOn: ['baseRef'],
+            description: messages.getMessage('refFlagDescription'),
+        }),
+        baseRef: Flags.string({
+            aliases: ['basebranch'],
+            description: messages.getMessage('baseRefFlagDescription'),
         }),
         orginfo: Flags.boolean({
             description: messages.getMessage('orgInfoFlagDescription'),
@@ -98,6 +101,13 @@ export default class ValidateAgainstOrg extends SfpCommand {
                 )}`
             )
         );
+        if(this.flags.ref) {
+            SFPLogger.log(COLOR_HEADER(`Ref: ${this.flags.ref}`));
+        }
+        if(this.flags.baseRef) {
+            SFPLogger.log(COLOR_HEADER(`Base Ref: ${this.flags.baseRef}`));
+        }
+
         if (this.flags.mode != ValidationMode.FAST_FEEDBACK) {
             SFPLogger.log(COLOR_HEADER(`Coverage Percentage: ${this.flags.coveragepercent}`));
         }
@@ -119,8 +129,9 @@ export default class ValidateAgainstOrg extends SfpCommand {
                 logsGroupSymbol: this.flags.logsgroupsymbol,
                 targetOrg: this.flags.targetorg,
                 diffcheck: this.flags.diffcheck,
-                baseBranch: this.flags.basebranch,
-                disableArtifactCommit: true,
+                branch: this.flags.ref,
+                baseBranch: this.flags.baseRef,
+                disableArtifactCommit: this.flags.disableartifactupdate,
                 disableSourcePackageOverride: this.flags.disablesourcepkgoverride,
                 disableParallelTestExecution: this.flags.disableparalleltesting,
                 orgInfo: this.flags.orginfo,

packages/sfp-cli/src/commands/validate/pool.ts

@@ -68,8 +68,14 @@ export default class ValidateAgainstPool extends SfpCommand {
             required: false,
             description: messages.getMessage('keysFlagDescription'),
         }),
-        basebranch: Flags.string({
-            description: messages.getMessage('baseBranchFlagDescription'),
+        ref: Flags.string({
+            aliases: ['branch'],
+            dependsOn: ['baseRef'],
+            description: messages.getMessage('refFlagDescription'),
+        }),
+        baseRef: Flags.string({
+            aliases: ['basebranch'],
+            description: messages.getMessage('baseRefFlagDescription'),
         }),
         tag: Flags.string({
             description: messages.getMessage('tagFlagDescription'),
@@ -126,6 +132,14 @@ export default class ValidateAgainstPool extends SfpCommand {
         if (this.flags.mode != ValidationMode.FAST_FEEDBACK) {
             SFPLogger.log(COLOR_HEADER(`Coverage Percentage: ${this.flags.coveragepercent}`));
         }
+
+        if(this.flags.ref) {
+            SFPLogger.log(COLOR_HEADER(`Ref: ${this.flags.ref}`));
+        }
+        if(this.flags.baseRef) {
+            SFPLogger.log(COLOR_HEADER(`Base Ref: ${this.flags.baseRef}`));
+        }
+
 
 
 
@@ -148,7 +162,8 @@ export default class ValidateAgainstPool extends SfpCommand {
                 shapeFile: this.flags.shapefile,
                 isDeleteScratchOrg: this.flags.deletescratchorg,
                 keys: this.flags.keys,
-                baseBranch: this.flags.basebranch,
+                branch: this.flags.ref,
+                baseBranch: this.flags.baseRef,
                 diffcheck: !this.flags.disablediffcheck,
                 disableArtifactCommit: true,
                 orgInfo: this.flags.orginfo,

packages/sfp-cli/src/core/display/ImpactedPackagesDisplayer.ts

@@ -0,0 +1,25 @@
+import SFPLogger, { Logger, LoggerLevel } from '@flxbl-io/sfp-logger';
+import { ZERO_BORDER_TABLE } from './TableConstants';
+const Table = require('cli-table');
+
+export default class ImpactedPackagesDisplayer {
+
+    public static displayImpactedPackages(packagesToBeBuilt: Map<string, any>,logger:Logger) {
+        let tableHead = ['Package', 'Reason', 'Last Known Commit Id/Tag'];
+        let table = new Table({
+            head: tableHead,
+            chars: ZERO_BORDER_TABLE,
+        });
+        for (const pkg of packagesToBeBuilt.keys()) {
+            let item = [
+                pkg,
+                packagesToBeBuilt.get(pkg).reason,
+                packagesToBeBuilt.get(pkg).tag ? packagesToBeBuilt.get(pkg).tag : '',
+            ];
+            table.push(item);
+        }
+         //Log Packages to be built
+         SFPLogger.log(table.toString(),LoggerLevel.INFO,logger);
+    }
+
+}

packages/sfp-cli/src/core/git/Git.ts

@@ -29,6 +29,10 @@ export default class Git {
         return this._git.revparse(['HEAD']);
     }
 
+    async getBaseBranchCommit(baseBranch:string): Promise<string> {
+        return this._git.revparse([baseBranch]);
+    }
+
     async show(options: string[]): Promise<string> {
         return this._git.show(options);
     }