Introspect Windows CI image and skip setup accordingly #192
Conversation
mokagio
force-pushed
the
ainfra-1459-update-ci-toolkit-to-introspect-ci-env-and-skip-unnecessary
branch
from
363f3a9 to
657e18f
Compare
mokagio
force-pushed
the
ainfra-1459-update-ci-toolkit-to-introspect-ci-env-and-skip-unnecessary
branch
from
657e18f to
6d8e152
Compare
AliSoftware
force-pushed
the
ainfra-1459-update-ci-toolkit-to-introspect-ci-env-and-skip-unnecessary
branch
from
2313531 to
003b86f
Compare
| $ciDir = "C:\CI" | ||
| if (-not (Test-Path $ciDir)) { | ||
| New-Item -ItemType Directory -Path $ciDir | Out-Null | ||
| } |
There was a problem hiding this comment.
@mokagio Note: now that I've updated the logic to expose the ami_version in the AMI_VERSION env var instead of in a file in https://github.com/Automattic/buildkite-ci/pull/740/commits/d5495b0b582d5d164f9c05305129bf88bc899ba9, I've updated the ps1 script accordingly above but haven't touched the unit test, so that part will still have to be updated accordingly before merging.
| if (![string]::IsNullOrWhiteSpace($amiVersion) -and [version]$amiVersion -ge [version]'0.2') { | ||
| Write-Output "Tooling is already pre-installed in AMI versions >= 0.2. Only installing code signing certificate." | ||
| & "setup_windows_code_signing.ps1" | ||
| Exit 0 |
There was a problem hiding this comment.
I wonder if Exit $LastExitCode would be more appropriate here.
I remember looking into the exit code management before sabbatical, but I don't remember what I learned or if/where I tracked my findings.
Logged https://linear.app/a8c/issue/AINFRA-1466/audit-ci-toolkit-ps1-scripts-for-appropriate-exit-code-reporting to followup.
| $amiVersion = [Environment]::GetEnvironmentVariable('AMI_VERSION', 'Machine') | ||
| Write-Output "AMI_VERSION is: $amiVersion" | ||
| if (![string]::IsNullOrWhiteSpace($amiVersion) -and [version]$amiVersion -ge [version]'0.2') { | ||
| Write-Output "Tooling is already pre-installed in AMI versions >= 0.2. Only installing code signing certificate." | ||
| & "setup_windows_code_signing.ps1" | ||
| Exit 0 | ||
| } |
There was a problem hiding this comment.
Eventually, this script should contain only this call. Tracked https://linear.app/a8c/issue/AINFRA-1465/remove-env-setup-logic-now-in-ami-from-ci-toolkit-once-all-projects for it.
There was a problem hiding this comment.
Actually I think ideally we should even just eventually remove that script and have client projects call & setup_windows_code_signing.ps1 directly themselves 🙃 (and only if they really need the distribution certificate; eg Unit Tests jobs probably won't even need it?)
Having issues setting the env in a transient way but that the scripts can read.
|
In the interest of addressing the time consuming / blocking issues in Windows builds downstream, I'm going to merge this PR and ship a Checks performed:
|
…-ci-env-and-skip-unnecessary
| Write-Output "[!] Failed to download code signing certificate." | ||
| Exit $LastExitCode | ||
| } | ||
| & "setup_windows_code_signing.ps1" |
There was a problem hiding this comment.
Should we add a test for exit code here? I.e.
| & "setup_windows_code_signing.ps1" | |
| & "setup_windows_code_signing.ps1" | |
| If ($LastExitCode -ne 0) { | |
| Exit $LastExitCode | |
| } |
Or is that supposed to be automatically handled implicitly in that case?
There was a problem hiding this comment.
Similar to the earlier question regarding exit codes, I'm not sure.
Will look at it as part of linear.app/a8c/issue/AINFRA-1466/audit-ci-toolkit-ps1-scripts-for-appropriate-exit-code-reporting.
mokagio
deleted the
ainfra-1459-update-ci-toolkit-to-introspect-ci-env-and-skip-unnecessary
branch
https://linear.app/a8c/issue/AINFRA-1459
CHANGELOG.mdif necessary.