Pipelines | Read the general-purpose pool name from one shared variable group - #4627
Open
paulmedynski wants to merge 3 commits into
Open
Pipelines | Read the general-purpose pool name from one shared variable group#4627paulmedynski wants to merge 3 commits into
paulmedynski wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The shared variables template is also imported below the pipeline root, violating the stated root-only variable-group design.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Centralizes general-purpose 1ES pool selection in the sqlclient_pipeline_config variable group.
Changes:
- Replaces legacy and hardcoded pool names with
general_purpose_pool_name. - Imports the new variable group across CI, PR, stress, sync, and package pipelines.
- Retains project detection only for signing behavior.
File summaries
| File | Description |
|---|---|
eng/pipelines/pr/variables/pr-variables.yml |
Imports the shared pool configuration. |
eng/pipelines/pr/sqlclient-pr-pipeline.yml |
Passes the shared pool name to PR stages. |
eng/pipelines/libraries/ci-build-variables.yml |
Adds the shared variable group import. |
eng/pipelines/github-sync-pipeline.yml |
Uses the shared pool name. |
eng/pipelines/dotnet-sqlclient-ci-core.yml |
Sets the default CI pool from the shared variable. |
eng/pipelines/ci/stress/sqlclient-ci-stress-pipeline.yml |
Uses the shared pool for stress stages. |
eng/pipelines/ci/package/sqlclient-ci-package-pipeline.yml |
Replaces project-based pool selection. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
paulmedynski
added
the
Area\Engineering
paulmedynski
force-pushed
the
dev/paul/pool-name
branch
from
September 3, 2026 19:57
794341b to
41a485d
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Import the variable group explicitly at pipeline roots instead of through the shared job-variable template.
Review details
Suppressed comments (1)
eng/pipelines/libraries/ci-build-variables.yml:12
- This shared template is also imported at job scope by
common/templates/jobs/ci-build-nugets-job.yml:91, so adding the group here makes a nested template import the pipeline-level group despite the PR's root-only design. It also couples every consumer of these general build variables to this group. Importsqlclient-pipeline-config-v1explicitly in the CI core, stress, and GitHub-sync roots instead, while keeping this shared job-variable template independent of the pool configuration.
- group: sqlclient-pipeline-config-v1
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
paulmedynski
force-pushed
the
dev/paul/pool-name
branch
from
September 8, 2026 12:35
41a485d to
a1290df
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The unrelated Kerberos behavior change and nested variable-group import must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
paulmedynski
force-pushed
the
dev/paul/pool-name
branch
from
September 8, 2026 12:49
a1290df to
a752767
Compare
mdaigle
force-pushed
the
dev/paul/pool-name
branch
from
September 9, 2026 00:06
a752767 to
7890fe0
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
A moderate CI-pool comment inconsistency remains, and the pipeline-wide configuration requires human validation.
Review details
Suppressed comments (1)
eng/pipelines/dotnet-sqlclient-ci-core.yml:97
- This says every CI job uses the general-purpose pool, but this file still routes ARM64, macOS, and enclave jobs to special-purpose pools (for example, lines 595, 728, and 753). Reword this as “most CI jobs” so the comment agrees with the exceptions documented below.
# The name of the general-purpose 1ES pool that all CI jobs run in.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
paulmedynski
force-pushed
the
dev/paul/pool-name
branch
from
September 9, 2026 10:23
7890fe0 to
4961cb7
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Azure DevOps execution remains unverified, and a moderate documentation inconsistency remains unresolved.
Review details
Suppressed comments (1)
eng/pipelines/dotnet-sqlclient-ci-core.yml:97
- This says every CI job uses the general-purpose pool, but the following comment correctly notes that Always Encrypted and ARM64 jobs use special-purpose pools. Saying “most CI jobs” would keep the documentation internally consistent with the actual routing.
# The name of the general-purpose 1ES pool that all CI jobs run in.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
paulmedynski
force-pushed
the
dev/paul/pool-name
branch
from
September 9, 2026 11:18
4961cb7 to
5dad865
Compare
paulmedynski
added a commit
that referenced
this pull request
Sep 9, 2026
Addresses review feedback on PR #4627. ci-build-nugets-job.yml imported ci-build-variables.yml at job scope, which meant the sqlclient-pipeline-config-v1 group was loaded below the pipeline root. The import was redundant: that job is only reachable from dotnet-sqlclient-ci-core.yml, which already imports the same template at its root, so localFeedPath and packagePath were already in scope. It was also the only job template in the repo importing a shared variables file. Remove it so ci-build-variables.yml is imported at pipeline root only. Also reword the defaultPoolName parameter comment to "most CI jobs", since the Always Encrypted, ARM64, and macOS jobs use special-purpose pools.
cheenamalhotra
previously approved these changes
Sep 9, 2026
The pool name was configured three different ways: the CI pipelines read 'ci_var_defaultPoolName' from 'ADO Build properties', the PR pipeline read 'PoolNameDefault' from 'sqlclient-testconfig-v1', and the package pipeline hardcoded ADO-1ES-Pool or ADO-CI-1ES-Pool based on which project it ran in. Three sources for one value, and the package pipeline's copy had to be edited by hand whenever the pool changed. Add a 'sqlclient_pipeline_config' variable group to both the ADO.Net and Public projects, holding 'general_purpose_pool_name' set to that project's pool, and read it at every pipeline root. The name says general-purpose deliberately: we also use special-purpose pools for Always Encrypted, ARM64, Kerberos and Managed Instance jobs, which are still named directly and could get their own variables later. This also removes the project-name check that chose the package pipeline's pool, so 'isInternalBuild' now serves only the signing key argument. No existing variable group is modified. 'ci_var_defaultPoolName' and 'PoolNameDefault' keep their current values; they are simply no longer read.
Match the naming convention used by the other groups in these projects, such as sqlclient-testconfig-v1 and symbols-variables-v3: dashes rather than underscores, and a version suffix. The group was renamed in place in both the ADO.Net and Public projects, so its contents and its pipeline authorization are unchanged.
Addresses review feedback on PR #4627. ci-build-nugets-job.yml imported ci-build-variables.yml at job scope, which meant the sqlclient-pipeline-config-v1 group was loaded below the pipeline root. The import was redundant: that job is only reachable from dotnet-sqlclient-ci-core.yml, which already imports the same template at its root, so localFeedPath and packagePath were already in scope. It was also the only job template in the repo importing a shared variables file. Remove it so ci-build-variables.yml is imported at pipeline root only. Also reword the defaultPoolName parameter comment to "most CI jobs", since the Always Encrypted, ARM64, and macOS jobs use special-purpose pools.
paulmedynski
force-pushed
the
dev/paul/pool-name
branch
from
September 9, 2026 17:35
4ccc68b to
1d1fa15
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One value - the general-purpose 1ES pool - was configured three different ways:
$(ci_var_defaultPoolName)from theADO Build propertiesgroup$(PoolNameDefault)from thesqlclient-testconfig-v1groupADO-1ES-Pool/ADO-CI-1ES-Pool, chosen by aSystem.TeamProjectcheckThis consolidates them onto a single variable in a single group.
New variable group
sqlclient-pipeline-config-v1, created in both theADO.NetandPublicprojects and authorized for all pipelines, holding one variable:general_purpose_pool_nameADO.NetADO-1ES-PoolPublicADO-CI-1ES-PoolThese are the values the two existing variables already carried in each project, so no pool assignment changes.
Read it at every pipeline root - CI core, PR, stress, GitHub sync and CI package. Templates below the root still receive the pool as a
poolNameparameter and never read a variable group directly.Drop the project-name check in the CI package pipeline, which had its own copy of both pool names.
isInternalBuildnow serves only the signing key argument.Remove a redundant job-scope import of
ci-build-variables.ymlfromci-build-nugets-job.yml, so no template below a pipeline root loads the group. See Review feedback.How the variable reaches a job
Three supply paths, all rooted at a pipeline root:
libraries/ci-build-variables.yml, imported at rootpr/variables/pr-variables.ymlpr/sqlclient-pr-pipeline.ymlci/package/sqlclient-ci-package-pipeline.ymlWhy "general-purpose"
We also run jobs on special-purpose pools -
ADO-CI-AE-1ES-Pool(Always Encrypted),ADO-CI-PUBLIC-ARM64-1ES-EUS-POOL(ARM64),ADO-Trusted-*-WestUS2(Kerberos),Managed-Instance-pool- plus the Microsoft-hostedAzure Pipelinespool for macOS. Those are still named directly at their point of use. The variable name leaves room for them to get their own entries in this group later.No existing variable group is modified
ci_var_defaultPoolNameandPoolNameDefaultkeep their current values in their current groups. They are simply no longer read by any pipeline, and can be retired separately once this has run clean.Review feedback
ci-build-nugets-job.ymlimportedci-build-variables.ymlat job scope, which meant the new group was loaded below a pipeline root. The import was redundant: that job is reachable only viabuild-sqlclient-package-ci-stage.ymlfromdotnet-sqlclient-ci-core.yml, which already imports the same template at its root, so the only two variables the job consumes from it -localFeedPathandpackagePath- were already in scope. It was also the only job template in the repo importing a shared variables file; every other job and stage template relies on root pipeline variables.Removing it leaves
ci-build-variables.ymlwith exactly three importers, all pipeline roots.Also reworded the
defaultPoolNameparameter comment from "all CI jobs" to "most CI jobs", to agree with the special-purpose pools listed a few lines below it.Validation
eng/pipelines/**ci_var_defaultPoolName/PoolNameDefaultgeneral_purpose_pool_nameci-build-variables.ymlallPipelines.authorized = truein both projectsPipeline runs
All runs below are complete. Across all 8, 643 jobs ran with 0 failed and 0 cancelled.
A missing or unresolvable variable group is reported at queue time, before an agent is allocated, so a run that compiles already proves the group resolves in that project.
Public
Runs are on
refs/pull/4627/merge(merge commit0695d7bc).PR-SqlClient-Project(2197)sqlclient-pr(2281)sqlclient-ci-package(2300)CI-SqlClient(1879) is path A on the same core template asPR-SqlClient-Project, and runs onmainafter merge.CI-SqlClient-Package(1917),PR-SqlClient-Package(2198) andsqlclient-ci-stress(2250) are affected but disabled, so there is nothing to run.ADO.Net
These build the internal
dotnet-sqlclientrepo, so no GitHub PR can reach them. All runs are onrefs/heads/dev/paul/pool-nameat commit5337d5c4.MDS Main CI(1825)MDS Main CI-Package(1933)sqlclient-ci-package(2277)sqlclient-ci-kerberos(2306)sqlclient-ci-managed-instance(2308)sqlclient-ci-stress(2284) is affected but disabled.sqlclient-pr(2271) is path B, already covered bysqlclient-pr(2281) in Public.sqlclient-ci-kerberosandsqlclient-ci-managed-instancecarry no path, because they read no pool from a variable group - their stage templates nameADO-Trusted-*-WestUS2andManaged-Instance-pooldirectly, which the pool data below confirms. They are included as regression evidence that the shared templates still expand for them, not as coverage of this change.GitHub ADO mirror sync(2263) is affected - it is the only path A consumer that exists solely inADO.Net- but running it pushes a sync branch and opens a PR in the internal repo, so it is left to its schedule rather than queued for validation.Why "partially succeeded"
The three
partiallySucceededruns have no failed and no cancelled job. The result is downgraded solely by the quarantined flaky-test steps (Run Flaky Unit Tests,Run Flaky Manual Tests,Run Flaky Functional Tests), which reportsucceededWithIssuesby design. That is the normal outcome for these pipelines.Pools actually used
Agent worker names were read from each run's timeline, covering all 643 jobs.
PublicADO-CI-1ES-PoolADO.NetADO-1ES-PoolADO.NetADO-CI-PUBLIC-ARM64-1ES-EUS-POOLPublicADO-CI-PUBLIC-ARM64-1ES-EUS-POOLADO.NetADO-CI-AE-1ES-PoolADO.NetADO-Trusted-Domain-Win-WestUS2ADO.NetADO-Trusted-Linux-WestUS2ADO.NetManaged-Instance-poolADO.NetAzure PipelinesPublicAzure PipelinesADO.NetFinalize buildEvery job landed on the pool its project expects. No
Publicjob ran onADO-1ES-Pooland noADO.Netjob ran onADO-CI-1ES-Pool, so the group resolves to the correct per-project value with no leakage. The special-purpose pools appear exactly where they are named directly, unchanged by this PR.build_mds_akv_packages_job- the job whosevariables:block this PR removes - succeeded in every run that contains it, on the correct general-purpose pool each time:PR-SqlClient-Project173748ADO-CI-1ES-Poolsqlclient-ci-package173752ADO-CI-1ES-PoolMDS Main CI173753ADO-1ES-Poolsqlclient-ci-package173754ADO-1ES-PoolMDS Main CI-Package173755ADO-1ES-PoolThis is the direct evidence that
localFeedPathandpackagePathstill resolve from pipeline-root scope after the job-scope import was dropped.Note on the red checks in this PR
PR-SqlClient-Project173781 andsqlclient-pr173782 show as cancelled in the checks list. They are duplicate re-runs of the same merge commit0695d7bc, re-triggered by an edit to this PR description rather than by any code change -triggerInforeports the samepr.sourceShaas the clean pair above.They were starved of agents: every cancelled job carries
Remote machine provider issue: The agent did not connect within the alloted time of 45 minute(s), with no worker ever assigned. That is 1ES provisioning capacity, not a pipeline problem - 108 jobs in 173781 and 33 in 173782 did acquire agents onADO-CI-1ES-Pool, the value this PR sets, and the earlier runs on the identical commit had zero agent issues.The authoritative results for this change are 173748 and 173749.
Not affected
OneBranch (2240/2241), perf (2305/2311/2313), the sni and ctaip pipelines, and the docs pipelines.
Checklist