fix(core): avoid unnecessary config version changes (#5508)

* fix(core): avoid unnecessary config version changes

* refactor: move action config version calculation to an outer function

For simpler testability.

* test: simple test case for `exclude` config field

chore: type import

chore: amend

* test: test cases for all non-versionable config fields

* chore: explain details behind some non-versioned config fields

---------

Co-authored-by: Vladimir Vagaytsev <[email protected]>

core/src/actions/base.ts

@@ -543,17 +543,12 @@ export abstract class BaseAction<
     return this._treeVersion
   }
 
-  @Memoize()
-  private stringifyConfig() {
-    return stableStringify(omit(this._config, "internal"))
-  }
-
   /**
    * The version of this action's config (not including files or dependencies)
    */
   @Memoize()
   configVersion() {
-    return versionStringPrefix + hashStrings([this.stringifyConfig()])
+    return getActionConfigVersion(this._config)
   }
 
   /**
@@ -787,6 +782,7 @@ export interface ExecutedActionExtension<
   getOutput<K extends keyof (StaticOutputs & RuntimeOutputs)>(
     key: K
   ): GetOutputValueType<K, StaticOutputs, RuntimeOutputs>
+
   getOutputs(): StaticOutputs & RuntimeOutputs
 }
 
@@ -896,3 +892,20 @@ export function actionIsDisabled(config: ActionConfig, _environmentName: string)
   // TODO: implement environment fields and check if environment is disabled
   return config.disabled === true
 }
+
+/**
+ * We omit a few fields here that should not affect versioning directly:
+ * - The internal field (basePath, configFilePath etc.) are not relevant to the config version.
+ * - The source, include and exclude stanzas are implicitly factored into the tree version.
+ *   Those are handled separately in {@link VcsHandler},
+ *   see {@link VcsHandler.getTreeVersion} and {@link VcsHandler.getFiles}.
+ * - The description field is just informational, shouldn't affect execution.
+ * - The disabled flag is not relevant to the config version, since it only affects execution.
+ */
+const nonVersionedActionConfigKeys = ["internal", "source", "include", "exclude", "description", "disabled"] as const
+export type NonVersionedActionConfigKey = keyof Pick<BaseActionConfig, (typeof nonVersionedActionConfigKeys)[number]>
+
+export function getActionConfigVersion<C extends BaseActionConfig>(config: C) {
+  const configToHash = omit(config, ...nonVersionedActionConfigKeys)
+  return versionStringPrefix + hashStrings([stableStringify(configToHash)])
+}

core/test/unit/src/actions/base.ts

@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2018-2023 Garden Technologies, Inc. <[email protected]>
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+import { expect } from "chai"
+import { DEFAULT_BUILD_TIMEOUT_SEC } from "../../../../src/constants.js"
+import type { ActionConfig, BaseActionConfig } from "../../../../src/actions/types.js"
+import type { NonVersionedActionConfigKey } from "../../../../src/actions/base.js"
+import { getActionConfigVersion } from "../../../../src/actions/base.js"
+
+describe("getActionConfigVersion", () => {
+  function minimalActionConfig(): ActionConfig {
+    return {
+      kind: "Build",
+      type: "test",
+      name: "foo",
+      timeout: DEFAULT_BUILD_TIMEOUT_SEC,
+      internal: {
+        basePath: ".",
+      },
+      spec: {},
+    }
+  }
+
+  context("action config version does not change", () => {
+    // Helper types for testing non-versioned config fields.
+    // The tests won't compile if the NonVersionedActionConfigKey type is modified.
+    type TestValuePair<T> = { leftValue: T; rightValue: T }
+    type TestMatrix = {
+      [key in NonVersionedActionConfigKey]: TestValuePair<BaseActionConfig[key]>
+    }
+
+    const testMatrix: TestMatrix = {
+      description: { leftValue: "Description 1", rightValue: "Description 2" },
+      disabled: { leftValue: true, rightValue: false },
+      exclude: { leftValue: ["file1"], rightValue: ["file2"] },
+      include: { leftValue: ["file1"], rightValue: ["file2"] },
+      internal: { leftValue: { basePath: "./base1" }, rightValue: { basePath: "./base2" } },
+      source: { leftValue: { path: "path1" }, rightValue: { path: "path2" } },
+    }
+
+    for (const [field, valuePair] of Object.entries(testMatrix)) {
+      it(`on ${field} field modification`, () => {
+        const config1 = minimalActionConfig()
+        config1[field] = valuePair.leftValue
+        const version1 = getActionConfigVersion(config1)
+
+        const config2 = minimalActionConfig()
+        config2[field] = valuePair.rightValue
+        const version2 = getActionConfigVersion(config2)
+
+        expect(version1).to.eql(version2)
+      })
+    }
+  })
+})