Skip to content
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

feat: automatically configure vendor mode #74

Conversation

LucasRoesler
Copy link
Member

@LucasRoesler LucasRoesler commented Apr 16, 2022

When a vendor folder is seen, we need to disable the go module mode,
instead we need to use GOPATH mode. This is a simple toggle of one
env variable, using a script means that users do not need to set any
build args.

How Has This Been Tested?

I have updated my example repo to include a function using vendor and with no build args, see the jules function here https://github.com/LucasRoesler/golang-http-template-examples

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

This will remove any manual steps to enable vendor mode in the templates, it should improve the DX

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

@LucasRoesler LucasRoesler force-pushed the feat-automatic-vendor-detection branch from 9a81df8 to 894296a Compare April 16, 2022 08:49
When a vendor folder is seen, we need to disable the go module mode,
instead we need to use GOPATH mode. This is a simple toggle of one
env variable, using a script means that users do not need to set any
build args.

Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
@LucasRoesler LucasRoesler force-pushed the feat-automatic-vendor-detection branch from 894296a to 6ae0208 Compare April 16, 2022 08:54
@LucasRoesler LucasRoesler marked this pull request as ready for review April 16, 2022 08:54
- To use Go modules without vendoring, the default already is set `GO111MODULE=on` but you also can make that explicit by adding `--build-arg GO111MODULE=on` to `faas-cli up`, you can also use `--build-arg GOPROXY=https://` if you want to use your own mirror for the modules
- You can also Go modules with vendoring, run `go mod vendor` in your function folder and add `--build-arg GO111MODULE=off --build-arg GOFLAGS='-mod=vendor'` to `faas-cli up`
- To use Go modules without vendoring, the default already is set `GO111MODULE=on` but you also can make that explicit by adding `--build-arg GO111MODULE=on` to `faas-cli up`, you can also use `--build-arg GOPROXY=https://` if you want to use your own mirror for the modules.
- You can also Go modules with vendoring, run `go mod vendor`, the build process will automatically detect the vendor folder and configure the Go environment correctly.
Copy link
Member

Choose a reason for hiding this comment

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

I think this should still mention that GO111MODULE will be forced to off.

auto-magic can end up being more confusing than setting explicit values.

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Can you run me through this on the next community call please @LucasRoesler ?

# automatically set the required module related flags
# when a vendor folder is detected.
#
# Currently, in Go 1.18, Go Workspaces are incompatible
Copy link
Contributor

@mrwormhole mrwormhole Jun 9, 2022

Choose a reason for hiding this comment

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

will be a dumb question but if this was 1.17 would we need this tweak?

I am kinda thinking how all sh scripts can be avoided but I thought vendor files were compatible with workspaces, it is intriguing. I think http templates are super simple compared to past sh scripts before 1.18 and this amount seems OK, I will chase this incompatibility issue. Looks good to me in this way tho, it is more clear than old shell scripts

I will be kinda confused again but I thought -mod=vendor was the default flag when Go detects vendor folder since 1.13, when did this actually change so we had to specify it explicitly? I am very bad vendor user(not using at all since 1.13) If you have a look at the footnotes, it says -mod=vendor is the default here https://www.ardanlabs.com/blog/2020/04/modules-06-vendoring.html

Copy link
Contributor

Choose a reason for hiding this comment

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

actually -mod readonly also reads the vendor as far as I understand from this thread golang/go#30404, so I think shell script may no longer needed? the end user needs to do -mod=readonly as you pointed out here Lucas https://github.com/LucasRoesler/openfaas-golang-template-workspace

Copy link
Member Author

Choose a reason for hiding this comment

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

@mrwormhole i don't 100% follow. Were you able to get a workspaces project to build with the vendor by using -mod=vendor -mod=readonly?

The main reason for the script, is that when using workspaces, go build ignores the vendor folder and attempts to download the modules again. When a vendor folder is detected, disabling modules forces Go to use the GOPATH, which then, correctly, detects and uses the vendor folder as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

-mod=readonly should use vendor if vendor exists, I also don't know why this config needs to specify -mod=vendor to get vendor files, it is automatic when not specified, default is -mod=readonly, -mod=mod is the one who only looks up for go modules and ignores vendor completely

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand what you are saying, but

  1. this code does not use or require -mod=vendor
  2. you are 100% right, we normally expect go build to automatically detect the vendor folder, however, the current behavior of workspaces seems to ignore this. I tried it (I had exactly the same hope that you did) but it was not building in the correct way. Even with a vendor folder present it was still downloading the modules.

Perhaps you can share a working example repo with the changes you would like to see?

When I tried to use vendor with the go.work, the only time I could get it to detect the vendor folder was by disabling modules. This is what the script does. It automates detecting this situation instead of requiring the developer to pass an additional build arg.

Copy link
Contributor

@mrwormhole mrwormhole Jun 19, 2022

Choose a reason for hiding this comment

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

so I actually created the same repository but without -mod vendor flag, I vendored my dependencies (which is the user responsibility) and deleted -mod=vendor and made go modules ON, it actually detected and built with vendor folder but the key here was I deleted the repository of my module, I do think -mod vendor is completely unneeded and shouldn't be used.

https://github.com/MrWormHole/openfaas-golang-template-workspace

the end-user should be able to know as a Go dev, what could be the use case for enabling go modules and doing -mod vendor. Vendor folder should exist only for disaster recovery, else it will download all deps with go.mod and not use vendor folder. Also go mod vendor should be called explicitly by developers and stored in the repository, else it is a problem

Copy link
Member

Choose a reason for hiding this comment

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

If we can move forward without using additional bash and if statements, I'd be interested to see that.

Copy link
Contributor

@mrwormhole mrwormhole Jun 29, 2022

Choose a reason for hiding this comment

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

I think Lucas is right @LucasRoesler depending on your Go version it gets set to default to -mod=readonly, I would keep it and resolve these comments. It will enhance the dev usage of these templates even though it sets it internally.

https://go.dev/ref/mod
image

I am on version 1.17.11 and -mod=mod always runs when I don't specify -mod=vendor with go modules "OFF" or "ON", sorry for the confusion I had earlier. One theory from my side is that alpine images(Dockerfile this uses) of 1.18 sets it to -mod=mod, I didn't confirm yet today but if you fork it, you will see this one builds https://github.com/MrWormHole/openfaas-golang-template-workspace without on or off specification because -mod flag figures it out

Copy link
Member

Choose a reason for hiding this comment

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

@mrwormhole can you clarify your recommendations in a few bullets or steps, or are you retracting what you said?

@mrwormhole
Copy link
Contributor

out of topic, I wish long-lived patient life for those who use vscode everyday for workspaces, it really delayed my response to this PR, as well as Goland IDE (they both don't work with the workspaces in terms of IDE completion)
image

@alexellis
Copy link
Member

out of topic, I wish long-lived patient life for those who use vscode everyday for workspaces, it really delayed my response to this PR, as well as Goland IDE (they both don't work with the workspaces in terms of IDE completion)

Go workspaces are working for me in VSCode, and I believe for @LucasRoesler too for highlighting and intellisense.

@alexellis
Copy link
Member

@LucasRoesler @mrwormhole

I think I have found the issue with #73, we seem to have assumed that @kevin-lindsay-1 is using the golang-middleware template and have examples that work with it. He's actually must be using the golang-http template.

Now, we did ask for this information and a repro several times in the issue, then yesterday what he said revealed it was actuallt the golang-http template, and not golang-middleware.

When I run Lucas' example, I am getting a cannot find package when I use this PR / template with golang-http and a vendor folder.

This is probably what Kevin is referring to when he says that an issue hasn't been fixed for 4 months.

His code sample that he sent us via Discord, will not work for the community because it always assumes a vendor folder, and we know that not everyone uses vendoring, furthermore, this PR automatically turns on vendoring whenever a vendor folder is present.

Excerpt:

WORKDIR "/usr/root"
# Copy module information
COPY [ "./function/go.sum", "./function/go.mod", "./" ]
COPY [ "./function/vendor/", "./vendor/" ]
# Precompile vendor binaries for caching purposes
RUN go build -v "./vendor/..."

Here's how I reproduced the error.

git clone https://github.com/LucasRoesler/golang-http-template-examples
cd golang-http-template-examples
faas-cli template pull https://github.com/LucasRoesler/golang-http-template#feat-automatic-vendor-detection

cd publisher
go mod vendor
cd ../

faas-cli build -f stack.yml --filter publisher

Then:

[0] > Building publisher.
Clearing temporary build folder: ./build/publisher/
Preparing: ./publisher/ build/publisher/function
Building: publisher:latest with golang-http template. Please wait..
Sending build context to Docker daemon  538.6kB
Step 1/42 : FROM --platform=${TARGETPLATFORM:-linux/amd64} ghcr.io/openfaas/of-watchdog:0.9.3 as watchdog
 ---> c8a51e4b8423
Step 2/42 : FROM --platform=${BUILDPLATFORM:-linux/amd64} golang:1.18-alpine3.15 as build
 ---> 99760761bfe0
Step 3/42 : ARG TARGETPLATFORM
 ---> Using cache
 ---> 5a5860cdcee9
Step 4/42 : ARG BUILDPLATFORM
 ---> Using cache
 ---> d4a04da5b0f2
Step 5/42 : ARG TARGETOS
 ---> Using cache
 ---> d91937f388cb
Step 6/42 : ARG TARGETARCH
 ---> Using cache
 ---> d3ad506ad823
Step 7/42 : RUN apk --no-cache add git
 ---> Using cache
 ---> ecbb40a90bfb
Step 8/42 : COPY --from=watchdog /fwatchdog /usr/bin/fwatchdog
 ---> Using cache
 ---> 7cb0e62fc206
Step 9/42 : RUN chmod +x /usr/bin/fwatchdog
 ---> Using cache
 ---> 964c43dd91fa
Step 10/42 : RUN mkdir -p /go/src/handler
 ---> Using cache
 ---> c98f4b23ac45
Step 11/42 : WORKDIR /go/src/handler
 ---> Using cache
 ---> 660395c49032
Step 12/42 : COPY . .
 ---> bbd5cfffc63c
Step 13/42 : ARG GO111MODULE="on"
 ---> Running in 920edb165b05
Removing intermediate container 920edb165b05
 ---> 31318c83cbc9
Step 14/42 : ARG GOPROXY=""
 ---> Running in 0e6a6fff0523
Removing intermediate container 0e6a6fff0523
 ---> cf4b245a873e
Step 15/42 : ARG GOFLAGS=""
 ---> Running in 6f1f0250f22c
Removing intermediate container 6f1f0250f22c
 ---> 8627a5a097a0
Step 16/42 : ARG CGO_ENABLED=0
 ---> Running in d1c2bc6bc3bd
Removing intermediate container d1c2bc6bc3bd
 ---> 3512cad1c97c
Step 17/42 : ARG DEBUG=0
 ---> Running in 1ae19016b421
Removing intermediate container 1ae19016b421
 ---> 8bc73cd713d9
Step 18/42 : ENV DEBUG=${DEBUG}
 ---> Running in 6eba586bdcc8
Removing intermediate container 6eba586bdcc8
 ---> 42fe29d9850d
Step 19/42 : ENV CGO_ENABLED=${CGO_ENABLED}
 ---> Running in abc7da9df3f8
Removing intermediate container abc7da9df3f8
 ---> c22a3eac3ff1
Step 20/42 : ENV GOOS=${TARGETOS}
 ---> Running in 74067bd9c516
Removing intermediate container 74067bd9c516
 ---> acb4ee60c870
Step 21/42 : ENV GOARCH=${TARGETARCH}
 ---> Running in 6cb85ec43621
Removing intermediate container 6cb85ec43621
 ---> b5fed88e4793
Step 22/42 : ENV GO=/go/src/handler/go.sh
 ---> Running in bc3211c95e08
Removing intermediate container bc3211c95e08
 ---> 7cbe8e9e9a2b
Step 23/42 : RUN chmod +x ${GO}
 ---> Running in 5bd5d68127a5
Removing intermediate container 5bd5d68127a5
 ---> a2f82673576f
Step 24/42 : RUN test -z "$(gofmt -l $(find . -type f -name '*.go' -not -path "./vendor/*" -not -path "./function/vendor/*"))" || { echo "Run \"gofmt -s -w\" on your Golang code"; exit 1; }
 ---> Running in 0e0c8e68e2c6
Removing intermediate container 0e0c8e68e2c6
 ---> d1cd7ef5de9f
Step 25/42 : WORKDIR /go/src/handler/function
 ---> Running in 6f29995b47c0
Removing intermediate container 6f29995b47c0
 ---> d7846c145261
Step 26/42 : RUN mkdir -p /go/src/handler/function/static
 ---> Running in 78f1f3b7ca58
Removing intermediate container 78f1f3b7ca58
 ---> 6c1fb795b7af
Step 27/42 : RUN ${GO} test ./... -cover
 ---> Running in 7aa5fdf2e3b6
Setting vendor mode env variables
?       handler/function        [no test files]
?       handler/function/pkg/version    [no test files]
Removing intermediate container 7aa5fdf2e3b6
 ---> d5acf8829571
Step 28/42 : WORKDIR /go/src/handler
 ---> Running in aae9159de83f
Removing intermediate container aae9159de83f
 ---> d54c79c00b04
Step 29/42 : RUN ${GO} build --ldflags "-s -w" -a -installsuffix cgo -o handler .
 ---> Running in 91c2734441a8
Setting vendor mode env variables
main.go:18:2: cannot find package "github.com/openfaas/templates-sdk/go-http" in any of:
        /usr/local/go/src/github.com/openfaas/templates-sdk/go-http (from $GOROOT)
        /go/src/github.com/openfaas/templates-sdk/go-http (from $GOPATH)
The command '/bin/sh -c ${GO} build --ldflags "-s -w" -a -installsuffix cgo -o handler .' returned a non-zero code: 1
[0] < Building publisher done in 7.68s.
[0] Worker done.

Total build time: 7.68s
Errors received during build:
- [publisher] received non-zero exit code from build, error: The command '/bin/sh -c ${GO} build --ldflags "-s -w" -a -installsuffix cgo -o handler .' returned a non-zero code: 1

@mrwormhole
Copy link
Contributor

mrwormhole commented Aug 7, 2022

@LucasRoesler @alexellis I feel like we can remove the shell scripts, there is really nothing that will go wrong if GOFLAGs are not specified by someone else if the Go version of that person is +v1.14.

I did check both templates RUN echo $GOFLAGS outputs empty strings since they are all +v1.14

Here is what I got from official docs (https://go.dev/ref/mod#build-commands)

  • The -mod flag controls whether go.mod may be automatically updated and whether the vendor directory is used.
    • -mod=mod tells the go command to ignore the vendor directory and to automatically update go.mod, for example, when an imported package is not provided by any known module.
    • -mod=readonly tells the go command to ignore the vendor directory and to report an error if go.mod needs to be updated.
    • -mod=vendor tells the go command to use the vendor directory. In this mode, the go command will not use the network or the module cache.
    • By default, if the go version in go.mod is 1.14 or higher and a vendor directory is present, the go command acts as if -mod=vendor were used. Otherwise, the go command acts as if -mod=readonly were used.

Technically I do go mod vendor as a separate process. I also avoid using -mod flag because go figures its own. Some common cases that go wrong when GOFLAGS is specified

(this fails if you have a vendor folder or you don't have a vendor folder, best to not specify(GOFLAGS) here and let it handle its own)
GO111MODULE: on
GOFLAGS: "-mod=vendor"

(this also fails if there is no vendor folder, will not fail if you have a vendor folder, will change to -mod=vendor)
GO111MODULE: off
GOFLAGS: "-mod=mod"

Also as a second note, I have seen this when I opened up my IDE, I think IDEs do set -mod flag sometimes, when I created go.work file, it disappeared, all the go installations don't have global GOfLAGS for -mod flag. Check with go env | grep "GOFLAGS" should output empty string

image

Last note: @kevin-lindsay-1 I don't set GOFLAGS as well and enable vendoring by doing go mod vendor(internally Go handles it, not GOFLAGS), it works fine for the private repos with golang-middlewares template

@alexellis
Copy link
Member

@mrwormhole try it with the golang-http template. The middleware template has no issue at present.

@LucasRoesler
Copy link
Member Author

No longer needed because of #83

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 this pull request may close these issues.

3 participants