From dd1f9312df9f797995cce73d8fa87795f99d85ee Mon Sep 17 00:00:00 2001 From: damccorm Date: Wed, 13 Nov 2019 13:43:26 -0500 Subject: [PATCH 1/3] Source bash tasks instead of executing them --- Tasks/BashV3/Tests/L0.ts | 28 ++++++++++++++++++++++++++-- Tasks/BashV3/Tests/L0Args.ts | 3 ++- Tasks/BashV3/Tests/L0External.ts | 4 +++- Tasks/BashV3/bash.ts | 20 +++++++++++++++++--- 4 files changed, 48 insertions(+), 7 deletions(-) diff --git a/Tasks/BashV3/Tests/L0.ts b/Tasks/BashV3/Tests/L0.ts index 85513d500b6b..ba1dd5a1e7a6 100644 --- a/Tasks/BashV3/Tests/L0.ts +++ b/Tasks/BashV3/Tests/L0.ts @@ -35,6 +35,30 @@ describe('Bash Suite', function () { it('Runs a checked in script correctly', (done: MochaDone) => { this.timeout(5000); + process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR'] = false; + let tp: string = path.join(__dirname, 'L0External.js'); + let tr: ttm.MockTestRunner = new ttm.MockTestRunner(tp); + + tr.run(); + + runValidations(() => { + assert(tr.succeeded, 'Bash should have succeeded.'); + assert(tr.stderr.length === 0, 'Bash should not have written to stderr'); + if (process.platform === 'win32') { + // This is different on windows because we change the script name to make sure the normalization call is happening. + assert(tr.stdout.indexOf(`Writing bash 'temp/path/script' to temp/path/fileName.sh`) > 0, 'Bash should have written the script to a file'); + } else { + assert(tr.stdout.indexOf(`Writing bash 'path/to/script' to temp/path/fileName.sh`) > 0, 'Bash should have written the script to a file'); + } + + assert(tr.stdout.indexOf('my script output') > 0,'Bash should have correctly run the script'); + }, tr, done); + }); + + it('Runs a checked in script correctly when using the old behavior', (done: MochaDone) => { + this.timeout(5000); + + process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR'] = true; let tp: string = path.join(__dirname, 'L0External.js'); let tr: ttm.MockTestRunner = new ttm.MockTestRunner(tp); @@ -67,9 +91,9 @@ describe('Bash Suite', function () { assert(tr.stderr.length === 0, 'Bash should not have written to stderr'); if (process.platform === 'win32') { // This is different on windows because we change the script name to make sure the normalization call is happening. - assert(tr.stdout.indexOf(`Writing . 'temp/path/script' myCustomArg to temp/path/fileName.sh`) > 0, 'Bash should have written the script to a file'); + assert(tr.stdout.indexOf(`Writing bash 'temp/path/script' myCustomArg to temp/path/fileName.sh`) > 0, 'Bash should have written the script to a file'); } else { - assert(tr.stdout.indexOf(`Writing . 'path/to/script' myCustomArg to temp/path/fileName.sh`) > 0, 'Bash should have written the script to a file'); + assert(tr.stdout.indexOf(`Writing bash 'path/to/script' myCustomArg to temp/path/fileName.sh`) > 0, 'Bash should have written the script to a file'); } assert(tr.stdout.indexOf('my script output') > 0,'Bash should have correctly run the script'); diff --git a/Tasks/BashV3/Tests/L0Args.ts b/Tasks/BashV3/Tests/L0Args.ts index ddc13527eb0e..c9bcd46089b3 100644 --- a/Tasks/BashV3/Tests/L0Args.ts +++ b/Tasks/BashV3/Tests/L0Args.ts @@ -51,7 +51,8 @@ let a: ma.TaskLibAnswers = { 'path/to/script': { isFile() { return true; - } + }, + mode: '777' } } }; diff --git a/Tasks/BashV3/Tests/L0External.ts b/Tasks/BashV3/Tests/L0External.ts index 09eb1128354c..8f8cd08e9b39 100644 --- a/Tasks/BashV3/Tests/L0External.ts +++ b/Tasks/BashV3/Tests/L0External.ts @@ -8,6 +8,7 @@ let tmr: tmrm.TaskMockRunner = new tmrm.TaskMockRunner(taskPath); tmr.setInput('targetType', 'filepath'); tmr.setInput('filePath', 'path/to/script'); tmr.setInput('workingDirectory', '/fakecwd'); +tmr.setInput('AZP_BASHV3_OLD_SOURCE_BEHAVIOR', process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR']); //Create assertAgent and getVariable mocks, support not added in this version of task-lib const tl = require('azure-pipelines-task-lib/mock-task'); @@ -50,7 +51,8 @@ let a: ma.TaskLibAnswers = { 'path/to/script': { isFile() { return true; - } + }, + mode: '777' } } }; diff --git a/Tasks/BashV3/bash.ts b/Tasks/BashV3/bash.ts index d78605f53427..2fd8e167b0bd 100644 --- a/Tasks/BashV3/bash.ts +++ b/Tasks/BashV3/bash.ts @@ -1,6 +1,5 @@ import fs = require('fs'); import path = require('path'); -import os = require('os'); import tl = require('azure-pipelines-task-lib/task'); import tr = require('azure-pipelines-task-lib/toolrunner'); var uuidV4 = require('uuid/v4'); @@ -45,8 +44,10 @@ async function run() { let input_filePath: string; let input_arguments: string; let input_script: string; + let old_source_behavior: boolean; let input_targetType: string = tl.getInput('targetType') || ''; if (input_targetType.toUpperCase() == 'FILEPATH') { + old_source_behavior = tl.getBoolInput('AZP_BASHV3_OLD_SOURCE_BEHAVIOR', /*required*/ false); input_filePath = tl.getPathInput('filePath', /*required*/ true); if (!tl.stats(input_filePath).isFile()) { throw new Error(tl.loc('JS_InvalidFilePath', input_filePath)); @@ -72,7 +73,20 @@ async function run() { targetFilePath = input_filePath; } - contents = `. '${targetFilePath.replace("'", "'\\''")}' ${input_arguments}`.trim(); + if (old_source_behavior) { + contents = `. '${targetFilePath.replace("'", "'\\''")}' ${input_arguments}`.trim(); + } else { + // Check if executable bit is set + let stats: fs.Stats = tl.stats(input_filePath); + if ((stats.mode & 1) > 0) { + contents = `bash '${targetFilePath.replace("'", "'\\''")}' ${input_arguments}`.trim(); + } + else { + console.log(stats.mode); + tl.warning('Executable bit is not set on target script, sourcing instead of executing. More info at https://github.com/Microsoft/azure-pipelines-tasks/blob/master/docs/bashnote.md'); + contents = `. '${targetFilePath.replace("'", "'\\''")}' ${input_arguments}`.trim(); + } + } console.log(tl.loc('JS_FormattedCommand', contents)); } else { @@ -152,4 +166,4 @@ async function run() { } } -run(); +run(); \ No newline at end of file From c118e519bdd0d901d3ac31fc691e27d13601ac8d Mon Sep 17 00:00:00 2001 From: damccorm Date: Wed, 13 Nov 2019 13:45:30 -0500 Subject: [PATCH 2/3] Bump version --- Tasks/BashV3/task.json | 4 ++-- Tasks/BashV3/task.loc.json | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Tasks/BashV3/task.json b/Tasks/BashV3/task.json index 8a0c735a0034..ec2986322969 100644 --- a/Tasks/BashV3/task.json +++ b/Tasks/BashV3/task.json @@ -17,8 +17,8 @@ "author": "Microsoft Corporation", "version": { "Major": 3, - "Minor": 159, - "Patch": 3 + "Minor": 160, + "Patch": 0 }, "releaseNotes": "Script task consistency. Added support for multiple lines and added support for Windows.", "minimumAgentVersion": "2.115.0", diff --git a/Tasks/BashV3/task.loc.json b/Tasks/BashV3/task.loc.json index 7322073e532a..2ecf26d895cc 100644 --- a/Tasks/BashV3/task.loc.json +++ b/Tasks/BashV3/task.loc.json @@ -17,8 +17,8 @@ "author": "Microsoft Corporation", "version": { "Major": 3, - "Minor": 159, - "Patch": 3 + "Minor": 160, + "Patch": 0 }, "releaseNotes": "ms-resource:loc.releaseNotes", "minimumAgentVersion": "2.115.0", From ef8fdc4d6a30ac9ac1be59e65f50f9b5954b19bd Mon Sep 17 00:00:00 2001 From: damccorm Date: Mon, 16 Dec 2019 15:38:01 -0500 Subject: [PATCH 3/3] Feedback --- Tasks/BashV3/Tests/L0.ts | 6 ++++-- Tasks/BashV3/Tests/L0External.ts | 1 - Tasks/BashV3/bash.ts | 12 +++++++++--- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/Tasks/BashV3/Tests/L0.ts b/Tasks/BashV3/Tests/L0.ts index ba1dd5a1e7a6..d6957b8b76c3 100644 --- a/Tasks/BashV3/Tests/L0.ts +++ b/Tasks/BashV3/Tests/L0.ts @@ -20,6 +20,7 @@ describe('Bash Suite', function () { it('Runs an inline script correctly', (done: MochaDone) => { this.timeout(5000); + delete process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR']; let tp: string = path.join(__dirname, 'L0Inline.js'); let tr: ttm.MockTestRunner = new ttm.MockTestRunner(tp); @@ -35,7 +36,7 @@ describe('Bash Suite', function () { it('Runs a checked in script correctly', (done: MochaDone) => { this.timeout(5000); - process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR'] = false; + delete process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR']; let tp: string = path.join(__dirname, 'L0External.js'); let tr: ttm.MockTestRunner = new ttm.MockTestRunner(tp); @@ -58,7 +59,7 @@ describe('Bash Suite', function () { it('Runs a checked in script correctly when using the old behavior', (done: MochaDone) => { this.timeout(5000); - process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR'] = true; + process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR'] = "true"; let tp: string = path.join(__dirname, 'L0External.js'); let tr: ttm.MockTestRunner = new ttm.MockTestRunner(tp); @@ -81,6 +82,7 @@ describe('Bash Suite', function () { it('Adds arguments to the script', (done: MochaDone) => { this.timeout(5000); + delete process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR']; let tp: string = path.join(__dirname, 'L0Args.js'); let tr: ttm.MockTestRunner = new ttm.MockTestRunner(tp); diff --git a/Tasks/BashV3/Tests/L0External.ts b/Tasks/BashV3/Tests/L0External.ts index 8f8cd08e9b39..78f6a469ad2e 100644 --- a/Tasks/BashV3/Tests/L0External.ts +++ b/Tasks/BashV3/Tests/L0External.ts @@ -8,7 +8,6 @@ let tmr: tmrm.TaskMockRunner = new tmrm.TaskMockRunner(taskPath); tmr.setInput('targetType', 'filepath'); tmr.setInput('filePath', 'path/to/script'); tmr.setInput('workingDirectory', '/fakecwd'); -tmr.setInput('AZP_BASHV3_OLD_SOURCE_BEHAVIOR', process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR']); //Create assertAgent and getVariable mocks, support not added in this version of task-lib const tl = require('azure-pipelines-task-lib/mock-task'); diff --git a/Tasks/BashV3/bash.ts b/Tasks/BashV3/bash.ts index 2fd8e167b0bd..41c054ab2db4 100644 --- a/Tasks/BashV3/bash.ts +++ b/Tasks/BashV3/bash.ts @@ -47,7 +47,7 @@ async function run() { let old_source_behavior: boolean; let input_targetType: string = tl.getInput('targetType') || ''; if (input_targetType.toUpperCase() == 'FILEPATH') { - old_source_behavior = tl.getBoolInput('AZP_BASHV3_OLD_SOURCE_BEHAVIOR', /*required*/ false); + old_source_behavior = !!process.env['AZP_BASHV3_OLD_SOURCE_BEHAVIOR']; input_filePath = tl.getPathInput('filePath', /*required*/ true); if (!tl.stats(input_filePath).isFile()) { throw new Error(tl.loc('JS_InvalidFilePath', input_filePath)); @@ -73,16 +73,22 @@ async function run() { targetFilePath = input_filePath; } + // Choose 1 of 3 behaviors: + // If they've set old_source_behavior, source the script. This is what we used to do and needs to hang around forever for back compat reasons + // If the executable bit is set, execute the script. This is our new desired behavior. + // If the executable bit is not set, source the script and warn. The user should either make it executable or pin to the old behavior. + // See https://github.com/Microsoft/azure-pipelines-tasks/blob/master/docs/bashnote.md if (old_source_behavior) { contents = `. '${targetFilePath.replace("'", "'\\''")}' ${input_arguments}`.trim(); } else { // Check if executable bit is set - let stats: fs.Stats = tl.stats(input_filePath); + const stats: fs.Stats = tl.stats(input_filePath); + // Check file's executable bit. if ((stats.mode & 1) > 0) { contents = `bash '${targetFilePath.replace("'", "'\\''")}' ${input_arguments}`.trim(); } else { - console.log(stats.mode); + tl.debug(`File permissions: ${stats.mode}`); tl.warning('Executable bit is not set on target script, sourcing instead of executing. More info at https://github.com/Microsoft/azure-pipelines-tasks/blob/master/docs/bashnote.md'); contents = `. '${targetFilePath.replace("'", "'\\''")}' ${input_arguments}`.trim(); }