Extend setVariable to allow 'output' variables to be created

[johnnyreilly]

This PR resolves #688 by extending the setVariable functionality to support output variables.

As suggested by @chris-codeflow, this is as simple as adding the isOutput=true property to the task.setvariable logging command.

The change is backwards compatible - it does not change the signature of setVariable. Rather it adds a new final parameter named isOutput. When true the command generated will feature isOutput=true.

Consider:

tl.setVariable('foo', 'bar', false, true)

This would generate the following:

##vso[task.setvariable variable=foo;isOutput=true;]bar

Tests

I've added a couple of tests. However it's been a little difficult since I'm unable to run tests on my Ubuntu laptop. I have a sense that this may not be Linux related, I think this project may depend upon node 6 which is no longer supported. This is the error encountered why I typed "npm run test":

> node make.js test

> tsc --outDir /home/john/src/azure-pipelines-task-lib/node/_build
Downloading file: https://nodejs.org/dist/v6.17.1/node-v6.17.1-linux-x64.tar.gz
/home/john/src/azure-pipelines-task-lib/node/node_modules/sync-request/index.js:27
    throw new Error(res.stderr.toString());
    ^

Error
    at doRequest (/home/john/src/azure-pipelines-task-lib/node/node_modules/sync-request/index.js:27:11)
    at downloadFile (/home/john/src/azure-pipelines-task-lib/node/buildutils.js:79:22)
    at downloadArchive (/home/john/src/azure-pipelines-task-lib/node/buildutils.js:111:27)
    at Object.exports.getExternals (/home/john/src/azure-pipelines-task-lib/node/buildutils.js:44:35)
    at Function.target.test (/home/john/src/azure-pipelines-task-lib/node/make.js:46:16)
    at Object.global.target.<computed> [as test] (/home/john/src/azure-pipelines-task-lib/node/node_modules/shelljs/make.js:28:26)
    at /home/john/src/azure-pipelines-task-lib/node/node_modules/shelljs/make.js:38:27
    at Array.forEach (<anonymous>)
    at Timeout._onTimeout (/home/john/src/azure-pipelines-task-lib/node/node_modules/shelljs/make.js:36:10)
    at listOnTimeout (internal/timers.js:554:17)

[chris-codeflow]

Great work @johnnyreilly . I would have implemented it and raised a PR myself, but I hadn't yet read the contribution guidelines.

[jessehouwing]

Note that if you define the output variable in the task json, you don't need to pass the isoutput=true.

[johnnyreilly]

That's fascinating @jessehouwing - is that documented anywhere do you know?

[jessehouwing]

Not that I know of.

[chris-codeflow]

@johnnyreilly . One review comment I have is that the call to setVariable doesn't make it obvious what the boolean arguments are for, as you can see from reading tl.setVariable('foo', 'bar', false, true).

Personally, I'm not a big fan of boolean parameters - especially after reading Clean Code by Robert Martin (Uncle Bob) many years ago and being enlightened!

My suggestion would be to use a new enum (I propose a new VariableScope enum with Job and Output, or suchlike, enumerations). This would make the code cleaner and easier to read. For example, constrast the above with tl.setVariable('foo', 'bar', false, VariableScope.Output).

I've deliberately not changed the existing secret boolean parameter (as that is part of the existing signature), but my comment applies to that also.

[johnnyreilly]

Hey @chris-codeflow!

Personally, I'm not a big fan of boolean parameters

Yeah I agree - my main motivation was to make a backwards compatible change. I'm not generally a fan of enums in TypeScript (something the TypeScript team is in agreement with) as they're not standard JS.

An alternative approach might be an explicit setOutputVariable which sits alongside the existing setVariable. Ultimately I'll be guided by what the maintainers of this project desire. So if they go with what's here, great! If they suggest your proposal then cool! Or setOutputVariable or another idea entirely.

Ultimately I don't feel to strongly about the approach as long as there is a way. 🤞 we'll have some interaction from the project owners on this PR soon!

[damccorm]

Thanks for the contribution, this generally LGTM.

As far as the boolean flag vs enum goes, I agree with the original choice made by @johnnyreilly to use a boolean. As mentioned, enums are a little weird for TS; more importantly though, that's the pattern this library has followed - I'd rather not introduce a one-off difference in style.

@johnnyreilly could you do a few things so we can get this in and published:

1. could you bump the minor version of the package.json/package-lock.json
2. Would you mind applying these same changes/open a pr targeting the releases/3.x branch? Right now we're maintaining both the 2.x and (preview) 3.x versions of this library, and I want to make sure this change gets into the 3.x version before it goes out of preview

[johnnyreilly]

Thanks @damccorm - I'll be happy to make that happen!

could you bump the minor version of the package.json/package-lock.json

The version is currently: 2.11.4 - so the new version will be 2.12.0.

[johnnyreilly]

I've made the changes requested on this PR @damccorm - I've opened up a second PR for the release/3.x changes: #692

[johnnyreilly]

Thanks @damccorm! I've just tested 2.12.0 in my pipeline and it works a treat!

[chris-codeflow]

Thanks @johnnyreilly and @damccorm for the feedback on the boolean vs enum type decision; I am only just starting out with TypeScript, so it's good to know that enums are not a great choice in TypeScript.

I suggested using a boolean when I raised the suggestion, so I was challenging my own suggestion when I commented on the type to use.

As an aside, what is a good alternative to a boolean in TypeScript, integers with constants?

[johnnyreilly]

Hey @chris-codeflow,

So if I was designing a function which took a number of inputs (as setVariable now does) I'd probably use the options object pattern.

VS Code actually offers this in the form of the "Convert parameters to destructured object" helper. eg

export function setVariable({ name, val, secret = false, isOutput = false }: { name: string, val: string, secret?: boolean, isOutput?: boolean}): void {
// ...
}

Where usage would look like this:

setVariable({ name: 'foo', val: 'bar', isOutput: true });

[chris-codeflow]

Thanks @johnnyreilly . I've seen that pattern in other projects I've contributed to; it's much cleaner (in my opinion) as you can see in the calling code the option being set, which is far more readable than having to look at the method signature.

[johnnyreilly]

By the way, it looks like you're based in Horsham? Small world! My Gran lived there all her life. West Sussex forever ❤️

[chris-codeflow]

Sorry the delayed response. I moved to Horsham in 2001. I was born in Croydon and was brought up in Worcester Park.