Skip to content

Fix incorrect conditions in eng/common/templates/job/job.yml#12471

Merged
ulisesh merged 2 commits intodotnet:mainfrom
ulisesh:FixCondition
Feb 8, 2023
Merged

Fix incorrect conditions in eng/common/templates/job/job.yml#12471
ulisesh merged 2 commits intodotnet:mainfrom
ulisesh:FixCondition

Conversation

@ulisesh
Copy link
Contributor

@ulisesh ulisesh commented Feb 7, 2023

Arcade issue #12393

@ulisesh ulisesh linked an issue Feb 7, 2023 that may be closed by this pull request
5 tasks
garath
garath previously approved these changes Feb 7, 2023
@ulisesh ulisesh enabled auto-merge (squash) February 7, 2023 22:30
@ulisesh
Copy link
Contributor Author

ulisesh commented Feb 8, 2023

/azp run

@ulisesh
Copy link
Contributor Author

ulisesh commented Feb 8, 2023

/azp run arcade-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

- task: NuGetAuthenticate@0

- ${{ if or(eq(parameters.artifacts.download, 'true'), ne(parameters.artifacts.download, '')) }}:
- ${{ if or(eq(parameters.artifacts.download, 'true'), eq(parameters.artifacts.download, '')) }}:
Copy link
Member

Choose a reason for hiding this comment

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

This makes "false" work, but still does the behavior in the case of a missing parameter. I strongly feel it should be just:

 ${{ if eq(parameters.artifacts.download, 'true') }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed, I'll update the issue

@RussKie
Copy link
Contributor

RussKie commented Feb 8, 2023

Thank you!

@ulisesh ulisesh merged commit 0978c75 into dotnet:main Feb 8, 2023
@RussKie
Copy link
Contributor

RussKie commented Feb 8, 2023

On a second thought (after speaking with @MattGal) I think it was wrong to omit the second part of the condition. That is, it's probably not uncommon for consumers to not specify the "root" node (such as parameters.artifacts.publish.logs) and only specify a subnode (e.g., parameters.artifacts.publish.logs.name). This means, that with this fix the existing definitions, such as this, will stop working:

name: ValidateArcadeSDK
displayName: Validate Arcade SDK
enableMicrobuild: true
artifacts:
download:
path: build_stage_artifacts
publish:
artifacts:
name: Artifacts_ValidateSdk_Windows_NT_Release
logs:
name: Logs_ValidateSdk_Windows_NT_Release

This is why we need the second condition that checks for the empty string.

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.

Incorrect conditions in eng/common/templates/job/job.yml

4 participants