add global-json-file input

[omsmith]

Description:
Our workflow involves doing a checkout to the subdirectory of the workspace. This results in a similar situation to that described in #253, where the global.json is not present in the root of the workspace.

This PR proposes solving this with a global-json-file input, similar to node-version-file from actions/setup-node.

• Unlike node-version-file, global-json-file can be used alongside dotnet-version since multiple versions are already allowed.
• If global-json-file is specified, it is an error for it not to exist.
• As before, if global-json-file is not specified ./global.json is still searched, but only if dotnet-version was not, and the debug message of No version found, trying to find version from global.json is still printed.

I've updated the documentation with the new input.

I've not added tests as there is not an existing framework for specifying inputs and I did not feel I should significantly refactor anything to do so. The existing tests pass and show the fallback to ./global.json still works as expected.

Related issue:
Similar to #253

Check list:

• Mark if documentation changes are required.
• Mark if tests were added or updated to cover the changes.

Happy Friday and hope you have a good weekend!

[omsmith]

Hey @thboop - don't know if you're terribly involved with setup-dotnet, but I see you working on a lot of the first-party action ecosystem. Seems like the CODEOWNERS here is out of date, referring to a defunct team.

Would you or someone else be able to review this?

[omsmith]

Thanks for taking a look @vsafonkin. Looks like I had missed regenerating the dist files. I've done that and committed the changes and believe it should get a successful build now.

[maxim-lobanov]

Personally, I think the logic with hasVersions variable is not clear enough and difficult from readability side because on the first sight it looks like we read globalJson file always.
I think it will be easier to read if we move the condition to parent function (run)
Something like that (pseudocode, not tested)

let versions = core.getMultilineInput('dotnet-version');
const globalJsonFileInput = core.getInput('global-json-file');
if (versions.length === 0 || globalJsonFileInput) {
  const globalJsonVersion = getVersionFromGlobalJson(globalJsonFileInput);
  if (globalJsonVersion) {
    versions.push(globalJsonVersion);
  }
}
...
function getVersionFromGlobalJson(globalJsonFileInput: string | null): string | null {

This way it will be clearer that we read globalJsonFile only in case if 0 versions are specified or explicit global json file path is defined.

[omsmith]

I believe it's important that we throw if you specified global-json-file but it doesn't exist. This is the least surprising behaviour (I could imagine trying to track down why the wrong version was being installed in my future if it didn't), and it aligns with the behaviour in setup-node.

I was also trying to maintain the existing debug output, but can take that as being unnecessary.

Perhaps just something like this (pseudo)? Changes a bit less, clutters the main path a bit more, but it will state the flow clearer.

let versions = core.getMultilineInput('dotnet-version');

const globalJsonFileInput = core.getInput('global-json-file');
if (globalJsonFileInput) {
  if (!existsSync(globalJsonFileInput)) {
    throw new Error('...');
  }
  versions.push(getVersionFromGlobalJson(globalJsonFileInput));
}

if (versions.length === 0) {
  core.debug('No version found, trying to find version from global.json');
  if (existsSync('global.json')) {
    versions.push(getVersionFromGlobalJson('global.json'));
  }
}

[maxim-lobanov]

yes, looks good to me! Thanks for proposing

[vsafonkin]

@omsmith @maxim-lobanov, do we definitely want to install dotnet version from global.json as additional version?
It's a bit of a contradiction the previous logic: if dotnet-version is not supplied, look a version in global.json

[omsmith]

I think if it's specified we should definitely attempt to use it. In setup-node it's an error to specify both node-version and node-version-file, but it also only supports a single version in general.

Given dotnet supports multiple version, and we support that here with multiline dotnet-version I thought it made sense not to make them mutually exclusive.

If I were to use both, I would probably treat my specified global.json file as my primary and dotnet-version as additional ones I'm installing.

[maxim-lobanov]

I agree that if global-json-file is defined, we can't ignore it silently. So we have two options if both dotnet-version and global-json-file are defined:

• Throw exception and fail task with message that options can't be mixed.
• Use both dotnet-version and global-json-file values

The second option looks a bit more friendly and I don't see significant problems with it. But I agree that it is pretty rare case to use :)

[maxim-lobanov]

Just to clarify - I mean only case when both dotnet-version and global-json-file are specified.
If global-json-file is not defined and dotnet-version is defined, we shouldn't look at default global.json

[omsmith]

Thanks for the feedback. I've rebased and force-pushed the update since it's reverting most of the changes I'd done.

[maxim-lobanov]

Everything is good for me. But I will leave it to @dmitry-shibanov @vsafonkin for their final review.

[vsafonkin]

@omsmith, could you please add the tests for case with global-json-path to this workflow?

[omsmith]

@omsmith, could you please add the tests for case with global-json-path to this workflow?

Sure thing - hadn't noticed that as a tool.

And I'll have to regenerate dist after the code comment updates. I'll never get used to that!

[omsmith]

Had a busy day yesterday. I've regenerated dist and squashed that into the first commit, and added tests for the happy path usage in 444a936 88a8d99. Would you like me to set one up expecting failure if the file doesn't exist as well?

EDIT: sorry - just running the workflows on my fork and working out the sdk versions to use

I was originally testing more combinations, including the standard global.json fallback, but the tests need both 2.x and 3.x installed to build the sample projects. So 88a8d99 just adds a test that has a specified global.json installing 2.x and the dotnet-version to install the 3.x.

[Hbutlercapone]

Approve #276

[vsafonkin]

@brcrista, could you please review?

[brcrista]

I agree with adding this input. @vsafonkin I'll defer to you on the implemenation. @omsmith, just one typo to fix and then we can merge. Thanks for the contribution!

[omsmith]

Thanks for taking the time to review