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.

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

Add pool provider template reference #82452

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented Feb 21, 2023

Use template to determine pool provider to use.

The provider can't be expanded in the pipeline-with-resources step as the expansion breaks once global variables are consolidated. All variables end up in the global scope correctly, but several expansion points are expanded before assignment - particularly the ones at the pipeline scope - meaning that several parameters derived from them are empty and cause issues in further expansion decisions.

@hoyosjs hoyosjs requested review from MattGal and a team February 21, 2023 22:49
@ghost ghost assigned hoyosjs Feb 21, 2023
Copy link
Member

@MattGal MattGal left a comment

Choose a reason for hiding this comment

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

LGTM, make sure to check in on any pipelines not run as part of PR validation after it merges.

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

Do we want to add the variable template in the pipeline-with-resources.yml template instead of in each job template? Not sure if it makes an appreciable difference.

@hoyosjs
Copy link
Member Author

hoyosjs commented Feb 21, 2023

@jkoritzinsky I tried, the yml fails to expand sadly. something already defines a variable scope globally.

@hoyosjs hoyosjs added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 21, 2023
@jkoritzinsky
Copy link
Member

Dang. Let's just go with this then.

@MattGal
Copy link
Member

MattGal commented Feb 21, 2023

Dang. Let's just go with this then.

There are some... compromises... in the eng/common folder's usage of the same for the same reason. I'm just happy it can work at all.

@hoyosjs
Copy link
Member Author

hoyosjs commented Feb 22, 2023

https://dev.azure.com/dnceng/internal/_build/results?buildId=2120060&view=logs&j=af986455-df3a-5ed6-b067-9144b6c8d0c6

verifies the internal build used the variables were properly expanded.

@hoyosjs
Copy link
Member Author

hoyosjs commented Feb 22, 2023

/azp run runtime-libraries enterprise-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hoyosjs hoyosjs removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 22, 2023
@hoyosjs hoyosjs merged commit 7d3a2bb into dotnet:main Feb 22, 2023
@hoyosjs hoyosjs deleted the juhoyosa/pool-providers branch February 22, 2023 06:45
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants