fix: address review comments from Wave 0 foundation PRs#53
Merged
Conversation
Substantive review feedback on the merged foundation PRs: go-check.yml (#47): - Remove unused `fuzz-timeout` input. The fuzz job runs corpus replay (`go test -run='^Fuzz'`), not active fuzzing, so the `-fuzztime` flag that would have consumed this input is never emitted. Dead input; callers setting it were silently ignored. - Pin `go-licenses` to commit 3e084b0 (was `@latest`, fragile). - Strict-mode the `pre-build-cmd` step: `bash -eu -c` so caller script errors fail CI instead of silently limping into a broken go build. Applied across all 6 jobs that use the pre-build hook. ghcr-retention.yml (#47): - Pin `setup-crane` to v0.22.1 (was `version: latest`, fragile). templates dependabot.yml (#50): - Add `groups: {devcontainers: {patterns: ['*']}}` to the devcontainers entry so it matches the shape of the other three ecosystems (gomod / github-actions / docker). Previously absent → devcontainer bumps would have opened one PR per package instead of grouped. sync-template.sh (#51): - Remove `trap ... RETURN` from the intentional-drift rsync path. `RETURN` only fires on function-return or sourced-script exit, not on main-script end, so it never cleaned up. Script already had an explicit `rm -f "$EXCLUDES"` at end of scope; the trap was dead weight. Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
This PR applies follow-up fixes to the Wave 0 Go workflow/templates foundation by tightening workflow reproducibility, aligning template config consistency, and adjusting the template sync script behavior.
Changes:
- Updates reusable workflows to remove an unused input, run pre-build commands in stricter mode, and pin
go-licenses. - Pins the installed
craneversion inghcr-retention.ymlto avoidlatestregressions. - Adds Dependabot grouping for
devcontainersupdates in both Go templates and removes a brokentrap ... RETURNfrom the sync script.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
templates/go-lib/.github/dependabot.yml |
Adds a devcontainers Dependabot group for consistent PR batching. |
templates/go-app/.github/dependabot.yml |
Adds a devcontainers Dependabot group for consistent PR batching. |
scripts/sync-template.sh |
Removes an ineffective RETURN trap during exclude-file generation. |
.github/workflows/go-check.yml |
Removes unused fuzz-timeout input, pins go-licenses, and changes pre-build execution flags. |
.github/workflows/ghcr-retention.yml |
Pins crane installation version instead of using latest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CybotTM
added a commit
that referenced
this pull request
Apr 20, 2026
PR #53 added `bash -eu` to all 6 pre-build-cmd steps in go-check.yml but omitted `-o pipefail`. A pipeline with a failing first stage (e.g. `bun run build:assets | tee logs/build.log`) would have its non-zero exit swallowed by the last stage's success. Copilot called this out across all 6 places in #53. Upgrade to `bash -euo pipefail -c` for consistency. Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
CybotTM
added a commit
that referenced
this pull request
Apr 20, 2026
Follow-up to #53 — `bash -eu` missed `-o pipefail`, so pipeline failures in caller-supplied pre-build scripts would be swallowed. 6 call sites updated.
CybotTM
added a commit
that referenced
this pull request
Apr 20, 2026
The SHA I pinned in #53 (3e084b0caf71) is actually tag v2.0.1 — its go.mod declares module path `github.com/google/go-licenses/v2`, so `go install github.com/google/go-licenses@<that-sha>` fails with: go.mod has post-v1 module path "github.com/google/go-licenses/v2" at revision 3e084b0caf71 Caught on simple-ldap-go#142's license-scan job. Repin to v1.6.0 (5348b744) which keeps the v1 module path that the installer expects. Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
CybotTM
added a commit
that referenced
this pull request
Apr 20, 2026
Fix bad pin introduced in #53 — the SHA I picked was v2.0.1 whose go.mod uses the `/v2` module suffix, so the install command in go-check.yml fails. Repin to v1.6.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses substantive feedback from review threads on the merged PRs #47, #50, #51:
fuzz-timeoutinput, pingo-licenses, strict-modepre-build-cmd(bash -eu -c)setup-craneversiongroupsto the devcontainers entry for consistencytrap ... RETURN(fires on function return, not script exit)