Skip to content

ci(examples): cover go-http-zip and standardize template filenames - #759

Open
JamBalaya56562 wants to merge 2 commits into
aws:mainfrom
JamBalaya56562:pr759
Open

ci(examples): cover go-http-zip and standardize template filenames#759
JamBalaya56562 wants to merge 2 commits into
aws:mainfrom
JamBalaya56562:pr759

Conversation

@JamBalaya56562

@JamBalaya56562 JamBalaya56562 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Problem

The validate job discovers SAM templates with a find that matches one exact filename:

find examples -maxdepth 2 -name "template.yaml"

Two examples spelled it template.yml instead — go-http-zip and sinatra — so find never matched them and both were silently skipped by template validation. go-http-zip was missing from the test-zip matrix as well, which left it with no CI coverage at all.

Changes

Rename template.ymltemplate.yaml in go-http-zip and sinatra.

These were the last two using the short spelling, so every example now shares one filename and find picks them all up. SAM auto-detects either spelling, so this is a no-op for anyone deploying the examples — the rename exists purely so CI stops skipping them.

Add go-http-zip to the test-zip matrix, asserting Hello, Website! on /.

The entry sets port: "3000" rather than the 8000 its neighbours use, because the app hardcodes http.ListenAndServe(":3000", nil); that matches the PORT: 3000 its template already passes to the adapter.

Fail the validate job when any example uses template.yml (second commit, added in response to review feedback).

Renaming the two stragglers closes today's gap but leaves the failure mode intact — the next example added as template.yml would be skipped just as silently. A guard ahead of the validation loop turns that filename mismatch into a red job instead of invisible missing coverage. It searches at any depth, so it also covers nested examples that the -maxdepth 2 loop would not reach.

Notes

CI coverage only — no adapter or runtime code is touched, and the renamed templates are byte-identical (100% similarity) to their originals. The repository currently has zero template.yml files, so the new guard passes as-is.


/cc @bnusunny — apologies for the ping. This has been open since June 5 without triage, so I wanted to surface it in case it slipped through. No rush on my end, and I'm happy to rebase or split it up if that makes review easier.

@JamBalaya56562
JamBalaya56562 requested a review from a team as a code owner June 5, 2026 21:52
go-http-zip used template.yml, which the validate job's find (template.yaml)
never matched, so the example was untested. Rename the two stragglers
(go-http-zip, sinatra) to the conventional template.yaml -- SAM auto-detects
either name, so there is no functional change -- and add go-http-zip to the
test-zip matrix.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@aws-sam-tooling-bot aws-sam-tooling-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Results

Reviewed: 34d3a29..78f18a8
Files: 3
Comments: 1

- { name: fasthtml-zip, path: /, expect_body: "Hello World", port: "8000" }
- { name: flask-zip, path: /, expect_body: "message", port: "8000" }
- { name: gin-zip, path: /, expect_body: "message", port: "8000" }
- { name: go-http-zip, path: /, expect_body: "Hello, Website!", port: "3000" }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] The matrix entry and the renames are consistent with the example (cmd/website/main.go hardcodes :3000 and the template sets PORT: 3000, so port: "3000" and expect_body: "Hello, Website!" both match), but the underlying cause of go-http-zip going untested is still present.

The validate job at line 40 discovers templates with:

for template in $(find examples -maxdepth 2 -name "template.yaml" | sort); do

Any example whose template is not named exactly template.yaml is skipped silently and the job still passes green — which is precisely how go-http-zip stayed unvalidated. Standardizing the two existing filenames fixes today's gap but relies on convention to prevent recurrence; the next contributor who adds template.yml (a name SAM accepts) reintroduces the same invisible hole.

Making discovery match both names keeps the loop honest without depending on reviewers to catch the filename:

for template in $(find examples -maxdepth 2 \( -name "template.yaml" -o -name "template.yml" \) | sort); do

If you prefer to keep enforcing the single conventional name, an explicit failure is better than a silent skip — e.g. fail the job when an example directory contains a template.yml, so the mismatch surfaces in CI rather than as missing coverage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the underlying point is right, so I've addressed it in a follow-up commit.

I went with the second suggestion rather than the first. Matching both spellings in find would fix the silent skip, but it also re-legitimises the template.yml name this PR just removed, so filename drift would stay permanently acceptable. Failing the job keeps a single conventional name and enforces it in CI instead of relying on reviewers to spot it.

The guard searches at any depth, so it also covers nested examples the -maxdepth 2 loop would miss. The repository has zero template.yml files today, so it passes as-is.

The validate job discovers templates with `find -name "template.yaml"`, so an
example using the template.yml spelling is skipped silently and the job still
passes green. Standardizing the two existing filenames closes today's gap but
leaves the failure mode intact: the next example added as template.yml is
unvalidated again, with no signal.

Reject the other spelling before the validation loop runs, so a filename
mismatch surfaces as a failed job instead of as missing coverage.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@aws-sam-tooling-bot aws-sam-tooling-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Results

Reviewed: 34d3a29..e8e49bd
Files: 3
Comments: 1

# coverage.
- name: Enforce template.yaml naming
run: |
stragglers=$(find examples -name "template.yml")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] The guard closes the exact case that bit go-http-zip, but it hard-codes the inverse of the discovery rule instead of deriving from it, so the two can still disagree:

# guard (new): any depth, only the .yml spelling
find examples -name "template.yml"

# discovery (line 51 loop): depth <= 2, only the .yaml spelling
find examples -maxdepth 2 -name "template.yaml"

Anything that is a SAM template but doesn't land at exactly examples/<name>/template.yaml — e.g. examples/foo/infra/template.yaml — passes the guard and is still skipped silently by validation, which is the same green-but-uncovered outcome the step exists to prevent. Conversely, the guard has no -maxdepth, so a nested third-party template.yml that validation was never meant to lint would fail the build.

Expressing the invariant once ("every SAM template lives at examples/<name>/template.yaml") covers both directions:

stragglers=$(find examples -name 'template.y*ml' | grep -vE '^examples/[^/]+/template\.yaml$' || true)
if [ -n "$stragglers" ]; then
 echo "These templates must be moved to examples/<name>/template.yaml:"
 echo "$stragglers"
 exit 1
fi

Worth pairing with a zero-match assertion in the validation loop itself — it currently exits 0 when find returns nothing, so a future change to the directory layout would also read as a pass rather than a failure.

I confirmed the rest of the change: no references to template.yml remain outside the diff, the renamed templates match the shapes of examples the validate job already lints (go-http-zip mirrors gin-zip, sinatra mirrors fastapi-response-streaming), and the matrix entry's port: "3000" / expect_body: "Hello, Website!" line up with cmd/website/main.go and the template's PORT: 3000. No example currently has a template below depth 2, so the point above is about keeping the guard honest going forward, not a present-day break.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both directions are real, and I'd rather not encode the stricter rule here.

The proposed invariant enforces that every SAM template sits at examples/<name>/template.yaml, but the repo already nests examples one level deeper — examples/datadog/*/cdk/ and examples/sls/nestjs/ — they just don't use SAM today. A future examples/datadog/fastapi/template.yaml would follow the existing grouping and still fail the guard, and the right fix in that case is widening the validation loop's -maxdepth 2, not rejecting the layout.

Since nothing currently has a template below depth 2 (as you note), I'd rather keep this PR to the coverage gap it set out to close and leave the depth question to a change that can decide the nested-example policy properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant