Skip to content

Go modules - unable to get a sub-package to work #33

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.

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

Closed
alexellis opened this issue Jan 24, 2020 · 7 comments · Fixed by #34
Closed

Go modules - unable to get a sub-package to work #33

alexellis opened this issue Jan 24, 2020 · 7 comments · Fixed by #34

Comments

@alexellis
Copy link
Member

alexellis commented Jan 24, 2020

Description

When adding a sub-package to a function using golang-middleware and Go modules, I'm unable to get it to build. The unit tests fails immediately, then the go build fails if the test step is commented out.

Step 17/29 : RUN go build --ldflags "-s -w" -a -installsuffix cgo -o handler .
 ---> Running in cd54e2e66fb9
build handler: cannot load github.com/alexellis/vend/vend/handlers: git ls-remote -q origin in /go/pkg/mod/cache/vcs/15c84a3c22a2d4367cca03dcf38500ba216f1f2e95fa3fdd4255f8434c417d89: exit status 128:
        fatal: could not read Username for 'https://github.com': terminal prompts disabled
Confirm the import path was entered correctly.
If this is a private repository, see https://golang.org/doc/faq#git_https for additional information.

Sample repo

https://github.com/alexellis/golang-middleware-relative-import

The top-level entry-point is main.go

main.go is a module and references a folder "function" which is also a module.

The basic path of a function handler with no other packages seems to work without Go modules, but when adding a sub-package i.e. handlers and referencing that, the tests and build fail.

main -> function -> handlers (sub-package)

This should have been a basic happy path scenario tested in #24, so I'm surprised to see that this didn't work. Pinging the people who tested / reviewed the PR.

@ewilde @LucasRoesler @bmcstdio @jonatasbaldin @stefanprodan

I imagine this is the same issue for go and golang-http also.

When using dep and a vendor folder this can be worked-around so that it works in the container, but not on a local system. The ideal resolution would have OpenFaaS functions with sub-packages working well in an IDE with highlighting and goreturns etc all working fine, and also working in a docker build.

@LucasRoesler
Copy link
Member

Per the official wiki https://github.com/golang/go/wiki/Modules#gomod

exclude and replace directives only operate on the current (“main”) module. exclude and replace directives in modules other than the main module are ignored when building the main module. The replace and exclude statements, therefore, allow the main module complete control over its own build, without also being subject to complete control by dependencies. (See FAQ below for a discussion of when to use a replace directive).

alexellis added a commit that referenced this issue Jan 24, 2020
This is part of a consistency exercise for #33 - the go mod
version now matches the Go version in the Dockerfile.

This was queried with @bmcstdio, but we got no response on his
PR.

#24 (comment)

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>
@alexellis
Copy link
Member Author

@LucasRoesler can you highlight what you want us to take from that quote, it isn't obvious to me what I'm supposed to get from it.

@LucasRoesler
Copy link
Member

only operate on the current (“main”) module

@LucasRoesler
Copy link
Member

this means nested uses of replace will not work

@alexellis
Copy link
Member Author

alexellis commented Jan 24, 2020

There is an alternative without Go modules which appears to work in Docker, but does not work properly on the local system.

In the repo, I can set this relative import, and then turn off go modules -> faas-cli build --build-arg GO111MODULE=off - then both the tests and the build work fine.

package function

import (
	"net/http"

	"handler/function/handlers"
)

func Handle(w http.ResponseWriter, r *http.Request) {

	handlers.Echo(w, r)
}

Setting "handler/function/handlers" works only because it matches the GOPATH in the Docker build of /go/src/handler/, it fails to work in an IDE or on a local system because the relative path is not the same.

Given the promise of Go modules and enthusiasm from the community for them, we need to find a way to make this work. What ideas to people have?

@alexellis
Copy link
Member Author

alexellis commented Jan 24, 2020

So here is a hacky suggestion, can we offer a MOD_REPLACE.txt that can be injected into the root-level main.go?

By default it'd be:

module handler

go 1.13

replace handler/function => ./function

Then users could write into GO_REPLACE.txt like follows:

replace github.com/alexellis/vend/vend/handlers => ./function/handlers

And the final Go.mod file for main.go would have that appended.

alexellis added a commit that referenced this issue Jan 24, 2020
Go sub-modules do not work with this template, because the of
limitations on how local imports and the "replace" word works
in Go modules. You can only use "replace" in the root-level module
i.e. "main.go"

https://github.com/golang/go/wiki/Modules#gomod

This patch was tested with the following repo:

[email protected]:alexellis/golang-middleware-relative-import.git

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>
alexellis added a commit that referenced this issue Jan 24, 2020
Go sub-modules do not work with this template, because the of
limitations on how local imports and the "replace" word works
in Go modules. You can only use "replace" in the root-level module
i.e. "main.go"

https://github.com/golang/go/wiki/Modules#gomod

This patch was tested with the following repo:

[email protected]:alexellis/golang-middleware-relative-import.git

* Documentation in the README has been updated
* Tested e2e and with a local "go test" in GOPATH with modules
turned on and Go 1.13
* Updated copy commands in Dockerfile so that they use a faster
mechanism, vs creating a new container step to chown (must be
tested with buildkit before merging, since buildkit did not like
this approach in the past when running as a restricted user at
runtime)

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>
alexellis added a commit that referenced this issue Jan 24, 2020
Fixes: #33

Go sub-modules do not work with this template, because the of
limitations on how local imports and the "replace" word works
in Go modules. You can only use "replace" in the root-level module
i.e. "main.go"

https://github.com/golang/go/wiki/Modules#gomod

This patch was tested with the following repo:

[email protected]:alexellis/golang-middleware-relative-import.git

* Documentation in the README has been updated
* Tested e2e and with a local "go test" in GOPATH with modules
turned on and Go 1.13
* Updated copy commands in Dockerfile so that they use a faster
mechanism, vs creating a new container step to chown (must be
tested with buildkit before merging, since buildkit did not like
this approach in the past when running as a restricted user at
runtime)

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>
@alexellis
Copy link
Member Author

I've opened #34 which seems to work and provide a workable solution. @LucasRoesler @stefanprodan did you have any other ideas to unblock this, or anything else we should try?

alexellis added a commit that referenced this issue Jan 24, 2020
Fixes: #33

Go sub-modules do not work with this template, because the of
limitations on how local imports and the "replace" word works
in Go modules. You can only use "replace" in the root-level module
i.e. "main.go"

https://github.com/golang/go/wiki/Modules#gomod

This patch was tested with the following repo:

[email protected]:alexellis/golang-middleware-relative-import.git

* Documentation in the README has been updated
* Tested e2e and with a local "go test" in GOPATH with modules
turned on and Go 1.13
* Updated copy commands in Dockerfile so that they use a faster
mechanism, vs creating a new container step to chown (must be
tested with buildkit before merging, since buildkit did not like
this approach in the past when running as a restricted user at
runtime)

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>
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 a pull request may close this issue.

2 participants