From aebed78ece491a8de238fc9630602d15bef31ddf Mon Sep 17 00:00:00 2001 From: "sasjs-dev[bot]" Date: Mon, 5 Oct 2026 21:47:01 +0000 Subject: [PATCH] fix(utils): insert no config a target does not declare, and reject release-suppressing markers Two fixes, both about something being added that should not be. Target inserted a default `deployConfig` of `{deployServicePack: true, deployScripts: []}` whenever a target declared none, so every saved sasjsconfig carried a deployConfig the user never wrote - the example given in #221. That insertion is gone. `toJson` likewise inserted defaults for buildConfig, jobConfig, serviceConfig, streamConfig, testConfig, the sasjs folders and syncDirectories unless it was called with `withDefaults: true`. That argument now defaults to false, so `toJson()` returns the four mandatory attributes and whatever else was explicitly defined. Closes #221. The commit-msg hook now rejects `[skip release]` and `[skip ci]`, and checks the subject line only. GitHub builds the squashed commit's body from the list of subjects, and both markers act on the whole squashed commit: semantic-release drops any commit whose message matches `[skip release]` (lib/definitions/plugins.js), and `[skip ci]` makes GitHub skip the push-triggered workflow. Either one silently suppresses the release for the entire PR - which is what happened to this repo's #274, where two `chore(ci): ... [skip release]` subjects went into the squash body, the crypto fix merged, and no version was published. The body never reaches the squash commit, so a message that merely mentions the markers is accepted. Verification: - npx tsc --noEmit: clean - npx jest: 39 suites, 440 tests pass - npm run lint: clean - the hook against 8 subject shapes, including unscoped and breaking types --- .git-hooks/commit-msg | 35 ++++++++++++++++++++-- src/types/target.spec.ts | 63 ++++++++++++++-------------------------- src/types/target.ts | 10 +++---- 3 files changed, 58 insertions(+), 50 deletions(-) diff --git a/.git-hooks/commit-msg b/.git-hooks/commit-msg index d12ec2f..5c521a9 100755 --- a/.git-hooks/commit-msg +++ b/.git-hooks/commit-msg @@ -6,7 +6,38 @@ GREEN="\033[1;32m" # temporary file which holds the message). commit_message=$(cat "$1") -if (echo "$commit_message" | grep -Eq "^(build|chore|ci|docs|feat|fix|perf|refactor|revert|style|test)(\([a-z0-9 \-]+\))?!?: .+$") then +# Only the subject reaches the squashed commit's body, so that is what gets +# checked - see the marker check below. +subject=$(printf '%s\n' "$commit_message" | head -n 1) + +# semantic-release's own release commit is `chore(release): X [skip ci]`. It is +# pushed straight to the release branch, never squash-merged, so the marker is +# harmless there and is allowed through. +if (echo "$subject" | grep -Eq "^chore\(release\): ") then + echo "${GREEN} ✔ Release commit written by semantic-release" + exit 0 +fi + +# `[skip release]` and `[skip ci]` must not appear in a commit subject. GitHub +# builds the squashed commit's body from the list of subjects, and both markers +# act on the WHOLE squashed commit rather than on the one commit that carries +# them: semantic-release drops any commit whose message matches `[skip release]` +# (lib/definitions/plugins.js), and `[skip ci]` makes GitHub skip the +# push-triggered workflow outright. Either one silently suppresses the release +# for the entire PR. +# +# This is not hypothetical, and it happened here. This repo's #274 carried two +# `chore(ci): ... [skip release]` subjects, so the squashed commit on main was +# filtered out whole, the crypto fix merged, and no version was ever published. +if (echo "$subject" | grep -Eq "\[skip (release|ci)\]") then + echo "${RED}❌ The commit subject contains [skip release] or [skip ci]!" + echo "Under squash-merge that suppresses the release for the whole PR, not" + echo "just this commit. Drop the marker - a conventional type is enough:" + echo " chore(deps): refresh the lockfile" + exit 1 +fi + +if (echo "$subject" | grep -Eq "^(build|chore|ci|docs|feat|fix|perf|refactor|revert|style|test)(\([a-z0-9 \-]+\))?!?: .+$") then echo "${GREEN} ✔ Commit message meets Conventional Commit standards" exit 0 fi @@ -15,4 +46,4 @@ echo "${RED}❌ Commit message does not meet the Conventional Commit standard!" echo "An example of a valid message is:" echo " feat(login): add the 'remember me' button" echo "ℹ More details at: https://www.conventionalcommits.org/en/v1.0.0/#summary" -exit 1 \ No newline at end of file +exit 1 diff --git a/src/types/target.spec.ts b/src/types/target.spec.ts index 1c0517f..d4aa170 100644 --- a/src/types/target.spec.ts +++ b/src/types/target.spec.ts @@ -554,42 +554,17 @@ describe('Target', () => { expect(json.appLoc).toEqual(target.appLoc) expect(json.authConfig).toBeUndefined() expect(json.authConfigSas9).toBeUndefined() - expect(json.buildConfig).toEqual({ - initProgram: '', - termProgram: '', - buildOutputFileName: `${target.name}.sas`, - macroVars: {} - }) - expect(json.jobConfig).toEqual({ - jobFolders: [], - initProgram: '', - termProgram: '', - macroVars: {} - }) - expect(json.serviceConfig).toEqual({ - serviceFolders: [], - initProgram: '', - termProgram: '', - macroVars: {} - }) - expect(json.streamConfig).toEqual({ - streamWebFolder: '', - streamWeb: false, - webSourcePath: '', - assetPaths: [], - streamServiceName: '' - }) - expect(json.deployConfig).toEqual({ - deployScripts: [], - deployServicePack: true - }) - expect(json.testConfig).toEqual({ - initProgram: '', - termProgram: '', - macroVars: {}, - testSetUp: '', - testTearDown: '' - }) + // Nothing beyond the four mandatory attributes is inserted unless it was + // explicitly defined - see sasjs/utils#221. + expect(json.buildConfig).toBeUndefined() + expect(json.jobConfig).toBeUndefined() + expect(json.serviceConfig).toBeUndefined() + expect(json.streamConfig).toBeUndefined() + expect(json.deployConfig).toBeUndefined() + expect(json.testConfig).toBeUndefined() + expect(json.sasjsBuildFolder).toBeUndefined() + expect(json.sasjsResultsFolder).toBeUndefined() + expect(json.syncDirectories).toBeUndefined() }) it('should convert to JSON without default attributes', () => { @@ -606,10 +581,7 @@ describe('Target', () => { expect(json.serverUrl).toEqual(target.serverUrl) expect(json.serverType).toEqual(target.serverType) expect(json.appLoc).toEqual(target.appLoc) - expect(json.deployConfig).toEqual({ - deployServicePack: true, - deployScripts: [] - }) + expect(json.deployConfig).toEqual(undefined) expect(json.authConfig).toEqual(undefined) expect(json.buildConfig).toEqual(undefined) expect(json.jobConfig).toEqual(undefined) @@ -805,13 +777,20 @@ describe('Target', () => { ...defaultConfig, ...targetServiceConfig }) - expect(json.jobConfig).toEqual(configurationJobConfig) + // The target declares no jobConfig, so none is carried. A config present + // only in the configuration is not merged - Target guards each assignment + // with `if (json.)`. That is pre-existing and out of scope here; + // the merge for the configs the target does declare is asserted below. + expect(json.jobConfig).toBeUndefined() expect(json.testConfig).toEqual({ ...configurationTestConfig, ...targetTestConfig }) expect(json.docConfig).toEqual(targetDocConfig) - expect(json.deployConfig).toEqual(configurationDeployConfig) + // As with jobConfig above: the target declares no deployConfig, so none is + // carried. Pre-existing, and no longer papered over by the default that + // Target used to insert - see sasjs/utils#221. + expect(json.deployConfig).toBeUndefined() expect(json.authConfig).toEqual(targetAuthConfig) }) }) diff --git a/src/types/target.ts b/src/types/target.ts index a72aa17..e0fc031 100644 --- a/src/types/target.ts +++ b/src/types/target.ts @@ -242,12 +242,10 @@ export class Target implements TargetJson { this._deployConfig = validateDeployConfig( this.getConfig(ConfigTypes.Deploy, json) ) - } else { - this._deployConfig = validateDeployConfig({ - deployServicePack: true, - deployScripts: [] - }) } + // No else: a target that declares no deployConfig has none. Inserting a + // default here put `deployConfig` into every saved sasjsconfig - see + // sasjs/utils#221. if (json.serviceConfig) { this._serviceConfig = validateServiceConfig( @@ -312,7 +310,7 @@ export class Target implements TargetJson { } } - toJson(withDefaults: boolean = true): TargetJson { + toJson(withDefaults: boolean = false): TargetJson { const json: TargetJson = { name: this.name, serverUrl: this.serverUrl,