Skip to content

Commit 3802cb9

Browse files
committed
fix(golden): short-circuit --new when both pins already exist + content-aware artifact digest
Two correctness fixes for the regen CLI and the harness it drives: 1. update_goldens.py --new: peek at expected.{build,package}.yaml before running _generate. When both already exist, skip the case entirely instead of running the full build+package pipeline and discarding the result. Add a regression test that monkeypatches _generate to raise so any future re-introduction of the wasted call is caught. 2. harness._packageable_replacement: include resource_id in the digest salt. After the build pass replaces local paths with <<BUILT_ARTIFACT>>, `current` is constant — so every Lambda Code in the corpus collapsed to the same S3Key, and a regression that swapped two functions' artifacts would have gone undetected. Salting on resource_id makes per-resource digests distinct (Alpha vs Beta within one template, etc.). Pins regenerated to reflect the new digests.
1 parent ddad99a commit 3802cb9

6 files changed

Lines changed: 46 additions & 14 deletions

File tree

tests/golden/harness.py

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -347,25 +347,31 @@ def _rewrite_artifacts_to_s3(template: Dict[str, Any], uploader, template_dir: s
347347
walker only triggers when the current value is a plain string
348348
(placeholder or path) — intrinsic function dicts (Fn::Sub etc.) survive.
349349
"""
350-
for _resource_id, prop_path, resource in _walk_artifact_properties(template):
350+
for resource_id, prop_path, resource in _walk_artifact_properties(template):
351351
current = _get_at_path(resource, prop_path)
352352
# Skip if not a path or placeholder; intrinsic dicts (Fn::Sub) etc. survive.
353353
if not isinstance(current, str):
354354
continue
355355
rtype = resource.get("Type")
356-
replacement = _packageable_replacement(rtype, prop_path, current, template_dir)
356+
replacement = _packageable_replacement(
357+
rtype, prop_path, current, template_dir, resource_id
358+
)
357359
_set_at_path(resource, prop_path, replacement)
358360

359361

360362
def _packageable_replacement(
361-
rtype: str, prop_path: str, current: Any, template_dir: str
363+
rtype: str, prop_path: str, current: Any, template_dir: str, resource_id: str
362364
) -> Any:
363365
"""Compute the deterministic replacement for an artifact property.
364366
365367
Returns either a dict ({"S3Bucket": ..., "S3Key": ...}) or a string
366368
("s3://...") depending on the resource type. Sentinel: hashes
367-
"<current>|<rtype>|<prop_path>" so each case has a stable URI without
368-
needing real file content.
369+
"<current>|<rtype>|<prop_path>|<resource_id>" so each resource gets a
370+
distinct URI even when ``current`` is the constant
371+
``BUILT_ARTIFACT_PLACEHOLDER`` after the build pass. Including
372+
``resource_id`` is what makes the digest content-aware: without it,
373+
every Lambda Code in the corpus would collapse to the same S3Key and a
374+
regression that swapped two functions' artifacts would go undetected.
369375
370376
Shapes covered (extend as new corpus cases require new shapes):
371377
- AWS::Lambda::Function .Code -> {"S3Bucket": ..., "S3Key": ...}
@@ -374,7 +380,7 @@ def _packageable_replacement(
374380
- All other artifact properties -> "s3://<bucket>/<sha256>"
375381
"""
376382
digest = hashlib.sha256(
377-
f"{current}|{rtype}|{prop_path}".encode("utf-8")
383+
f"{current}|{rtype}|{prop_path}|{resource_id}".encode("utf-8")
378384
).hexdigest()
379385
# Lambda image — must check before the .Code dict shape
380386
if rtype == "AWS::Lambda::Function" and prop_path == "Code.ImageUri":

tests/golden/templates/language_extensions/foreach_static_zip/expected.package.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ Resources:
44
Properties:
55
Code:
66
S3Bucket: golden-bucket
7-
S3Key: cbe90f4157bf2048dddade15fccab9bb6ea1ff37413fe0f78b7543e206e8910d
7+
S3Key: 5e959f785bd23fcb9c18696f2b6855d4c9fbe5130a1e06aff5c4aa4d0091383c
88
Handler: app.handler
99
Role:
1010
Fn::GetAtt:
@@ -36,7 +36,7 @@ Resources:
3636
Properties:
3737
Code:
3838
S3Bucket: golden-bucket
39-
S3Key: cbe90f4157bf2048dddade15fccab9bb6ea1ff37413fe0f78b7543e206e8910d
39+
S3Key: 0169c64c6d32a8001c8f727f32f9b538ba4e1479e47262b4258f969a345a3f51
4040
Handler: app.handler
4141
Role:
4242
Fn::GetAtt:

tests/golden/templates/packageable_resources/lambda_function_zip/expected.package.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ Resources:
44
Properties:
55
Code:
66
S3Bucket: golden-bucket
7-
S3Key: cbe90f4157bf2048dddade15fccab9bb6ea1ff37413fe0f78b7543e206e8910d
7+
S3Key: 17c3a49c87d6d228b7f4e7b016472d9da14ad548c986f67d3cc3c42365dabcf4
88
FunctionName: hello
99
Handler: app.handler
1010
MemorySize: 128

tests/golden/templates/sam_resources/serverless_function_zip/expected.package.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ Resources:
44
Properties:
55
Code:
66
S3Bucket: golden-bucket
7-
S3Key: cbe90f4157bf2048dddade15fccab9bb6ea1ff37413fe0f78b7543e206e8910d
7+
S3Key: 17c3a49c87d6d228b7f4e7b016472d9da14ad548c986f67d3cc3c42365dabcf4
88
Handler: app.handler
99
MemorySize: 128
1010
Role:

tests/golden/test_update_goldens.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,3 +88,17 @@ def test_new_only_writes_missing_pins(isolated_corpus):
8888
assert (sam_case / "expected.build.yaml").exists()
8989
# LE case stays stale (--new does not touch existing pins)
9090
assert le_pin.read_text() == "STALE\n"
91+
92+
93+
def test_new_short_circuits_when_all_pins_exist(isolated_corpus, monkeypatch):
94+
"""When every case is fully pinned, --new must not invoke _generate."""
95+
# Pre-pin everything (uses real _generate).
96+
update_goldens.main([])
97+
98+
# Now poison _generate so a single call would blow up.
99+
def boom(*_args, **_kwargs):
100+
raise AssertionError("_generate must not be called when all pins exist under --new")
101+
102+
monkeypatch.setattr(update_goldens, "_generate", boom)
103+
rc = update_goldens.main(["--new"])
104+
assert rc == 0

tests/golden/update_goldens.py

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -111,14 +111,26 @@ def main(argv: Optional[List[str]] = None) -> int:
111111

112112
for case_dir in _iter_cases(args.filter):
113113
case_rel = str(case_dir.relative_to(TEMPLATES_ROOT))
114+
115+
# --new fast path: if both pins already exist, skip the build+package
116+
# pipeline entirely. Only invoke _generate when at least one pin is
117+
# missing, then write only the missing files.
118+
if args.new:
119+
build_existing = _existing(case_dir, "build")
120+
pkg_existing = _existing(case_dir, "package")
121+
if build_existing is not None and pkg_existing is not None:
122+
continue
123+
build_yaml, pkg_yaml = _generate(case_dir)
124+
if build_existing is None:
125+
(case_dir / "expected.build.yaml").write_text(build_yaml, encoding="utf-8")
126+
if pkg_existing is None:
127+
(case_dir / "expected.package.yaml").write_text(pkg_yaml, encoding="utf-8")
128+
continue
129+
114130
build_yaml, pkg_yaml = _generate(case_dir)
115131

116132
for kind, generated in (("build", build_yaml), ("package", pkg_yaml)):
117133
existing = _existing(case_dir, kind)
118-
if args.new:
119-
if existing is None:
120-
(case_dir / f"expected.{kind}.yaml").write_text(generated, encoding="utf-8")
121-
continue
122134
if dry_run:
123135
if existing != generated:
124136
any_diff = True

0 commit comments

Comments
 (0)