Skip to content

Commit 4601b99

Browse files
l0lawrenceCopilot
andcommitted
Slim down emitter-diff CI workflow
Cut the python workflow to ~half its size: - trim explanatory comment headers - drop the hand-rolled github-script patch parser; reuse the tool's own 'Diff summary:' line (ANSI-stripped) as a step output for the PR comment - drop the redundant job-summary step and the now-unused --patch output - collapse 'Fail on tool error' to a one-liner Behavior is unchanged: sticky PR comment, HTML artifact, informational-only (fails only on a tool/build error, never on a diff). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent abe62b0 commit 4601b99

1 file changed

Lines changed: 31 additions & 124 deletions

File tree

Lines changed: 31 additions & 124 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,20 @@
11
name: "python / emitter diff"
22

3-
# Generates code with the current checkout's emitter and with the emitter as of
4-
# the base-branch commit this PR is based on (the merge-base), then diffs the two.
5-
# The rendered HTML diff is uploaded as a per-PR artifact and a sticky PR comment
6-
# summarizes the change with a link to download it, so reviewers can see exactly
7-
# how an emitter change affects generated SDKs. Powered by the language-agnostic
8-
# eng/emitter-diff tool.
9-
#
10-
# This check is informational: it always passes unless the tool hits a real
11-
# tool/build error. A generated-output diff does not fail the PR — it is only
12-
# reported (job summary + PR comment + HTML artifact) for reviewers to eyeball.
13-
#
14-
# Python's generator uses a native two-phase pipeline (TypeSpec emits YAML, then
15-
# a batched Python subprocess writes the .py files) with a venv co-located with
16-
# each emitter version. So this workflow builds + sets up a venv for both the
17-
# head checkout and a worktree of the baseline commit before diffing.
3+
# Diffs generated code between this PR's emitter and the merge-base baseline.
4+
# Informational only: reports a sticky PR comment + HTML artifact, and fails the
5+
# job only on a tool/build error (never on a diff). See eng/emitter-diff.
186

197
on:
208
pull_request:
21-
branches:
22-
- main
23-
- release/*
9+
branches: [main, release/*]
2410
paths:
2511
- "packages/http-client-python/**"
2612
- "eng/emitter-diff/**"
2713
- ".github/workflows/ci-emitter-diff-python.yml"
2814
workflow_dispatch:
2915
inputs:
3016
baseline:
31-
description: "Baseline emitter ref (npm version, local path, or github ref). Defaults to the PR's merge-base with its base branch."
17+
description: "Baseline emitter ref (npm version, local path, or github ref). Defaults to the PR merge-base."
3218
required: false
3319
default: ""
3420

@@ -48,47 +34,32 @@ jobs:
4834
- uses: actions/checkout@v6
4935
with:
5036
fetch-depth: 0
51-
5237
- uses: ./.github/actions/setup
53-
5438
- uses: actions/setup-python@v5
5539
with:
5640
python-version: "3.12"
57-
5841
- name: Install repo dependencies (emitter-diff tool)
5942
run: pnpm install
60-
6143
- name: Build + venv for head emitter
6244
working-directory: packages/http-client-python
63-
run: |
64-
npm ci
65-
npm run build
66-
npm run install
45+
run: npm ci && npm run build && npm run install
6746

6847
- name: Determine baseline
6948
id: baseline
70-
# The dispatch input is untrusted; pass everything through `env` and
71-
# reference only shell variables so nothing is interpolated into the
72-
# script body (GitHub Actions expression injection).
49+
# Dispatch input is untrusted: pass via env, reference only as "$VAR".
7350
env:
7451
BASELINE_INPUT: ${{ github.event.inputs.baseline || '' }}
7552
BASE_REF: ${{ github.event.pull_request.base.ref || github.event.repository.default_branch }}
7653
RUNNER_TEMP: ${{ runner.temp }}
7754
run: |
7855
input="$BASELINE_INPUT"
7956
if [ -z "$input" ]; then
80-
# Baseline = the base-branch commit this PR is based on (merge-base).
81-
# It is always a real commit on the target branch, so it survives
82-
# squash-merge / rebase / force-push (unlike a pinned branch SHA).
57+
# merge-base = a real commit on the base branch; survives squash/rebase.
8358
git fetch --no-tags origin "$BASE_REF"
8459
base_sha="$(git merge-base FETCH_HEAD HEAD)"
85-
if [ -z "$base_sha" ]; then
86-
echo "::error::Could not determine a merge-base with $BASE_REF."
87-
exit 1
88-
fi
89-
echo "Baseline (merge-base with $BASE_REF): $base_sha"
60+
[ -n "$base_sha" ] || { echo "::error::No merge-base with $BASE_REF."; exit 1; }
9061
git worktree add "$RUNNER_TEMP/baseline" "$base_sha"
91-
(cd "$RUNNER_TEMP/baseline/packages/http-client-python" && npm ci && npm run build && npm run install)
62+
( cd "$RUNNER_TEMP/baseline/packages/http-client-python" && npm ci && npm run build && npm run install )
9263
input="local:$RUNNER_TEMP/baseline/packages/http-client-python"
9364
echo "sha=$base_sha" >> "$GITHUB_OUTPUT"
9465
fi
@@ -98,43 +69,23 @@ jobs:
9869
- name: Run emitter diff
9970
id: diff
10071
working-directory: eng/emitter-diff
101-
# `BASELINE_REF` derives from the untrusted dispatch input, so keep it in
102-
# `env` and reference it as "$BASELINE_REF" rather than interpolating it.
10372
env:
10473
BASELINE_REF: ${{ steps.baseline.outputs.ref }}
10574
RUNNER_TEMP: ${{ runner.temp }}
10675
run: |
10776
set +e
108-
# Run with plain `node` (Node 24 runs TypeScript natively) so the tool's
109-
# real exit code propagates. No `--fail-on-diff`: this check is
110-
# informational and never fails on a diff. A real tool/build error still
111-
# surfaces via a non-zero exit code (checked in "Fail on tool error").
77+
# Node 24 runs TypeScript natively; no --fail-on-diff (informational).
78+
# A tool/build error still exits non-zero (checked in "Fail on tool error").
11279
node src/cli.ts \
113-
--emitter python \
114-
--baseline "$BASELINE_REF" \
80+
--emitter python --baseline "$BASELINE_REF" \
11581
--work-dir "$RUNNER_TEMP/emitter-diff" \
11682
--html "$RUNNER_TEMP/emitter-diff.html" \
117-
--patch "$RUNNER_TEMP/emitter-diff.patch" \
11883
| tee "$RUNNER_TEMP/emitter-diff.log"
11984
echo "status=${PIPESTATUS[0]}" >> "$GITHUB_OUTPUT"
120-
121-
- name: Write job summary
122-
if: always()
123-
env:
124-
BASELINE_SHA: ${{ steps.baseline.outputs.sha }}
125-
RUNNER_TEMP: ${{ runner.temp }}
126-
run: |
127-
{
128-
echo "## Emitter diff"
129-
echo ""
130-
echo "Baseline (merge-base): \`$BASELINE_SHA\` vs current checkout."
131-
echo ""
132-
echo '```'
133-
grep -E "Diff summary:" "$RUNNER_TEMP/emitter-diff.log" || echo "No summary captured."
134-
echo '```'
135-
echo ""
136-
echo "Download the **emitter-diff-html** artifact for the full rendered diff."
137-
} >> "$GITHUB_STEP_SUMMARY"
85+
# Reuse the tool's own summary line (strip ANSI) instead of re-parsing.
86+
summary="$(sed -r 's/\x1b\[[0-9;]*m//g' "$RUNNER_TEMP/emitter-diff.log" \
87+
| grep -oE 'Diff summary: [0-9]+ file\(s\), \+[0-9]+ / -[0-9]+' | head -1)"
88+
echo "summary=${summary:-No changes to generated output.}" >> "$GITHUB_OUTPUT"
13889
13990
- name: Upload HTML diff
14091
if: always()
@@ -150,71 +101,27 @@ jobs:
150101
continue-on-error: true
151102
uses: actions/github-script@v7
152103
env:
153-
BASELINE: ${{ steps.baseline.outputs.sha }}
104+
BASELINE: ${{ steps.baseline.outputs.sha || steps.baseline.outputs.ref }}
154105
STATUS: ${{ steps.diff.outputs.status }}
155-
PATCH_FILE: ${{ runner.temp }}/emitter-diff.patch
156-
HTML_FILE: ${{ runner.temp }}/emitter-diff.html
106+
SUMMARY: ${{ steps.diff.outputs.summary }}
157107
with:
158108
script: |
159-
const fs = require("fs");
160109
const marker = "<!-- emitter-diff-python -->";
161110
const runUrl = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`;
162-
const status = process.env.STATUS;
163-
164-
// Parse the unified patch directly so the numbers match the tool exactly.
165-
let patch = "";
166-
try { patch = fs.readFileSync(process.env.PATCH_FILE, "utf8"); } catch {}
167-
const lines = patch.split("\n");
168-
const files = [];
169-
let insertions = 0, deletions = 0;
170-
for (const line of lines) {
171-
const m = line.match(/^diff --git a\/(.+?) b\/(.+)$/);
172-
if (m) files.push(m[2]);
173-
else if (line.startsWith("+") && !line.startsWith("+++")) insertions++;
174-
else if (line.startsWith("-") && !line.startsWith("---")) deletions++;
175-
}
176-
const hasChanges = patch.trim().length > 0;
177-
const htmlExists = fs.existsSync(process.env.HTML_FILE);
178-
179-
let body = `${marker}\n## Python emitter diff\n\n` +
180-
`Baseline \`${process.env.BASELINE}\` (merge-base) vs this PR's checkout.\n\n`;
181-
if (status !== "0") {
182-
// Hard error (build/venv/generate threw) — not a diff. Don't claim "no changes".
183-
body += `**emitter-diff failed to run** (exit \`${status}\`). This is a tool/build ` +
184-
`error, not a generated-output diff. See the ` +
185-
`[workflow run](${runUrl}) logs` +
186-
(htmlExists ? ` and the **emitter-diff-html** artifact` : "") + `.\n`;
187-
} else if (!hasChanges) {
188-
body += `**No changes** — generated output matches the baseline.\n`;
189-
} else {
190-
body += `**${files.length} file(s) changed** · +${insertions} / -${deletions}\n\n`;
191-
if (htmlExists) {
192-
body += `Download the **emitter-diff-html** artifact from the ` +
193-
`[workflow run](${runUrl}) for the full side-by-side rendered diff.\n`;
194-
}
195-
}
196-
body += `\n_Generated by \`eng/emitter-diff\`. This check is informational and does not block the PR._`;
197-
198-
const { owner, repo } = context.repo;
199-
const issue_number = context.issue.number;
200-
const { data: comments } = await github.rest.issues.listComments({ owner, repo, issue_number });
201-
const existing = comments.find((c) => c.body && c.body.includes(marker));
202-
if (existing) {
203-
await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body });
204-
} else {
205-
await github.rest.issues.createComment({ owner, repo, issue_number, body });
206-
}
111+
const { STATUS, SUMMARY, BASELINE } = process.env;
112+
let body = `${marker}\n### Python emitter diff\nBaseline \`${BASELINE}\` vs this PR.\n\n`;
113+
body += STATUS !== "0"
114+
? `⚠️ **emitter-diff failed** (exit \`${STATUS}\`) — tool/build error, not a diff. See the [run](${runUrl}).\n`
115+
: `${SUMMARY}\n\nFull rendered diff: **emitter-diff-html** artifact on the [run](${runUrl}).\n`;
116+
body += `\n_Informational check (eng/emitter-diff); does not block the PR._`;
117+
const { owner, repo } = context.repo, issue_number = context.issue.number;
118+
const { data } = await github.rest.issues.listComments({ owner, repo, issue_number });
119+
const hit = data.find((c) => c.body && c.body.includes(marker));
120+
if (hit) await github.rest.issues.updateComment({ owner, repo, comment_id: hit.id, body });
121+
else await github.rest.issues.createComment({ owner, repo, issue_number, body });
207122
208123
- name: Fail on tool error
209124
if: always()
210125
env:
211126
STATUS: ${{ steps.diff.outputs.status }}
212-
run: |
213-
# Informational check: a generated-output diff does NOT fail the PR
214-
# (the tool is run without --fail-on-diff, so a diff still exits 0).
215-
# Only a real tool/build error (non-zero exit) fails the job.
216-
if [ "$STATUS" != "0" ]; then
217-
echo "::error::emitter-diff failed (exit $STATUS). See the log and the emitter-diff-html artifact."
218-
exit 1
219-
fi
220-
echo "emitter-diff ran successfully (informational; diffs are reported, not enforced)."
127+
run: '[ "$STATUS" = "0" ] || { echo "::error::emitter-diff failed (exit $STATUS)."; exit 1; }'

0 commit comments

Comments
 (0)