Skip to content

Commit fd5e38c

Browse files
bnusunnybnusunny
committed
docs(designs): reframe motivation around review-surface for output shape
The previous motivation leaned on a retroactive "we'd have caught the four bugs" claim. That claim is partially false: pre-#8637, SAM CLI didn't process AWS::LanguageExtensions locally, so a pre-existing LE corpus pinning behavior nobody had committed to would not have been written in the first place. The right moment to add the corpus is the feature PR itself, not before. Replace the motivation with the actual gap: existing tests verify internals (unit) and narrow output assertions (integration), but neither puts the rendered template in front of a human reviewer at PR time. Subtle interactions between pipeline stages — LE expansion vs. SAM transform vs. global-transform export vs. artifact-path merge — are invisible at review time. #8637 is exhibit A: the four post-release bugs are each a YAML-shape diff a reviewer would have caught on sight, if the diff had been visible. Also reframe the semver pitch around the actual customer impact: the most impactful behavior changes don't merely surprise tooling, they break customer CI/CD pipelines outright. #9004 is the canonical example — sam build raised InvalidTemplateException on any template using !FindInMap over a parameter without Default, blocking every pipeline that consumed it. The 1.159 → 1.160 bump was minor per semver, signalling "additive, backwards-compatible" — but local LE processing turning on by default broke working pipelines, which is the definition of a backwards-incompatible change. The version number didn't tell consumers to audit their pipelines, so they didn't.
1 parent 656f93c commit fd5e38c

1 file changed

Lines changed: 87 additions & 20 deletions

File tree

‎designs/golden_templates_and_semver_check.md‎

Lines changed: 87 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -4,26 +4,93 @@ Title: Golden Templates and Semver Check
44
What is the problem?
55
--------------------
66

7-
PR #8637 (sam-cli 1.160.0) shipped in-tree CloudFormation Language Extensions
8-
(LE) support. Within weeks, four production regressions surfaced — every one
9-
a behavior shift in the *expanded-template output* that no automated test
10-
caught before release:
11-
12-
- #9004 — `Fn::FindInMap` / `Join` / `Select` / `Base64` raised on unresolved
13-
parameter `Ref`s in PARTIAL mode (fixed in #9010).
14-
- #9005 — `PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES` had drifted from canonical
15-
resource lists; `AWS::Serverless::Application` and seven sibling types
16-
failed packaging (fixed in #9009).
17-
- #9027 — `AWS::Include` buried inside `Fn::ToJsonString` was invisible to the
18-
global-transform walker because LE expansion ran first (fixed in #9030).
19-
- #9029 — LE-aware build merge overwrote `Code: { ZipFile: !Sub ... }` with
20-
the LE-resolved value, baking pseudo-param defaults into the built template
21-
(fixed in #9031).
22-
23-
PR #9033 made local LE processing opt-in to give customers a clean migration
24-
path. The action item from that retrospective: a golden-template corpus that
25-
pins the expanded output of representative templates, plus a CI gate that
26-
requires a major-version bump whenever a pin changes.
7+
SAM CLI processes user templates and writes the result to disk for
8+
CloudFormation to consume. The output template is the contract with users
9+
and their tooling — yet the existing test suite has no review surface for
10+
that output's *shape*. Unit tests verify individual functions in
11+
isolation; integration tests assert exit codes and a few narrow
12+
properties (`Resources.X.Type == ...`); neither puts the rendered
13+
template in front of a human reviewer.
14+
15+
This shows up most painfully in feature PRs that introduce new
16+
output-shaping behavior. The reviewer can read the code and run the
17+
tests, but cannot easily see *what the feature produces for representative
18+
inputs* — the diff in the PR is `samcli/lib/...` Python code, not the
19+
YAML that customers will deploy. Subtle interactions between pipeline
20+
stages — LE expansion vs. SAM transform vs. global-transform export
21+
vs. artifact-path merge — are invisible at review time.
22+
23+
PR #8637 (sam-cli 1.160.0, in-tree CFN Language Extensions support) is
24+
exhibit A. It introduced large new output-shaping behavior across the
25+
build and package paths. Within weeks of release four bugs surfaced,
26+
each a behavior shift visible in the post-build or post-package
27+
template that no automated test caught before release:
28+
29+
- #9004 — `Fn::FindInMap` / `Join` / `Select` / `Base64` raised on
30+
unresolved parameter `Ref`s in PARTIAL mode (fixed in #9010).
31+
- #9005 — `PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES` had drifted from
32+
canonical resource lists; `AWS::Serverless::Application` and seven
33+
sibling types failed packaging (fixed in #9009).
34+
- #9027 — `AWS::Include` buried inside `Fn::ToJsonString` was invisible
35+
to the global-transform walker because LE expansion ran first (fixed
36+
in #9030).
37+
- #9029 — LE-aware build merge overwrote `Code: { ZipFile: !Sub ... }`
38+
with the LE-resolved value, baking pseudo-param defaults into the
39+
built template (fixed in #9031).
40+
41+
PR #9033 made local LE processing opt-in to give customers a clean
42+
migration path.
43+
44+
These bugs didn't slip past #8637 because the test suite was small —
45+
the LE module shipped with comprehensive unit tests and integration
46+
tests. They slipped past because nobody was asked, at PR-review time,
47+
to look at the actual templates `sam build` and `sam package` produce
48+
for templates exercising each new LE intrinsic. Each bug is a YAML-shape
49+
diff that a reviewer would have caught on sight — if the diff had been
50+
visible.
51+
52+
The right moment for that review surface is the feature PR itself.
53+
#8637 should have shipped with one pinned input/output pair per LE
54+
intrinsic plus the cross-cutting interactions (LE × nested stacks, LE
55+
× AWS::Include, LE × Globals). Each iteration on the PR would have
56+
produced output diffs alongside code diffs; the four bugs would have
57+
appeared as visible shape changes during development, before the PR
58+
merged. That same pattern then carries forward: every future feature
59+
PR pins what it produces, every patch release is auditable for
60+
behavior changes.
61+
62+
A pinned-output corpus solves only half the problem, though. Behavior
63+
changes — intentional or not — must also be visible to *consumers* of
64+
SAM CLI, not just to reviewers. The most impactful changes don't just
65+
alter output shape: they cause `sam build` or `sam package` to fail
66+
outright, breaking customer CI/CD pipelines on what was assumed to be
67+
a routine upgrade. #9004 is the canonical example — `sam build`
68+
started raising `InvalidTemplateException` on any template that used
69+
`!FindInMap` over a parameter without a `Default`, blocking every CI
70+
that consumed such a template until the customer either pinned to
71+
1.159.x or upgraded past the fix. The 1.159 → 1.160 bump was minor
72+
per semver, signalling "additive, backwards-compatible" — but local
73+
LE processing turning on by default broke working pipelines, which
74+
is the definition of a backwards-incompatible change. The version
75+
number didn't tell consumers to audit their pipelines, so they
76+
didn't.
77+
78+
This design proposes:
79+
80+
1. **A golden-template corpus** under `tests/golden/templates/`.
81+
For each representative input, pin the post-build and post-package
82+
YAML byte-for-byte. The pins live in the repo; PR diffs include
83+
both the code change and the resulting output change. Reviewers see
84+
what users will see.
85+
86+
2. **A CI semver gate** that requires a major-version bump in
87+
`samcli/__init__.py` whenever an existing pinned output is modified
88+
or deleted. New pins (corpus growth) are ungated. The gate makes
89+
user-visible behavior changes explicit at version-number granularity
90+
so downstream tooling can react deliberately.
91+
92+
The two compose: goldens make output changes *visible* to reviewers;
93+
the semver check makes them *commitments* to consumers.
2794

2895
What will be changed?
2996
---------------------

0 commit comments

Comments
 (0)