Replace generated byte array with go:embed for templates#3597
Replace generated byte array with go:embed for templates#3597cardil wants to merge 11 commits intoknative:mainfrom
Conversation
- generate/templates/main.go: write binary templates.zip instead of Go source - generate/templates/gen.go: extract zip-writing logic (with .gitignore filtering) - generate/templates/check.go: 3-tier verification (exist/stale/hash) - generate/embed.go: //go:embed templates.zip exposes TemplatesZip []byte - delete generate/zz_filesystem_generated.go - .gitignore: add generate/templates.zip - .gitattributes: remove zz_filesystem_generated.go entry - Makefile: update targets/clean to use generate/templates.zip - hack/verify-codegen.sh: use Go checker for templates.zip verification - pkg/functions/templates_embedded.go: run whole package instead of single file - pkg/filesystem/filesystem_test.go: update stale error message Assisted-by: 🤖 Claude Sonnet 4.6
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cardil The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow
Bot
added
the
size/L
|
Would this work even if I imported function as a library (We doing that now in Functions OCP dynamic plugin)? |
|
Sorry, but we tried EDIT: Ah, I see we have a Slack thread about this.
tl;dr, This could perhaps be minimized by splitting the monolithic generated file into a per-language, per-template set of generated files. |
|
I think I have a solution. Will keep you posted... /hold |
knative-prow
Bot
added
the
do-not-merge/hold
knative-prow
Bot
added
the
do-not-merge/work-in-progress
- Rename go.mod/go.sum → .embd suffix (go:embed skips dirs with go.mod) - Convert symlinks → .symlink marker files (go:embed drops symlinks) - Add //go:build embd to Go template source files - Add templates/embed.go with //go:embed directive - Add pkg/filesystem/mangling.go: manglingFS transparently reverses mangling at runtime (strips build tags, unmangles names, restores perms) - Add hack/cmd/permissiongen: codegen for templates/.permissions manifest (preserves file permissions lost by embed.FS which returns 0444 for all) - Add hack/cmd/embd: embd/unembd subcommands for Go tooling compatibility (test-go, update-runtime-go targets need real go.mod to work) - Update Makefile: use hack/cmd/embd for test-go and update-runtime-go - Update hack/update-codegen.sh and hack/verify-codegen.sh - Remove generate/ zip infrastructure (embed.go, templates/gen.go etc) - Add .gotagsignore to exclude embd tag from CI build matrix - Export EmbdSuffix, SymlinkSuffix, EmbdBuildConstraint, StripEmbdBuildTag, AddEmbdBuildTag from pkg/filesystem for reuse by hack/cmd/embd - Use go/build/constraint for proper AST-based build tag manipulation Assisted-by: 🤖 Claude Sonnet 4.6
knative-prow-robot
added
the
needs-rebase
knative-prow
Bot
added
size/XXL
Assisted-by: 🤖 Claude Sonnet 4.6
knative-prow-robot
removed
the
needs-rebase
knative-prow
Bot
removed
the
do-not-merge/work-in-progress
|
/unhold |
knative-prow
Bot
removed
the
do-not-merge/hold
- check-go: use hack/cmd/embd unembd/embd around go vet/lint - Remove make generate/zz_filesystem_generated.go from: update-python-platform.yaml, update-ca-bundle.js, update-quarkus-platform.js, update-springboot-platform.js Assisted-by: 🤖 Claude Opus 4.6
…e-go Scaffolding symlinks (f → ../../http/) cross directory boundaries, so individual subdir unembd leaves targets in mangled state. Unmangle the whole templates/go tree at once instead.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3597 +/- ##
==========================================
+ Coverage 56.33% 56.39% +0.05%
==========================================
Files 180 180
Lines 20571 20858 +287
==========================================
+ Hits 11589 11763 +174
- Misses 7780 7881 +101
- Partials 1202 1214 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Template Go files are now part of the root module (go.mod renamed to go.mod.embd), so go mod tidy resolves their imports.
On Windows, FilesystemFromRepo returns nil for file:// URIs. Guard against nil before passing to NewManglingFilesystem to avoid nil pointer dereference in loadPerms.
|
E2E Runtime failures (go, python, quarkus, rust, typescript) are infra flakes: Docker Hub rate limiting (TOOMANYREQUESTS) and job timeouts. Go E2E TestMatrix_Run and TestMatrix_Deploy all passed — only TestMatrix_Remote was cancelled mid-run. /retest |
|
Thanks for the feedback @matejvasek @lkingland. Your comments were about the earlier version of this PR, which took a different approach. The PR has been fully reworked since then. @matejvasek good point about library import. The current approach uses @lkingland you're right that raw
A The main motivation was merge conflicts. The old The tradeoff is the mangling tooling ( |
…emplates # Conflicts: # generate/zz_filesystem_generated.go # pkg/filesystem/filesystem_test.go
…emplates # Conflicts: # generate/zz_filesystem_generated.go
|
/retest E2E Runtime failures are infra flakes: Red Hat container registry returned 502 Bad Gateway when pulling |
…emplates # Conflicts: # generate/zz_filesystem_generated.go
| @@ -0,0 +1,189 @@ | |||
| // Package main implements the embd/unembd tool for reversibly mangling | |||
There was a problem hiding this comment.
More fitting than embd/unembd would be mangele/unmangle (or demangle?) IMO.
But more importantly we should just need unmangle (unembd).
We could unmangle (unembd) templates to some temp directory and run smoke test on that temp directory.
IMO that would be more robust -- imagine test gets killed/interrupted in the middle, then templates re not re-mangled.
Furthermore the unmangling seems to be re-implemented here. The re-implementation may potentially differ/diverge from actual demangling as defined in the filesystem module. We should rather call filesystem here and do demangling.
Something like:
-
or
fsToCopy := functions.EmbeddedTemplatesFS filesystem.CopyFromFS("go", tempDir, fsToCopy)
-
fsToCopy := filesystem.NewOsFilesystem(dir) filesystem.CopyFromFS(".", tempDir, fsToCopy)
There was a problem hiding this comment.
Reimplementing unmangling may not be hard, still it best just call the unmangling code that we aready have, and more importantly the one that is actually called during func create.
There was a problem hiding this comment.
Also looking at Makefile it appears that this is called only for Go since only that has some serious manglin going on. However Java templates also have some subtle transformation it it -- the permissions restoration. I would prefer running the Java test on demangled version too, so we are sure the permissions are correctly passed trought embedding process.
There was a problem hiding this comment.
Let me take a look. I intended to reuse that package...
To the mangle/unmangle -- we need to un-mangle to upgrade deps, and then re-mangle after changes, with make update-runtime-go
There was a problem hiding this comment.
I would prefer running the Java test on demangled version too, so we are sure the permissions are correctly passed trought embedding process.
but might be overkill if we reliably test perms somewhere else.
| @@ -207,16 +203,26 @@ presubmit-unit-tests: ## Run prow presubmit unit tests locally | |||
| check-embedded-fs: ## Check the embedded templates FS | |||
There was a problem hiding this comment.
IMO We might to delete this test. It purpose is just to test that the generated file is in sync with templates content.
| func parsePermLine(line string, mode *uint32, path *string) (int, error) { | ||
| var n int | ||
| var err error | ||
| _, err = func() (int, error) { |
There was a problem hiding this comment.
Why the immediately called closure here. Sometimes closure that is immediately called makes sense, e.g.: goroutine, complex defer, defer in loop. But it does not seems to apply here?
| func (s *symlinkFileInfo) Size() int64 { return 0 } | ||
| func (s *symlinkFileInfo) ModTime() time.Time { return s.FileInfo.ModTime() } | ||
| func (s *symlinkFileInfo) IsDir() bool { return false } | ||
| func (s *symlinkFileInfo) Sys() interface{} { return nil } |
There was a problem hiding this comment.
nit: return any instead of interface{}
| // permissions in the templates/ tree, generated by hack/cmd/permissiongen. | ||
| PermissionsManifest = ".permissions" | ||
|
|
||
| permissionsManifest = PermissionsManifest |
| func (m manglingFS) Open(name string) (fs.File, error) { | ||
| f, err := m.inner.Open(name) | ||
| if err != nil { | ||
| // Try with .embd suffix (handles go.mod.embd, go.sum.embd) |
There was a problem hiding this comment.
IMO this should be done only if errors.Is(err, fs.ErrNotExist), okay in practice it always will be fs.ErrNotExist but still...
| return s.buf.Read(p) | ||
| } | ||
|
|
||
| func (s *strippedFile) Stat() (fs.FileInfo, error) { |
There was a problem hiding this comment.
Is this correct implementation? Won't this return bigger Size that the actual file data, since we are removing the Go build tags? Right now, it may not cause any troubles but if some implementation details changes this may lead to funny bugs.
| //go:embed all:go all:node all:python all:quarkus all:rust | ||
| //go:embed all:springboot all:typescript all:certs | ||
| //go:embed manifest.yaml README.md .permissions | ||
| var Content embed.FS |
There was a problem hiding this comment.
Can't we just embed "."?
There was a problem hiding this comment.
Is there rule against it?
There was a problem hiding this comment.
Okey, maybe it unnecessary embed the embed.go file but so what?
|
@lkingland PTAL |
4 participants
Summary
Replaces the codegen-based template embedding with direct
//go:embed templates/, eliminating the 14K-line generated filegenerate/zz_filesystem_generated.goand the codegen tool that produced it.Problem
The old approach used
generate/templates/main.goto walk thetemplates/directory, zip its contents in memory, and write the result as a Go byte array literal (var TemplatesZip) intogenerate/zz_filesystem_generated.go(~14K lines of hex). This required running codegen before any build or test, produced a large committed generated file, and made it harder to inspect embedded template contents or review template diffs.Solution
Use
//go:embeddirectly on thetemplates/directory tree, working around two hardgo:embedlimitations:go.modare silently skipped → renamego.mod/go.sumtogo.mod.embd/go.sum.embd.symlinkplain-text marker files containing the target pathA new
manglingFSwrapper inpkg/filesystemtransparently reverses this mangling at runtime:*.embdfiles are exposed with their real names*.symlinkfiles are presented as symlink directory entries//go:build embdconstraints are stripped from Go source file contentembed.FSwhich returns 0444 for everything) are restored from a committedtemplates/.permissionsmanifestNew Tools
hack/cmd/embd—embd/unembdsubcommands to temporarily unmangle a template directory so normal Go tooling (go mod tidy,go get,go test) can operate on it, then re-mangle it. Uses the same constants/logic asmanglingFSto keep mangling rules in sync.hack/cmd/permissiongen— codegen tool that scanstemplates/and generatestemplates/.permissionslisting non-standard file permissions (only executable files differ from the default 0644). Run viago run ./hack/cmd/permissiongen.Build tag approach
Go template source files are tagged with
//go:build embdso the Go toolchain does not attempt to compile them as part of the main module. Thego/build/constraintpackage is used for correct AST-based manipulation of build tags (preserving any existing tags on round-trips throughembd/unembd).Changes
templates/embed.go—//go:embeddirectivepkg/filesystem/mangling.go—manglingFSruntime FS wrapperpkg/functions/templates_embedded.go— wires upmanglingFShack/cmd/embd/main.go— embd/unembd CLI toolhack/cmd/permissiongen/main.go— permissions manifest codegentemplates/.permissions— committed generated manifestMakefile— updatedcheck-go,test-goandupdate-runtime-gotargetshack/update-codegen.sh/hack/verify-codegen.sh— updated.gotagsignore— excludesembdfrom CI build tag matrixgenerate/zz_filesystem_generated.go,generate/templates/main.goAssisted-by: 🤖 Claude Opus/Sonnet 4.6