Skip to content

Changes from statik to go:embed for example and extension templates #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Apr 7, 2021
Merged

Changes from statik to go:embed for example and extension templates #129

merged 7 commits into from
Apr 7, 2021

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Mar 24, 2021 •
edited
Loading

This reduces build complexity by eliminating a generation step with go:embed. This implicitly reduces tech debt even more because not only were we using a stalled project, statik, but also a fork of it.

go:embed is not perfect, as it disallows the file name go.mod. The workaround is to rename it to go.mod_ per golang/go#45197. While this is imperfect an if-statement is a far better punch than a dependency on a fork of a stalled project which also requires a codegen step.

Fixes #126

@codefromthecrypt codefromthecrypt mentioned this pull request Mar 24, 2021
Closed
@codefromthecrypt codefromthecrypt force-pushed the goembed branch 2 times, most recently from 4f12463 to ed15f0d Compare March 24, 2021 01:35
@codefromthecrypt
Copy link
Contributor Author

One simple workaround could be to serve the go.mod files dynamically in go. This could stay until there's a go.mod solution in go:embed, and still get us off the abandoned library and more complicated build.

Each of the go.mod files are identical.
data/extension/init/templates/tinygo/envoy.access_loggers/default/go.mod
data/extension/init/templates/tinygo/envoy.filters.http/default/go.mod
data/extension/init/templates/tinygo/envoy.filters.network/default/go.mod

module {{ .Extension.Name }}

go 1.15

require (
	github.com/stretchr/testify v1.6.1
	github.com/tetratelabs/proxy-wasm-go-sdk v0.1.0
)

@codefromthecrypt
Copy link
Contributor Author

might be saved by the bell, as I mistook the root path used in statik. it was data/example/init not data/extension/init. The latter has go, but the former doesn't.

🤞

@codefromthecrypt
Copy link
Contributor Author

OK I think the go.mod in the extensions dir seems to be about our lint setup. If all tests pass except lint, it might be a better punch to avoid that lint check vs override go's behavior or rely on codegen.

In a bit, I'll raise an issue in go to document the surprising limitation about go.mod files. I can understand how someone could equate presence of go.mod meaning the directory is no longer in the module, but it is surprising enough to have an issue to point to and ideally docs about.

@codefromthecrypt
Copy link
Contributor Author

raised golang/go#45197 on the constraint about go.mod. Not sure if having a go.mod is critical in tinygo or not. It seems that it trips our lint meanwhile.

need a hand on the lint failure as it isn't consistent running locally. I tried to exclude but guess I got something wrong.

ERRO Running error: context loading failed: failed to load program with go/packages: -: go: downloading github.com/stretchr/testify v1.6.1

@yskopets
Copy link
Member

Not sure if having a go.mod is critical in tinygo or not.

The goal of GetEnvoy Extension Toolkit is help a user to kick off extension development that follows all best practices (as opposed to starting from a "Hello World" and learning painfully it's not production-ready in any way).

All modern Go project should use Go modules. So, we have to provide go.mod out-of-the-box one way or another.

@yskopets
Copy link
Member

One simple workaround could be to serve the go.mod files dynamically in go. This could stay until there's a go.mod solution in go:embed, and still get us off the abandoned library and more complicated build.

Personally, I don't see what features of go:embed justify the need in such workarounds.

I earlier said that "go:embed provides exactly the same features as statik but has slightly different API". Apparently, it's not the case - go:embed is a less feature-complete solution.

Again, I'm just one vote. If other reviewers are fine with it, I won't object.

@codefromthecrypt
Copy link
Contributor Author

The following suggestion seems alright to me, provided we document it golang/go#45197 (comment) basically rename go.mod to go.mod_

We should keep in mind that the file is already templated, so it isn't valid anyway.

@codefromthecrypt
Copy link
Contributor Author

FWIW I'd like to finish #130 before merging this anyway because testing is where I lost most time

codefromthecrypt
codefromthecrypt commented Mar 25, 2021
View reviewed changes
data/extension/init/templates/tinygo/.gitignore Outdated
@@ -0,0 +1,2 @@
# We rename go.mod to go.mod_ to workaround https://github.com/golang/go/issues/45197
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mathetake this will only effect tinygo as I don't expect any other extension language to want a file named go.mod in it.

codefromthecrypt
codefromthecrypt commented Mar 25, 2021
View reviewed changes
pkg/extension/init/init.go Outdated
@@ -102,7 +102,12 @@ func (s *scaffolder) walk(sourceDirName, destinationDirName string) (errs error)
}

func (s *scaffolder) visit(sourceDirName, destinationDirName string, sourceFileInfo os.FileInfo) (errs error) {
relOutputFileName := filepath.Join(destinationDirName, sourceFileInfo.Name())
baseOutputFileName := sourceFileInfo.Name()
// We rename go.mod to go.mod_ to workaround https://github.com/golang/go/issues/45197
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is far far easier than writing an FS renamer, especially considering we are already walking the file tree. However, I linked the issue as it is soliciting a library to do that.

codefromthecrypt
codefromthecrypt commented Mar 25, 2021
View reviewed changes
@codefromthecrypt
Copy link
Contributor Author

good news is the tinygo tests pass now.

Not sure about the rust failures haven't looked deeply yet

           /tmp/cache/getenvoy/extension/rust-builder/cargo/registry/src/github.com-1ecc6299db9ec823/proxy-wasm-experimental-0.0.8/src/hostcalls.rs:919: undefined reference to `proxy_done'
          collect2: error: ld returned 1 exit status

@codefromthecrypt
Copy link
Contributor Author

will rebase after #135 and look into adding unit tests that show the templates directory tree isn't empty due to faulty go:embed setup.

@codefromthecrypt
Copy link
Contributor Author

pulled the lint thing out to here #138 and will rebase later

@codefromthecrypt codefromthecrypt mentioned this pull request Mar 26, 2021
Merged
@codefromthecrypt codefromthecrypt changed the base branch from master to start-migration March 26, 2021 07:45
@codefromthecrypt
Copy link
Contributor Author

changed merge base to start-migration until #130 is in, or I figure out what the glitches are. debugging before that is arduous.

@codefromthecrypt
Copy link
Contributor Author

Thanks for the tip by @carlmjohnson I didn't realize that dot directories are filtered in go:embed expressions. I renamed .cargo to cargo to work around this.

Summary is two if statements which would be common to users who understand go:embed pros and cons instead of a codegen phase powered by a fork of a fork. Dirty, but less dirty.

@codefromthecrypt
Copy link
Contributor Author

ps it isn't strictly required to rename the config file to config.toml, but I noticed that the old naming isn't encouraged anymore per https://doc.rust-lang.org/cargo/reference/config.html so fixed it while here.

@codefromthecrypt codefromthecrypt force-pushed the start-migration branch 2 times, most recently from 155bbde to b24f531 Compare March 30, 2021 07:39
@codefromthecrypt
Copy link
Contributor Author

will look into this again after #130 and #151 are merged because it is a lot easier to debug things that way.

Base automatically changed from start-migration to master March 31, 2021 05:45
@codefromthecrypt
Copy link
Contributor Author

I think we're having docker problems. I'll re-kick this tomorrow, but I think the code change is done now. There's a test coverage issue on "getenvoy extension init" which I'll do in a separate PR, so that there's nothing needed here.

@codefromthecrypt
Copy link
Contributor Author

#161 finishes backfilling the tests needed for here

@codefromthecrypt codefromthecrypt force-pushed the goembed branch 2 times, most recently from 72020bd to 12af57c Compare April 6, 2021 11:36
@codefromthecrypt
Copy link
Contributor Author

circleci is no longer in use, so we can ignore that

codefromthecrypt
codefromthecrypt commented Apr 6, 2021
View reviewed changes
go.mod Outdated
@@ -36,6 +36,7 @@ require (
github.com/tetratelabs/getenvoy-package v0.0.0-20190730071641-da31aed4333e
github.com/tetratelabs/log v0.0.0-20190710134534-eb04d1e84fb8
github.com/tetratelabs/multierror v1.1.0
github.com/tetratelabs/proxy-wasm-go-sdk v0.12.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

one glitch of renaming go.mod -> go.mod_ is that if you do go mod tidy, this pops into the root go.mod only because the examples import it. I'll look into it..

Adrian Cole added 2 commits April 7, 2021 09:17
Changes from statik to go:embed for examples serving
b8c0ad2 
This reduces build complexity by eliminating a generation step with go:embed. This implicitly reduces tech debt even more because not only were we using a stalled project, statik, but also a fork of it.

go:embed is not perfect, as it disallows the file name go.mod. The workaround is to rename it to go.mod_ per golang/go#45197. While this is imperfect an if-statement is a far better punch than a dependency on a fork of a stalled project which also requires a codegen step.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Adds RATIONALE.md
554af22 
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt changed the title Changes from statik to go:embed for examples serving Changes from statik to go:embed for example and extension templates Apr 7, 2021
format
ba836c1 
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt marked this pull request as ready for review April 7, 2021 01:21
mathetake
mathetake reviewed Apr 7, 2021
View reviewed changes
typos and restores chmod hack
74b8fff 
Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt and others added 2 commits April 7, 2021 09:39
@codefromthecrypt @mathetake
typos
854d5bf 
Signed-off-by: Adrian Cole <adrian@tetrate.io>

Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>
less pasta
b5af0b9 
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt merged commit 212d750 into master Apr 7, 2021
@codefromthecrypt codefromthecrypt deleted the goembed branch April 7, 2021 04:48
@codefromthecrypt
Copy link
Contributor Author

Thanks for the review @mathetake!

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

@mathetake mathetake mathetake approved these changes

@yskopets yskopets Awaiting requested review from yskopets

@musaprg musaprg Awaiting requested review from musaprg

Assignees
No one assigned
Labels
tech debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can go:embed replace statik?
3 participants
@codefromthecrypt @yskopets @mathetake