-
Notifications
You must be signed in to change notification settings - Fork 241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve output to match build system #3430
base: master
Are you sure you want to change the base?
improve output to match build system #3430
Conversation
I don't think testing for various environment variables deep inside ContainerHelper is the right approach. |
i wanted to change as little as possible, hence the somewhat inelegant solution. your approach is of course better. but why shouldn't we automatically recognize the build system when loading the bccontainerhelper (possibly overridable by a setting)? if it only works nicely when the user activates a setting, most people will overlook it and not have the best experience. |
@freddydk can you have another look on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes and then we can merge this one.
@@ -120,6 +120,9 @@ function Get-ContainerHelperConfig { | |||
"MSAppsNuGetFeedUrl" = 'https://dynamicssmb2.pkgs.visualstudio.com/DynamicsBCPublicFeeds/_packaging/MSApps/nuget/v3/index.json' | |||
"TrustedNuGetFeeds" = @( | |||
) | |||
"IsGitHubActions" = [Boolean]$env:GITHUB_ACTIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PowerShell - [boolean]"false" is also true - I would prefer
"IsGitHubActions" = ($env:GITHUB_ACTIONS -eq "true")
(and the same with the other)
function Write-GroupStart([string] $Message) { | ||
switch ($true) { | ||
$bcContainerHelperConfig.IsAzureDevOps { Write-Host "##[group]$Message"; break } | ||
$bcContainerHelperConfig.IsGitHubActions { Write-Host "::group::$Message"; break } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the order of these to have IsGitHub on top and test for IsAzureDevOps afterwards.
I can see situations where TF_BUILD could be set to something in GitHub - something a partner could define themselves. I don't see anybody defining an environment variable called GITHUB_ACTIONS themselves.
@ChrisBlankDe - please fix the remaining two issues and let's get this in |
github actions and azure pipelines use different environment variables we could use to identify the running system.
we should use this possibility to output formatting commands for the given system.