-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix CI jobs regression to *not* default to shouldContinueOnError: true
#69736
Conversation
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsMy change in 110b4cf, changed
but IIUC, yml is treating this as a string, and not a bool, and so is This patch changes the condition to match how it is done in other files:
--
|
/azp run runtime-wasm,runtime-extra-platforms |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run runtime-wasm,runtime-extra-platforms |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
My change in 110b4cf, changed xplat-setup.yml to have: `shouldContinueOnError: ${{ or(parameters.shouldContinueOnError, ..` but IIUC, yml is treating this as a string, and not a bool, and so is evaluating that to `true` if the string is not empty, which would be the case even if the string has `false`. (`To Boolean: '' (the empty string) → False, any other string → True`[1]). This patch changes the condition to match how it is done in other files: `shouldContinueOnError: ${{ or(eq(parameters.shouldContinueOnError, true), ` To confirm this, I added: ```diff + - name: foo_orig + value: ${{ parameters.shouldContinueOnError }} + - name: foo_true + value: ${{ eq(parameters.shouldContinueOnError, true) }} + - name: foo_false + value: ${{ eq(parameters.shouldContinueOnError, false) }} + - name: foo_true_x + value: ${{ eq(parameters.shouldContinueOnError, True) }} + - name: foo_false_x + value: ${{ eq(parameters.shouldContinueOnError, False) }} + - name: foo_bar + value: ${{ or(parameters.shouldContinueOnError, False) }} ``` .. and the resulting values in the expanded yml were: ```yml - name: foo_orig value: false - name: foo_true value: False - name: foo_false value: True - name: foo_true_x value: False - name: foo_false_x value: True - name: foo_bar value: True ``` So, if we have a parameter `shouldContinueOnError: false` (note the lower case), then it is a string. And using it in `or(parameters.shouldContinueOnError, false)` becomes `or(True, false)` since `false` is a non-empty string. And using `eq(parameters.shouldContinueOnError, true)` correctly "converts" that to a boolean. -- 1. https://docs.microsoft.com/en-us/azure/devops/pipelines/process/expressions?view=azure-devops
shouldContinueOnError
as a booleanshouldContinueOnError: true
/azp run runtime-wasm,runtime-extra-platforms |
Azure Pipelines successfully started running 2 pipeline(s). |
I checked the expanded yaml files to confirm that they have the correct values. I'm going to merge this now, so helix failures can correctly fail the respective jobs. |
My change in radical@110b4cf, changed
xplat-setup.yml to have:
shouldContinueOnError: ${{ or(parameters.shouldContinueOnError, ..
but IIUC, yml is treating this as a string, and not a bool, and so is
evaluating that to
true
if the string is not empty, which would be thecase even if the string has
false
.(
To Boolean: '' (the empty string) → False, any other string → True
[1]).This patch changes the condition to match how it is done in other files:
shouldContinueOnError: ${{ or(eq(parameters.shouldContinueOnError, true),
To confirm this, I added:
.. and the resulting values in the expanded yml were:
So, if we have a parameter
shouldContinueOnError: false
(note the lower case),then it is a string. And using it in
or(parameters.shouldContinueOnError, false)
becomesor(True, false)
since
false
is a non-empty string. And usingeq(parameters.shouldContinueOnError, true)
correctly "converts" thatto a boolean.
--