also looking at this
fix: stop releases when a required command fails
#2014
3 files
+40
-34
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -773,27 +773,22 @@ export async function versionBump(options: VersionBumpOptions): Promise<void> { | ||
| 773 | 773 | |
| 774 | 774 | // Execute custom commands before git operations |
| 775 | 775 | if (execute && !dryRun) { |
| 776 | try { | |
| 777 | const commands = Array.isArray(execute) ? execute : [execute] | |
| 778 | for (const command of commands) { | |
| 779 | // Check for interruption before each command | |
| 780 | checkInterruption() | |
| 781 | if (progress) { | |
| 782 | // Provide full payload to satisfy VersionBumpProgress typing | |
| 783 | progress({ | |
| 784 | event: ProgressEvent.Execute, | |
| 785 | script: command, | |
| 786 | updatedFiles, | |
| 787 | skippedFiles, | |
| 788 | newVersion: lastNewVersion || '', | |
| 789 | oldVersion: _lastOldVersion, | |
| 790 | }) | |
| 791 | } | |
| 792 | executeCommandWithOutput(command, effectiveCwd) | |
| 776 | const commands = Array.isArray(execute) ? execute : [execute] | |
| 777 | for (const command of commands) { | |
| 778 | // A failed release prerequisite must reach the outer rollback handler | |
| 779 | // before any commit, tag or push can publish incomplete artifacts. | |
| 780 | checkInterruption() | |
| 781 | if (progress) { | |
| 782 | progress({ | |
| 783 | event: ProgressEvent.Execute, | |
| 784 | script: command, | |
| 785 | updatedFiles, | |
| 786 | skippedFiles, | |
| 787 | newVersion: lastNewVersion || '', | |
| 788 | oldVersion: _lastOldVersion, | |
| 789 | }) | |
| 793 | 790 | } |
| 794 | } | |
| 795 | catch (error) { | |
| 796 | console.warn(`Warning: Command execution failed: ${error}`) | |
| 791 | executeCommandWithOutput(command, effectiveCwd) | |
| 797 | 792 | } |
| 798 | 793 | } |
| 799 | 794 | else if (execute && dryRun) { |
| @@ -389,7 +389,7 @@ describe('Git Operations (Integration)', () => { | ||
| 389 | 389 | expect(mockExecSyncWithOutput).toHaveBeenCalledWith('echo "third"', expect.any(String)) |
| 390 | 390 | }) |
| 391 | 391 | |
| 392 | it('should handle command execution failures gracefully', async () => { | |
| 392 | it('rejects failed release commands and rolls back the version', async () => { | |
| 393 | 393 | const packagePath = join(tempDir, 'package.json') |
| 394 | 394 | writeFileSync(packagePath, JSON.stringify({ name: 'test', version: '1.0.0' }, null, 2)) |
| 395 | 395 | |
| @@ -400,27 +400,20 @@ describe('Git Operations (Integration)', () => { | ||
| 400 | 400 | } |
| 401 | 401 | }) |
| 402 | 402 | |
| 403 | const consoleSpy = spyOn(console, 'warn').mockImplementation(() => {}) | |
| 404 | ||
| 405 | await versionBump({ | |
| 403 | await expect(versionBump({ | |
| 406 | 404 | release: 'patch', |
| 407 | 405 | files: [packagePath], |
| 408 | execute: 'failing-command', | |
| 406 | execute: ['failing-command', 'echo "must not run"'], | |
| 409 | 407 | commit: false, |
| 410 | 408 | tag: false, |
| 411 | 409 | push: false, |
| 412 | 410 | quiet: true, |
| 413 | 411 | noGitCheck: true, |
| 414 | }) | |
| 412 | })).rejects.toThrow('Command failed') | |
| 415 | 413 | |
| 416 | // Version should still be updated despite command failure | |
| 417 | 414 | const updatedContent = JSON.parse(readFileSync(packagePath, 'utf-8')) |
| 418 | expect(updatedContent.version).toBe('1.0.1') | |
| 419 | ||
| 420 | // Warning should be shown | |
| 421 | expect(consoleSpy).toHaveBeenCalledWith('Warning: Command execution failed: Error: Command failed') | |
| 422 | ||
| 423 | consoleSpy.mockRestore() | |
| 415 | expect(updatedContent.version).toBe('1.0.0') | |
| 416 | expect(mockExecSyncWithOutput).not.toHaveBeenCalledWith('echo "must not run"', expect.any(String)) | |
| 424 | 417 | }) |
| 425 | 418 | |
| 426 | 419 | it('should not execute commands in dry run mode', async () => { |