Skip to content
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.

Sign up for GitHub

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

Bh is pull request #117

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+33 −1
Open

Bh is pull request #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f88c2d
Update Get-BuildVariable.ps1
FISHMANPET Jun 20, 2019
a4d660a
Return IsPullRequest
FISHMANPET Aug 8, 2019
ade8ef4
Update Set-BuildVariable.ps1
FISHMANPET Aug 8, 2019
4a666c0
Merge branch 'RamblingCookieMonster:master' into BHIsPullRequest
FISHMANPET Mar 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions BuildHelpers/Public/Get-BuildEnvironment.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ function Get-BuildEnvironment {
CommitMessage via Get-BuildVariable
CommitHash via Get-BuildVariable
BuildNumber via Get-BuildVariable
IsPullRequest via Get-BuildVariable
ProjectName via Get-ProjectName
PSModuleManifest via Get-PSModuleManifest
ModulePath via Split-Path on PSModuleManifest
Expand Down Expand Up @@ -107,6 +108,11 @@ function Get-BuildEnvironment {
PSModuleManifest = ${Build.ManifestPath}
ModulePath = ${Build.ModulePath}
}
if (${Build.Vars}.IsPullRequest) {
#This will ensure IsPullRequest is inserted into the ordered table after the BuildNumber property
$index = $($BuildHelpersVariables.keys).IndexOf("BuildNumber")
Copy link
Owner

Choose a reason for hiding this comment

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

It's late so may be missing it, but any reason not to add IsPullRequest to the ordered hash definition above rather than this extra logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention here (and answer to the rest of your questions sort of flow from this) is that since an Environment Variable can't be a boolean, just some text, that IsPullRequest would only exist if the run was a pull request, and so you could check just for existence of the environment variable. Here, I wanted to ensure that it would appear after BuildNumber in the ordered hash, but I couldn't just add it to the hash definition because it wouldn't always be defined. This addition here will add it into the ordered hash after BuildNumber, but only do so if there actually is a value

$BuildHelpersVariables.Insert($index+1,"IsPullRequest",${Build.Vars}.IsPullRequest)
}
foreach($VarName in $BuildHelpersVariables.keys){
$BuildOutput = $BuildOutput -replace "\`$$VarName", $BuildHelpersVariables[$VarName]
}
Expand Down
22 changes: 21 additions & 1 deletion BuildHelpers/Public/Get-BuildVariable.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ function Get-BuildVariable {
BranchName: git branch for this build
CommitMessage: git commit message for this build
BuildNumber: Build number provided by the CI system
IsPullRequest: "True" if CI says the current build is the result of a PR, otherwise this value will not be present

.PARAMETER Path
Path to project root. Defaults to the current working path
Expand Down Expand Up @@ -263,12 +264,31 @@ function Get-BuildVariable {
$BuildNumber = 0
}

[pscustomobject]@{
$PullRequest = switch ($BuildSystem)
{
'AppVeyor' {if ($Environment.APPVEYOR_PULL_REQUEST_NUMBER) {"True"}; break}
'GitLab CI' {if ($Environment.CI_MERGE_REQUEST_ID) {"True"}; break}
'Azure Pipelines' {if ($ENV:BUILD_REASON -eq "PullRequest") {"True"}; break}
'Bamboo' {if ($Environment.bamboo.repository.pr.key) {"True"}; break}
'GoCD' {if ($Environment.PR_TITLE) {"True"}; break}
'Travis CI' {if ($ENV:TRAVIS_EVENT_TYPE -eq "pull_request") {"True"}; break}
'GitHub Actions' {if ($ENV:GITHUB_EVENT_NAME -eq "pull_request") {"True"}; break}
#'Jenkins' {if ($Environment.CHANGE_ID) {"True"}} ???? is this correct?
#'Teamcity' {if ???????) {"True"}} who even knows
}
Copy link
Owner

Choose a reason for hiding this comment

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

Two questions!

  • Can we add a default case that sets this to $null
  • Can we add else statements to indicate $false when a build system is identified, but no pull request evidence is found (i.e. not in a PR, unlike default case of not detecting being in a PR, which is $null)
  • Can we switch to boolean $true/$false rather than strings?

Sorry to be a pain, and sorry for such a long delay!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing anything differently with the variable here (setting it to null or false) would be a change of my intended functionality. Additionally, since these are going to be written to the environment as Environment variables, we'd lose the boolean-ness and they'd just end up as flat strings either way.

The intention was that the environment variable would only exist if it was a pull request, though I can see the benefit to knowing if you're definitively not in a PR build vs not being in a build environment where it can be detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok looked more closely at the code. The usual entry point is going to be Set-BuildEnvironment which takes the values from Get-BuildEnvironment which is what calls this (Get-BuildVariable).

In the end it runs this line:

$Output = New-Item -Path Env:\ -Name $prefixedVar -Value $BuildHelpersVariables[$VarName] -Force:$Force

If the value is $true that will set the Environment Variable to be 'True', if it's $false it will set it as 'False', but if it's $null this will throw an error.


$ReturnHash = @{
BuildSystem = $BuildSystem
ProjectPath = $BuildRoot
BranchName = $BuildBranch
CommitMessage = $CommitMessage
CommitHash = $CommitHash
BuildNumber = $BuildNumber
}
if ($PullRequest) {
$ReturnHash['IsPullRequest'] = $PullRequest
}

[PSCustomObject]$ReturnHash

}
6 changes: 6 additions & 0 deletions BuildHelpers/Scripts/Set-BuildVariable.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
BHProjectPath via Get-BuildVariable
BHBranchName via Get-BuildVariable
BHCommitMessage via Get-BuildVariable
BHCommitHash via Get-BuildVariable
BHBuildNumber via Get-BuildVariable
BHIsPullRequest via Get-BuildVariable
BHProjectName via Get-ProjectName
BHPSModuleManifest via Get-PSModuleManifest
BHModulePath via Split-Path on BHPSModuleManifest
Expand Down Expand Up @@ -111,11 +113,15 @@ $BuildHelpersVariables = @{
ProjectPath = ${Build.Vars}.ProjectPath
BranchName = ${Build.Vars}.BranchName
CommitMessage = ${Build.Vars}.CommitMessage
CommitHash = ${Build.Vars}.CommitHash
BuildNumber = ${Build.Vars}.BuildNumber
ProjectName = ${Build.ProjectName}
PSModuleManifest = ${Build.ManifestPath}
ModulePath = $(Split-Path -Path ${Build.ManifestPath} -Parent)
}
if (${Build.Vars}.IsPullRequest) {
$BuildHelpersVariables.IsPullRequest = [bool]${Build.Vars}.IsPullRequest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here at least we're setting PowerShell variables so it can be a boolean. Here it might make sense to set $false or $null, but then it's a slightly different behavior then when environment variables are used.

}
foreach ($VarName in $BuildHelpersVariables.Keys) {
Set-Variable -Scope $Scope -Name ('{0}{1}' -f $VariableNamePrefix,$VarName) -Value $BuildHelpersVariables[$VarName]
}