ci: add Control Tower prcheck to the PR check + reusable step templates#17885
ci: add Control Tower prcheck to the PR check + reusable step templates#17885PawelWMS wants to merge 14 commits into
Conversation
Rename the confusingly-named --source-commit/--target-commit flags to --from-commit (baseline / merge base) and --to-commit (newer commit), and add optional --changed-components-file / --specs-diff-file / --render-set-file output-name overrides (defaults unchanged). Update all three callers: pr-package-build-stages.yml, common-steps.yml, and the check-rendered-specs GitHub Actions gate. Ordering preserved: from -> to (base -> head). Resolves the two AI: comments on the compute_change_set.sh call site. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
compute_changed.sh had a single caller (compute_change_set.sh), so fold it in as a compute_changed() function and delete the standalone script. Behavior is unchanged: azldev component changed --from <base> --to <head> with the same supply-chain drift guard. Update the components README table and the common-steps.yml comment that referenced the removed script. Resolves the @ai: comment on the compute_changed.sh wrapper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add reusable step templates under templates/steps/ so the post-merge and PR
Control Tower pipelines can compose only the pieces they need instead of
duplicating setup:
ensure-full-history.yml unshallow guard
install-deps.yml PipAuthenticate + azldev + python deps, with
installMock / installAdoRequirements /
normalizeGoGitConfig toggles
commit-range-postmerge.yml previous-build delta (ADO Builds API)
commit-range-pr.yml merge-commit-parent range (PR)
prepare-change-set.yml compute_change_set.sh + triage artifact,
from/to variable names parameterized
verify-locks.yml lock freshness check
verify-rendered-specs.yml render --check-only drift check
These are additive; nothing consumes them yet (wired in follow-up commits).
Two AI: fixes land here, in commit-range-pr.yml:
- build-reason check moved to a compile-time template expression on
variables['Build.Reason'] so a mis-triggered run fails fast.
- the merge-base is now named baseCommit / base_commit (it is the fork
point, not the target tip).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the inlined step bodies in common-steps.yml with composition of the new templates/steps/* templates. The post-merge pipelines (package-build, source-upload) that splice common-steps in are unchanged in effect: same steps, same order, same emitted variables (sourceCommit/targetCommit -> changedComponentsFile/renderSetFile). Note one intentional, safe hardening: the shared install-deps.yml validates .azldev-version against ^[0-9A-Za-z._+-]+$ before `go install`, which the old common-steps did not. The committed .azldev-version is well-formed, so this only rejects a genuinely malformed value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lates
Rewrite the PR package-build stages template to:
- compose the shared templates/steps/* templates (ensure-full-history,
install-deps with normalizeGoGitConfig, commit-range-pr, prepare-change-set)
instead of duplicating the setup inline; and
- call Control Tower 'prcheck' BEFORE the scratch package build. prcheck
uploads the missing lookaside sources for the changed components so they are
present when the build fetches them, so it must run first.
This folds the prcheck call (previously in sources-upload-stages.yml) into the
active PR check. The scratch-build step is unchanged. Reusing the granular
templates also lands the remaining template-local AI: fixes (build-reason
template expression and baseCommit rename live in commit-range-pr.yml; the
change_set_dir/changedComponentsFile derivation lives in prepare-change-set.yml).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename the template pair now that the PR check does prcheck + package build: pr-package-build.yml -> pr-check-ct.yml pr-package-build-stages.yml -> pr-check-ct-stages.yml Rename the stage/job PRPackageBuild -> PRCheckCT, repoint the wrapper @self template path and artifactBaseName (prcheckct), and refresh the header comments and the components README references. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Control Tower prcheck (source upload) call now lives in the PR check (pr-check-ct), so the standalone source-upload pipeline is redundant. Delete both files: .github/workflows/ado/sources-upload.yml .github/workflows/ado/templates/sources-upload-stages.yml Scrub the now-dangling references in the package-build wrapper/stages headers, common-steps.yml, the scripts/ci READMEs, and the ado-pipeline instructions (canonical pairing example and shared-sub-template note repointed to the still-live package-build pipeline and the granular templates/steps/*). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR re-wires the ADO PR Control Tower check so it calls Control Tower prcheck (intended to upload changed components' missing lookaside sources) before submitting a scratch package build, and refactors the shared setup steps into granular, parameterized step sub-templates. It fits the repo's wrapper/raw-stages ADO convention and the cross-pipeline scripts/ci/components/ change-set contract, consolidating the previously separate sources-upload and pr-package-build pipelines into a single pr-check-ct flow.
Changes:
- Extract
common-steps.ymlinto granular templates (ensure-full-history,install-deps,commit-range-pr/commit-range-postmerge,prepare-change-set,verify-locks,verify-rendered-specs) and recompose the post-merge pipeline from them. - Rename
compute_change_set.shargs--source/--target-commit→--from/--to-commit, inline the now-deletedcompute_changed.sh, and add configurable output-file names; update all callers. - Add
prcheckbefore the scratch build in the renamedpr-check-ctpipeline and remove the standalonesources-uploadpipeline, updating docs/READMEs.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ado/templates/pr-check-ct-stages.yml |
New PR CT stages: prcheck + scratch build composed from shared step templates (contains the prcheck no-op discrepancy). |
.github/workflows/ado/pr-check-ct.yml |
Wrapper retargeted to pr-check-ct-stages.yml, artifact renamed prcheckct, passes all required params. |
.github/workflows/ado/templates/steps/install-deps.yml |
New parameterized dep-install step (mock/ADO SDK/go-git-normalize opt-ins) with azldev version validation. |
.github/workflows/ado/templates/steps/prepare-change-set.yml |
New change-set step; from/to commit vars parameterized, publishes triage artifact + pipeline vars. |
.github/workflows/ado/templates/steps/commit-range-pr.yml |
New PR merge-base range resolver with compile-time PR-build guard + SHA validation. |
.github/workflows/ado/templates/steps/commit-range-postmerge.yml |
Extracted post-merge commit-range logic (ADO Builds API). |
.github/workflows/ado/templates/steps/{ensure-full-history,verify-locks,verify-rendered-specs}.yml |
Extracted individual steps from common-steps.yml. |
.github/workflows/ado/templates/steps/common-steps.yml |
Recomposed post-merge step set from the granular templates (order preserved). |
scripts/ci/components/compute_change_set.sh |
Inlines compute_changed.sh, renames args to --from/--to-commit, adds output-name overrides. |
scripts/ci/components/compute_changed.sh |
Deleted (single caller inlined). |
.github/workflows/check-rendered-specs.yml |
GH Actions caller updated to --from-commit/--to-commit. |
.github/workflows/ado/{package-build.yml,templates/package-build-stages.yml} |
Comment updates reflecting composed-from-templates wording. |
.github/workflows/ado/{sources-upload.yml,templates/{sources-upload,pr-package-build}-stages.yml} |
Removed redundant pipelines/templates. |
scripts/ci/{components,ado}/README.md, .github/instructions/ado-pipeline.instructions.md |
Docs updated for new callers/canonical examples. |
| python3 scripts/ci/control-tower/run_prcheck.py \ | ||
| --api-audience "$API_AUDIENCE" \ | ||
| --api-base-url "$API_BASE_URL" \ | ||
| --build-reason "$CT_BUILD_REASON" \ | ||
| --changed-components-file "$CHANGED_COMPONENTS_FILE" \ | ||
| --source-commit "$SOURCE_COMMIT" \ | ||
| --repo-uri "$UPSTREAM_REPO_URL" | ||
| env: | ||
| API_AUDIENCE: $(ApiAudience) | ||
| API_BASE_URL: $(ApiBaseAFDUrl) | ||
| # Non-reserved name: an `env:` override of the reserved BUILD_REASON var is silently ignored by the agent. | ||
| CT_BUILD_REASON: $(Build.Reason) |
- run_prcheck.py: remove the `build_reason == PullRequest` short-circuit so prcheck actually runs (and uploads sources) on PR triggers. (CT-side support for PR-triggered prcheck still to be verified — see session TODO.) - pr-check-ct-stages.yml: replace the `variableGroup` parameter with explicit `apiAudience` / `apiBaseAFDUrl` string params passed by the wrapper; drop the job-scope `- group:`. The wrapper now declares the `ControlTower-PRCheck` group at root and passes `$(ApiAudience)` / `$(ApiBaseAFDUrl)`. - Drop the redundant `CT_BUILD_REASON` env var in both AzureCLI steps; use the auto-provided `$BUILD_REASON` directly. - Drop the trailing "runs first…" sentence from the prcheck step comment. - Parameterize the names of job variables the step templates set (commit-range-pr: sourceCommitVar/baseCommitVar; commit-range-postmerge: sourceCommitVar/targetCommitVar; prepare-change-set: changedComponentsFileVar/renderSetFileVar), defaults = current names. - Document the `<name>Var` / `<name>OutputVar` variable-naming convention in ado-pipeline.instructions.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address the second round of PR microsoft#17885 review comments and apply the same patterns across all ADO YAMLs (package-build as well as pr-check-ct): - Drop the `variableGroup` parameter from both stages templates; take `apiAudience`/`apiBaseAFDUrl` params and surface them (plus packageTarget / pollTimeoutSeconds) in the job `variables:` section so a caller may pass runtime $[ ] / $(macro) expressions. Both wrappers now declare the ControlTower-PRCheck group at root and pass $(ApiAudience)/$(ApiBaseAFDUrl). - Drop the redundant CT_BUILD_REASON everywhere; use the auto-provided $BUILD_REASON. - Route every pipeline parameter/variable used by a script through `env:` (setvariable name params, packageTarget, pollTimeoutSeconds, build reason) — no `${{ parameters }}`/`$(var)` inlined in script bodies. - Trim the wrapper variable-group comments to name only the extracted variables. AI instructions (ado-pipeline.instructions.md): - `<var>OutputVarTask` for the output-variable task-name parameter. - New rule: pass pipeline params/vars into scripts through `env:`. - New rule: templates that own a `variables:` section surface runtime-consumed params as variables (runtime $[ ] expressions only evaluate there). - Update the worked wrapper/stages example to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| --changed-components-file "$CHANGED_COMPONENTS_FILE" \ | ||
| --source-commit "$SOURCE_COMMIT" \ | ||
| --repo-uri "$UPSTREAM_REPO_URL" | ||
| env: |
| # Control Tower API audience + base URL, passed by the wrapper (which owns the | ||
| # variable group / env-specific values). The template stays group-agnostic. |
There was a problem hiding this comment.
Remove comment - template has no knowledge of the origin (variable group at the moment) of the parameters. This comment can easily go stale. Remove such assumptions of caller's intentions from all templates and scripts you've touched in this PR.
| # Runtime-consumed params surfaced as variables so a caller may pass | ||
| # runtime $[ ] / $(macro) expressions (evaluated here, not inline). |
There was a problem hiding this comment.
Remove comment, don't explain the calling convention in every YAML. Apply globally to all changes in this PR. If this approach is missing from AI instruction - update them.
…plates Third round of PR microsoft#17885 review comments: - Restore a short note (both stages templates' env blocks) that Build.Reason is auto-exposed as $BUILD_REASON and must NOT be added to env: -- the agent drops env: overrides of that reserved predefined variable. Also documented as an exception to the "pass params through env:" rule in the ADO instructions. - Remove comments that assumed the caller's intent / parameter value origin (the "passed by the wrapper / owns the variable group" and "surfaced as variables so a caller may pass..." notes) from the stages and step templates; the calling convention lives in the ADO instructions, not in every YAML. Reworded install-deps.yml / prepare-change-set.yml param docs to be caller-agnostic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| # PR commit range = merge-commit parents (baseCommit = merge base, | ||
| # sourceCommit = PR head). prepare-change-set diffs baseCommit -> | ||
| # sourceCommit. | ||
| - template: steps/commit-range-pr.yml |
There was a problem hiding this comment.
Global ADO pipelines rule to update: each job or output variable originating from a template requires a comment above the template setting it in the following format:
# Sets job variables:
# - <var_name1>
# - <var_name2>
#
# Sets output job variables:
# - <output_var_name1>
# - <output_var_name2>
- template: steps/commit-range-pr.ymlDon't mention variables set by the template but unused in the caller. The goal is uniform comment structure and ease of tracking each used variables and parameter.
Adjust instructions and all touched files.
| scriptType: bash | ||
| scriptLocation: inlineScript | ||
| inlineScript: | | ||
| set -euo pipefail | ||
|
|
||
| python3 scripts/ci/control-tower/run_prcheck.py \ |
There was a problem hiding this comment.
Global ADO pipeline instructions (apply also for this PR): for tasks running only one functionally-important command/script like this one, use the script path option instead of inlineScript.
| API_AUDIENCE: $(apiAudience) | ||
| API_BASE_URL: $(apiBaseAFDUrl) | ||
| CHANGED_COMPONENTS_FILE: $(changedComponentsFile) | ||
| SOURCE_COMMIT: $(sourceCommit) | ||
| # TODO: Base commit is not used yet. Will be needed once we move | ||
| # detection of affected components to CT. ADO task: 18816 | ||
| BASE_COMMIT: $(baseCommit) | ||
| UPSTREAM_REPO_URL: $(Build.Repository.Uri) |
There was a problem hiding this comment.
Global ADO pipeline rule (apply here as well): sort variables, parameters, env variables etc. in alphabetical order.
| # Step template: ensure the full git history is present, ONCE, before anything | ||
| # that needs it. The OneBranch checkout may be shallow (depth 1), but lock | ||
| # resolution, spec rendering (rpmautospec derives Release/changelog from | ||
| # `git log` over the lock files), and commit-range detection all require the | ||
| # complete history. Doing the unshallow here means the helper scripts can assume | ||
| # full history and never fetch themselves -- a `git fetch --depth=N` inside a | ||
| # script would re-shallow a full clone, which silently corrupts the rpmautospec | ||
| # Release calculation (and is a footgun when running the scripts locally). |
There was a problem hiding this comment.
Only explain what the template itself does, not how it may or may not be used. Apply to other new templates.
| @@ -0,0 +1,20 @@ | |||
| # Microsoft Corporation | |||
There was a problem hiding this comment.
Global instructions for all files we introduce (not just ADO pipelines): the copyright notice to use is:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.
Commenting method may need to be adjusted depending on the file the notice is included in.
| env: | ||
| RENDER_SET_FILE: $(renderSetFile) | ||
| displayName: "Verify rendered specs are up to date" | ||
| - template: ensure-full-history.yml |
There was a problem hiding this comment.
I think common-steps.yml is now used only by a single YAML. However, pr-check-ct-stages.yml should also verify locks and rendered specs, which it currently doesn't. Make the common-steps.yml re-usable by pr-check-st-stages.yml and then apply it there as well. Treat install-deps.yml in common-steps.yml as satisfying only the deps required for its internal needs (verification + commit range calculation). If the caller of common-steps.yml needs deps for other purposes, they should use the install-deps.yml template separately with their own parameters. Also, re-name common-steps.yml to get-changes-info.yml to better reflect its purpose for the caller. Pipe through var-naming parameters, if needed to give caller control over variables storing the changes info they need.
| - name: sourceCommitVar | ||
| type: string | ||
| default: sourceCommit | ||
| - name: targetCommitVar |
There was a problem hiding this comment.
Unify naming with commit-range-pr.yml - use baseCommit instead of targetCommit.
Address the fourth round of PR microsoft#17885 comments and back-apply the new ADO instruction rules across all touched files: - Rename common-steps.yml -> get-changes-info.yml and make it self-contained: a `mode` (pr|postmerge) parameter selects the commit-range template, and it runs full-history + install-deps(its own needs) + verify-locks + prepare-change-set + verify-rendered-specs, emitting the change-info vars via name params. The PR check now also verifies locks and rendered specs. - install-deps.yml is now granular (installAzldev / installControlTowerClient / installMock / installAdoRequirements / normalizeGoGitConfig); get-changes-info installs only what it needs, and each stages template installs the Control Tower client itself for its submission step. - Unify commit-range-postmerge on baseCommit (was targetCommit). - New instruction rules, applied everywhere: MIT copyright header on new files; alphabetical ordering of parameters/variables/env; "Sets job variables:" comment above a composed `- template:`; `<var>OutputVarTask` for output-var task names; template comments describe only what the template does (no caller-intent / value-origin assumptions); prefer scriptPath over inlineScript for single-command tasks (with the AzureCLI-python exception documented). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| # Microsoft Corporation | ||
| # |
| # Fail fast, at compile time, when this is not a PR build: the merge-commit | ||
| # parent logic only makes sense for a PullRequest trigger. | ||
| - ${{ if ne(variables['Build.Reason'], 'PullRequest') }}: | ||
| - script: | | ||
| echo "##[error]This pipeline must run as a PR build (Build.Reason=PullRequest); got '$BUILD_REASON'." | ||
| exit 1 | ||
| displayName: "Guard: PR build only" | ||
|
|
| scripts/ci/control-tower/verify_locks.sh \ | ||
| --output-file "$(Build.ArtifactStagingDirectory)/lock-update.json" \ | ||
| --publish-dir "$(ob_outputDirectory)" |
| steps: | ||
| - script: | | ||
| set -euo pipefail | ||
| change_set_dir="$(Build.ArtifactStagingDirectory)/change-set" |
| mkdir -p "$(ob_outputDirectory)/changed-components" | ||
| cp "$json_file" "$(ob_outputDirectory)/changed-components/" || true |
| esac | ||
| done | ||
| [[ -z "${output_dir:-}" || -z "${source_commit:-}" || -z "${target_commit:-}" ]] && usage | ||
| [[ -z "${output_dir:-}" || -z "${from_commit:-}" || -z "${to_commit:-}" ]] && usage |
| **Copyright notice on every new file.** Any new file introduced (any language, not just ADO pipelines) starts with: | ||
|
|
||
| ```text | ||
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT License. | ||
| ``` | ||
|
|
||
| Adjust the comment syntax to the file type (e.g. `//` for C, `<!-- -->` for XML/HTML). |
There was a problem hiding this comment.
This instruction doesn't belong in this file. This file focuses only on instructions for managing ADO pipeline YAMLs. Drop this instruction from this file for now. Still follow it in this PR.
|
|
||
| **Sort lists alphabetically.** Within a file, keep `parameters`, `variables`, and step `env:` entries in alphabetical order by name (case-insensitive). This makes additions and diffs predictable and variables easy to find. Ordered sequences whose order is semantically load-bearing — job `steps:`, script lines — are NOT sorted. | ||
|
|
||
| **Prefer a script file over `inlineScript` for single-command tasks.** A task whose body is one functionally-important command/script should reference a script file (e.g. `Bash@3` `filePath`, or `AzureCLI@2` `scriptLocation: scriptPath`) rather than embedding it inline. **Exception:** an `AzureCLI@2` step that exists only to establish the WIF az-login context for a non-shell script (e.g. `python3 some_script.py …` consumed by `DefaultAzureCredential`) keeps `inlineScript` — `scriptPath` runs the file *through* bash (ignoring any `#!` line), so honoring the rule would require a throwaway bash wrapper that adds indirection without value. |
There was a problem hiding this comment.
The exception is more general: if the script path cannot be used without an artificial wrapper, then use inline scripts.
| # Sets job variables named by baseCommitVar (previous build) and sourceCommitVar | ||
| # (this build). |
There was a problem hiding this comment.
Drop the phrases in the brackets.
| # Baseline = the MERGE BASE (fork point) of the target-branch tip | ||
| # (HEAD^1) and the PR head (HEAD^2), NOT the target tip itself. |
There was a problem hiding this comment.
Restore these 2 comment lines.
| # Sets job variables: | ||
| # - changedComponentsFile | ||
| # - renderSetFile |
There was a problem hiding this comment.
Bad use of comment. The changedComponentsFile variable is not used within the scope of this template at all. The renderSetFile is not the actual var name - it's parameterized, in which case say parameters.renderSetFileVar, so the name can be searched within the file.
Apply this to the other "Sets job variables" section in this YAML.
If this contradicts instructions, update instructions.
- Add the MIT copyright/license header to the ADO pipeline YAMLs that were missing it (both stages templates + both wrappers). - Route $(Build.ArtifactStagingDirectory) and $(ob_outputDirectory) through env: in verify-locks.yml and prepare-change-set.yml instead of inlining the macros in the script bodies. - compute_change_set.sh: reject unsafe --changed-components-file / --specs-diff-file / --render-set-file values (path separators / traversal) so they cannot escape --output-dir. - ADO instructions: drop the (repo-global) copyright rule from the ADO-specific file (still followed in this PR); generalize the inlineScript exception to "if scriptPath needs an artificial wrapper, use inlineScript". - commit-range-postmerge.yml: drop the parenthetical phrases from the header. - commit-range-pr.yml: restore the merge-base explanation comment above base_commit. Kept the compile-time Build.Reason guard (Copilot's "not available at compile time" concern is a false positive for this queue-time predefined variable). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d vars Audited every ADO YAML changed in this PR against .github/instructions/ado-pipeline.instructions.md. One violation of the reserved-predefined-variable rule found and fixed: commit-range-postmerge.yml mapped five auto-exposed predefined variables (Build.BuildId, Build.SourceVersion, System.CollectionUri, System.DefinitionId, System.TeamProject) in env:. Per the rule these are auto-exposed as $NAME and should not be mapped; the script already references them directly (and determine_commit_range.py reads System.CollectionUri/TeamProject from os.environ). Removed the five mappings and switched --branch to the auto-exposed $BUILD_SOURCEBRANCH; kept only the secret SYSTEM_ACCESSTOKEN (not auto-exposed) plus the two var-name parameters. All other changed files pass the audit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
What
Re-adds the Control Tower
prcheck(source-upload) call into the active PRcheck, and refactors the ADO Control Tower pipeline templates to share setup via
reusable, parameterized step templates.
Per-PR flow of the renamed
pr-check-ctpipeline: shared setup + changedetection →
prcheck(uploads the changed components' missing lookasidesources) → scratch package build. prcheck runs before the build because
the build depends on the sources prcheck uploads.
Commits (each self-contained)
compute_change_set.sh: rename--source/--target-commit→--from/--to-commit, add optional output-name params.compute_changed.shintocompute_change_set.sh(single caller); delete the wrapper.common-steps.ymlinto granular parameterized step templates undertemplates/steps/.common-steps.yml(post-merge pipeline) from those templates — behavior-preserving.prcheckto the PR check and compose it from the shared templates.pr-package-build*→pr-check-ct*(stage/jobPRCheckCT).sources-uploadpipeline; scrub references.Resolved inline
AI:/@AI:review notesBuild.Reason(fail fast).targetCommit/target_commit→baseCommit/base_commit(it is the fork point, not the target tip).change_set_dir/changedComponentsFilederived via the sharedprepare-change-set.yml.compute_change_set.sharg rename + output-name params; wrapper script inlined.Reviewer notes
compute_change_set.shpassesbash -n. Real end-to-end validation is the ADO pipeline (mariner-org/azldef 5465) — not run here.pr-check-ct.ymlstays NonOfficial / DEV for now (uses the DEV service connection + DEV endpoint, scratch builds only). Revisit whether the prcheck source upload into the 'scanned' location warrants promotion to Official.Related work items (ADO
mariner-org/mariner)Draft for review.