Skip to content

Notify if global.json was not found in the root directory.#344

Merged
marko-zivic-93 merged 2 commits into
actions:mainfrom
teo-tsirpanis:patch-1
Nov 17, 2022
Merged

Notify if global.json was not found in the root directory.#344
marko-zivic-93 merged 2 commits into
actions:mainfrom
teo-tsirpanis:patch-1

Conversation

@teo-tsirpanis

@teo-tsirpanis teo-tsirpanis commented Nov 10, 2022

Copy link
Copy Markdown
Contributor

Description:
I was investigating a bug where my workflow was installing .NET through this action, with the version specified in global.json. The action finished immediately with no log messages and baffled me until I realized that I was cloning the repository on a subdirectory and had to specify where global.json is by myself.

This PR adds a message that might have helped me figure this out sooner.

Related issue:
Add link to the related issue.

Check list:

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

Comment thread src/setup-dotnet.ts Outdated
}
DotnetCoreInstaller.addToPath();
} else {
core.warning('No .NET versions were specified to be installed.');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be more informative to change the logging level in line 45 from debug to info? And add an else statement after the line 49 to incorporate another info log with text like that "global.json file isn't found. No version was specified to be installed."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do it. Instead of this warning I added?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an info (how about I make it a warning instead? It indicates the user might have done something wrong) if the global.json in the root wasn't found, and removed the warning I previously added. I kept the severity of the debug message you mentioned; better not emit any more diagnostics in the happy path. Let me know what you think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About keeping debug level, I think you're right - there is no need to spam customers if their intention is not to use dotnet-version and global-json-file inputs. As for the changing info to warning I think it'll be an overkill because some of our customers use action just for authentication (without need for SDK installation), so the info is enough in my opinion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Can you approve again the CI?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To solve issues with CI, just update the dotnet-installer scripts using: npm run update-installers and in the workflow.yml in the test-proxy job change this image: datadog/squid:latest to image: ubuntu/squid:latest.

@IvanZosimov

Copy link
Copy Markdown
Contributor

May I also ask you to resolve 5 failing checks? If you wonder how to do this, there is the documentation for contributors.

@teo-tsirpanis teo-tsirpanis changed the title Warn if the action ends up not installing any .NET version. Notify if global.json was not found in the root directory. Nov 16, 2022
@marko-zivic-93 marko-zivic-93 merged commit 069c35e into actions:main Nov 17, 2022
@teo-tsirpanis teo-tsirpanis deleted the patch-1 branch November 17, 2022 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants