Add opt-in support for using Go modules.

[bmcustodio]

Description

As discussed earlier today with @alexellis and other maintainers, this PR aims at allowing function authors to use Go modules in an backwards-compatible, opt-in fashion. It relies on the following observations:

• None of the current templates actually rely on dep (or any other vendoring tool for that matter). As long as there's a populated vendor/ directory inside the directory containing the function, things just work.
• GO111MODULE=on can be used to turn Go modules on when required (as well as to force Go modules to be enabled when inside GOPATH).
• faas build allows users to set build-time variables, allowing GO111MODULE to be set as desired.
• Additionally, and as a side note, go mod vendor can be used to create a populated vendor/ directory before running faas build if that is ever needed.

This allows for a "new" workflow to be established:

• If the user wants to use dep or some other vendoring tool, no action is required as these changes are backwards-compatible. The user may keep on doing whatever they currently do.
• If the user wants to use Go modules, they need to...

$ faas new my-fn --lang golang-http
$ cd my-fn
$ go mod init handler/function. # Important!
$ cd ..
$ faas build --build-arg GO111MODULE=on [--build-arg GOPROXY=https://example.com]

This will cause Go modules to kick-in in build-time to fetch the required dependencies. In case this is not desired (such as when some dependencies come from private repositories, for example), the user can use go mod vendor to pre-populate the vendor/ directory and simply run faas build as usual (hence effectively disabling Go modules in build-time).

I am keeping propagation of this change to https://github.com/openfaas/templates and updates to the documentation on hold until we can gather more feedback on this, as I believe that's the best for now.

How Has This Been Tested?

I've tested by executing the workflow described above against two branches in the https://github.com/bmcstdio/golang-http-template-modules-example repository:

• A branch containing a function that uses Go modules.
• A branch containing a function that uses dep.

Each branch contains a Makefile defining a build target that can be used to illustrate and reproduce the behaviour described above.

How are existing users impacted? What migration steps/scripts do we need?

As mentioned above, users wanting to retain the current behaviour do not need to perform any action.

Checklist:

I have:

• updated the documentation and/or roadmap (if required)
• read the CONTRIBUTION guide
• signed-off my commits with git commit -s
• added unit tests

[matipan]

This looks really good. I have two quick questions:

• Why are we splitting this into two commands?
• Why are we not ignoring the tests in the vendor directory? Are they ignored by default?

[bmcustodio]

Hi @matipan!

Why are we splitting this into two commands?

That's just for the sake of readability. It doesn't impact the final image size AFAIU (plus this corresponds to the builder image, which is not final).

Why are we not ignoring the tests in the vendor directory? Are they ignored by default?

On the one hand, and according to my understanding, go mod vendor does not include test files:

$ go help mod vendor
(...)
It does not include test code for vendored packages.
(...)

On the other hand, and also according to my understanding, dep (and friends?) prunes test files by default:

[prune]
(...)
  test-go = true

There may, of course, be flaws in this thinking, so please let me know what your thoughts are.

[LucasRoesler]

Overall, i think the changes look good, but agree with @matipan 's comment about the build/test command

[fopina]

👍 on this one, started recently with golang and managed to get away from godep and $GOPATH prison into go modules, it'd be great to use that with openfaas.

anything I can do to help pushing this?

[alexellis]

Hi @fopina, we have carried out a review and are waiting on @bmcstdio's response.

In the interim, would you like to do some testing on this version of the template?

Alex

[fopina]

yes, I am using it (since I posted last time so not for long I’d say :)).
I can answer on the second question though: there is no vendor directory as modules go into specific directory and are not packaged in the repo (another good thing, git-space friendly), so nothing to exclude :)

[alexellis]

We have a specific requirement to be backwards compatible with the vendor folder. That's something Bruno suggested.

[fopina]

I think that makes perfect sense and I haven’t tested with a godep app so I can’t confirm at the moment.
If it’s not compatible as it is, would it become too polluted if separate templates were added?

[bmcustodio]

I think that makes perfect sense and I haven’t tested with a godep app so I can’t confirm at the moment.

@fopina cc @alexellis as I've mentioned above, and according to my understanding, dep will prune test files by default.

[alexellis]

@bmcstdio hi, this will need a rebase, I can see 3 files conflicting.

Thanks,

Alex

[alexellis]

as I've mentioned above, and according to my understanding, dep will prune test files by default.

Could you do a quick test please? 👍

[alexellis]

Hi, it looks like it's been 40 days since my last two comments:

• A rebase is needed - it cannot be merged.

• You said "according to my understanding, it works like x" and I asked if you could do a test (manual is fine) to show that your understanding is correct.

Once that's covered, I think this can be merged.

Alex

[alexellis]

Sorry, this can't be merged without a rebase.

[jonatasbaldin]

Just tested everything and it works like a charm. I'd really love to see this merged ❤️

Could u take a look into the rebase needed @bmcstdio?

Thanks for the work!

[bmcustodio]

@jonatasbaldin rebased (cc @alexellis), and bumped to Go 1.13.1 in the meantime. Would you be able to test again, please? I don't have a lot of time at hand at the moment, unfortunately. :(

[jonatasbaldin]

My tests using the example form @bmcstdio:

dep

• go test doesn't run on vendored packages with dep
• faas build and faas up works normally, being backwards compatible

go mod

• the make build command fails with:

Step 15/27 : RUN go test ./function/... -cover
 ---> Running in 3943fc3f660e
go: directory function is outside main module
The command '/bin/sh -c go test ./function/... -cover' returned a non-zero code: 1
2019/10/16 14:35:26 ERROR - Could not execute command: [docker build --build-arg GO111MODULE=on -t localhost:5000/golang-http-template-modules-example:latest .]
make: *** [build] Error 1

I tried changing the test command to the old one, but them the tests don't run.

I also tried changing the build image to the older one, which got me another error:

Step 14/27 : RUN go build --ldflags "-s -w" -a -installsuffix cgo -o handler .
 ---> Running in bff185df2278
go: finding github.com/openfaas-incubator/go-function-sdk v0.0.0-20181217173454-cac4b8744921
go: downloading github.com/openfaas-incubator/go-function-sdk v0.0.0-20181217173454-cac4b8744921
main.go:12:2: unknown import path "handler/function": cannot find module providing package handler/function
The command '/bin/sh -c go build --ldflags "-s -w" -a -installsuffix cgo -o handler .' returned a non-zero code: 1
2019/10/16 14:36:52 ERROR - Could not execute command: [docker build --build-arg GO111MODULE=on -t jonatasbaldin/golang-http-template-modules-example:latest .]

Could u take a look @bmcstdio?

[alexellis]

Thank you for taking a look at this and doing the additional testing.

[bmcustodio]

@jonatasbaldin @alexellis I believe the issue is fixed now, could you please re-test on your side?

[alexellis]

Since @jonatasbaldin was testing, I'll await feedback from him @bmcstdio .

[jonatasbaldin]

Hey there, it worked!

Just making sure (as already stated in the PR):

• the dep is backwards compatible, the faas build works normally
• the go mod requires, the faas build requires --build-arg GO111MODULE=on

So, from the examples, everything is working fine ✨

I also tested with this little app I made called Brunchies locally and also worked fine.

I think we are good!

Thanks a lot @bmcstdio 🎉

[jonatasbaldin]

hey @alexellis, I've tested this and it was working ok, if you could take a look into it :)

[fopina]

@jonatasbaldin as you seem to be "needing" this merge, you are aware you can easily use the templates from @bmcstdio and skip core ones completely right?
Like this

[jonatasbaldin]

Thanks @fopina for the tip, but the case is that I want to use it in the public OFC, which doesn't accept custom templates :(

[fopina]

Ah ok, haven’t tried OFC yet, but sounds like a decent feature request hehe

[alexellis]

Approved

[alexellis]

@bmcstdio I just noticed the Go version was changed to 1.13 and the Go mod says "go 1.12", do you want to send a follow-up PR to make them the same?

I'll merge this now given that @jonatasbaldin gave the all-clear.

Thanks so much for working on this and for collaborating with the community on it.