Add short script to move vendor folder around #56

LucasRoesler · 2021-03-10T19:14:47Z

What

Extract the vendor and modules cleanup into a separate shell script.
This provides more space for documenting each step cleanly.
This scripting also removes the need for the GO_REPLACE.txt hack,
it will automatically modify any local replacements in the original
go.mod file, so that they work as expected.
Update the README to describe how dependencies work in the three
supported modes: modules with vendoring, modules without vendoring,
and traditional vendoring via dep.
This new flow allows dynamic downloading of Go modules or disabling
Go modules and only using vendor. You can not use vendor and modules
at the same time due to how Go will validate that the vendor folder
and the go.mod are insync

Description

How Has This Been Tested?

You can test this branch of the template using

$ faas-cli template pull https://github.com/LucasRoesler/golang-http-template#impr
oved-module-and-vendor-support
$ faas-cli new --lang golang-http

I manually tested with this handler implementation

#  publisher/handler.go
package function

import (
	"bytes"
	"fmt"
	"log"
	"net/http"
	"os"

	nats "github.com/nats-io/nats.go"
	handler "github.com/openfaas/templates-sdk/go-http"
)

var (
	subject        = "nats-test"
	defaultMessage = "Hello World"
)

// Handle a function invocation
func Handle(r handler.Request) (handler.Response, error) {
	var err error

	msg := defaultMessage
	if r.Body != nil {
		msg = string(bytes.TrimSpace(r.Body))
	}

	natsURL := nats.DefaultURL
	val, ok := os.LookupEnv("nats_url")
	if ok {
		natsURL = val
	}

	nc, err := nats.Connect(natsURL)
	if err != nil {
		errMsg := fmt.Sprintf("can not connect to nats: %s", err)
		log.Print(errMsg)

		return handler.Response{
			Body:       []byte(errMsg),
			StatusCode: http.StatusInternalServerError,
		}, err
	}
	defer nc.Close()

	s := r.Header.Get("nats-subject")
	if s != "" {
		subject = s
	}

	log.Printf("Publishing %d bytes to: %q\n", len(msg), subject)

	err = nc.Publish(subject, []byte(msg))
	if err != nil {
		log.Println(err)

		return handler.Response{
			Body:       []byte(fmt.Sprintf("can not publish to nats: %s", err)),
			StatusCode: http.StatusInternalServerError,
		}, err

	}
	return handler.Response{
		Body:       []byte(fmt.Sprintf("Published %d bytes to: %q", len(msg), subject)),
		StatusCode: http.StatusOK,
	}, err
}

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

Users should not notice any change.

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

**What** - Add Dockerfile testp to mov ethe vendor folder up from the function volder to function root. This allows the user to control the vendor folder and avoid collisions/validation errors from Go in modules mode - This new flow allows dynamic downloading of Go modules _or_ disabling Go modules and only using vendor. You can not use vendor _and_ modules at the same time due to how Go will validate that the vendor folder and the go.mod are insync Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

template/golang-http/Dockerfile

alexellis · 2021-03-10T19:17:58Z

template/golang-middleware/Dockerfile

 # Add user overrides to the root go.mod, which is the only place "replace" can be used
 RUN cat function/GO_REPLACE.txt >> ./go.mod || exit 0
+RUN if [ -d ./function/vendor ] && [ "${GO111MODULE}" == "off" ]; then \


As a note, I don't think we'll need the GO111MODULE check in 1.17, when it can't be turned off.

yeah, i removed it in the http template, they aren't in perfect sync yet

alexellis · 2021-03-10T19:18:54Z

This looks good. Do you have any concerns or areas where I can help with testing / validation?

LucasRoesler · 2021-03-10T19:28:19Z

This looks good. Do you have any concerns or areas where I can help with testing / validation?

This change is not 100% backwards compatible. For example, there is no support for vendor and modules at the same time. When modules is enabled, it will always attempt to download the modules. Which could be an issue for things like private modules. These users must use vendoring with go mod vendor but then disable modules with the build arg.

I don't know about any other configurations, e.g. existing functions using vendoring and dep, we should test that. I am not sure about any other configurations we want to support

**What** - Remove the tidy step because it is not needed and does not work as expected Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

**What** - Remove check for modules = off from the middleware template. This isn't needed Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

alexellis · 2021-03-24T16:56:01Z

golang-middleware template:

Go modules on, public modules, no vendor folder, gets downloaded every time
Go modules off, with a vendor folder (using GOPATH)
Go modules on, with a vendor folder

In Go 1.17, this will fail, because go modules are always forced on, and you cannot build from GOPATH

Go modules off, with a vendor folder

golang-http template:

Go modules on, public modules, no vendor folder, gets downloaded every time
Go modules off, with a vendor folder (using GOPATH)
Go modules on, with a vendor folder

alexellis · 2021-03-24T17:05:51Z

Couple of suggestions for things to try:

1 - lift the handler/function code up one level and overwrite the go.mod/sum of the main entrypoint
2 - Copy the files directly into the main entrypoint and rename that to "function" so that there's no "import" needed.

**What** - Add scripting that handles copying the function's go.mod to the function root. This requires several mutations to handle renaming the module and dealig with local replacements of the function - Add scriptin to handle the vendor/modules.txt. It also handles mutations to deal with the local replacement of the function code Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

LucasRoesler · 2021-03-27T11:18:43Z

@alexellis i have pushed a change that lifts the go.mod/go.sum to the main entrypoint, but only for the http template. With this, i have had successful builds with

modules ON, no vendor folder
modules ON, with vendor folder
modules OFF, with vendor folder

This also includes have sub-packages in my handler code, here is the structure of my test function with the vendor folder

├── publisher
│   ├── go.mod
│   ├── go.sum
│   ├── handler.go
│   ├── pkg
│   │   └── version
│   │       └── version.go
│   └── vendor
│       ├── github.com
│       │   ├── nats-io
│       │   │   ├── nats.go
│       │   │   │   ├── CODE-OF-CONDUCT.md
│       │   │   │   ├── context.go
│       │   │   │   ├── dependencies.md
│       │   │   │   ├── enc.go
│       │   │   │   ├── encoders
│       │   │   │   │   └── builtin
│       │   │   │   │       ├── default_enc.go
│       │   │   │   │       ├── gob_enc.go
│       │   │   │   │       └── json_enc.go
│       │   │   │   ├── go.mod
│       │   │   │   ├── go.sum
│       │   │   │   ├── GOVERNANCE.md
│       │   │   │   ├── js.go
│       │   │   │   ├── jsm.go
│       │   │   │   ├── LICENSE
│       │   │   │   ├── MAINTAINERS.md
│       │   │   │   ├── nats.go
│       │   │   │   ├── netchan.go
│       │   │   │   ├── parser.go
│       │   │   │   ├── README.md
│       │   │   │   ├── timer.go
│       │   │   │   ├── TODO.md
│       │   │   │   └── util
│       │   │   │       ├── tls.go
│       │   │   │       └── tls_go17.go
│       │   │   ├── nkeys
│       │   │   │   ├── crc16.go
│       │   │   │   ├── creds_utils.go
│       │   │   │   ├── go.mod
│       │   │   │   ├── go.sum
│       │   │   │   ├── GOVERNANCE.md
│       │   │   │   ├── keypair.go
│       │   │   │   ├── LICENSE
│       │   │   │   ├── main.go
│       │   │   │   ├── MAINTAINERS.md
│       │   │   │   ├── public.go
│       │   │   │   ├── README.md
│       │   │   │   ├── strkey.go
│       │   │   │   └── TODO.md
│       │   │   └── nuid
│       │   │       ├── GOVERNANCE.md
│       │   │       ├── LICENSE
│       │   │       ├── MAINTAINERS.md
│       │   │       ├── nuid.go
│       │   │       └── README.md
│       │   └── openfaas
│       │       └── templates-sdk
│       │           ├── go-http
│       │           │   ├── handler.go
│       │           │   └── README.md
│       │           └── LICENSE
│       ├── golang.org
│       │   └── x
│       │       └── crypto
│       │           ├── AUTHORS
│       │           ├── CONTRIBUTORS
│       │           ├── ed25519
│       │           │   ├── ed25519.go
│       │           │   ├── ed25519_go113.go
│       │           │   └── internal
│       │           │       └── edwards25519
│       │           │           ├── const.go
│       │           │           └── edwards25519.go
│       │           ├── LICENSE
│       │           └── PATENTS
│       ├── handler
│       │   └── function
│       │       └── pkg
│       │           └── version
│       │               └── version.go
│       └── modules.txt

It is important to note that this requires a small bit of schenanigens for local development, specifically, if you have sub-packages, then you need to import them relative to handler/function/...., not the "actual" function/... and then you need to add a replace of handler/function => ./

This is what the function/go.mod looks like so that local development works as expected

module function

go 1.16

require (
	github.com/nats-io/nats.go v1.10.1-0.20210228004050-ed743748acac
	github.com/openfaas/templates-sdk v0.0.0-20200723110415-a699ec277c12
	handler/function v0.0.0-00010101000000-000000000000
)

replace handler/function => ./

A side of effect of this approach is that the GO_REPLACE.txt is not needed anymore because we are just copying the go.mod to the project root, it will contain any of the original replace statements anyway.

alexellis · 2021-03-31T16:24:56Z

template/golang-http/Dockerfile

+ARG GOFLAGS=""
+
+# Add user overrides to the root go.mod, which is the only place "replace" can be used
+RUN cat function/GO_REPLACE.txt >> ./go.mod || exit 0


Lucas: is this line needed still?

alexellis · 2021-03-31T16:29:00Z

template/golang-http/Dockerfile

+        # remove any local require for the handler/function, this can exist _if_ the
+        # developer has subpackages in their handler. It is ok to just remove it
+        # because we will replace it later
+        grep -v "handler/function" go.mod > gomod2; mv gomod2 go.mod && \


Is there an issue with piping this instead? Would ; work as &&?

alexellis · 2021-03-31T16:39:54Z

Question for you - could this be a bash script that's copied in from the base template?

alexellis · 2021-03-31T16:41:16Z

From our call - what does a sub-package look like now with these changes?

**What** - Extract the vendor and modules cleanup into a separate shell script. This provides more space for documenting each step cleanly. - This scripting also removes the need for the `GO_REPLACE.txt` hack, it will automatically modify any local replacements in the original go.mod file, so that they work as expected. - Update the README to describe how dependencies work in the three supported modes: modules with vendoring, modules without vendoring, and traditional vendoring via dep. Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

LucasRoesler · 2021-04-05T09:16:08Z

@alexellis i have moved the logic to a shell script now. Note, from the commit message, this will automatically handle the GO_REPLACE so that we don't need the extra file anymore:

Extract the vendor and modules cleanup into a separate shell script.
This provides more space for documenting each step cleanly.

This scripting also removes the need for the GO_REPLACE.txt hack,
it will automatically modify any local replacements in the original
go.mod file, so that they work as expected.

Update the README to describe how dependencies work in the three
supported modes: modules with vendoring, modules without vendoring,
and traditional vendoring via dep.

I would like to also commit a sample function to the repo, what do you think? It would be really helpful to have a test/sample function that we can all reference as a baseline verification that the template compiles, at least for the two module use cases: with and without vendoring.

**What** - Add check and skip when the funciton folder is not a module Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

LucasRoesler · 2021-04-09T08:15:11Z

ping-ity ping ping

LucasRoesler · 2021-04-18T08:03:35Z

@alexellis can you re-review this and let me know what needs to be done for it to be merged? I feel like it is ready to be merged

alexellis · 2021-05-05T13:30:41Z

README.md

+go mod edit -replace=handler/function=./
+```
+
+Now if you want to reference the`version` sub-package, import it as


Nit: needs a space after "the"

alexellis

Approved. Let's see how we get on with this.

Will the script work on Alpine Linux's shell (ash) and on Ubuntu's bash shell?

XVincentX · 2021-05-05T19:35:59Z

template/golang-http/modules-cleanup.sh

+
+    # Copy the user's go.mod
+    mv -f ./function/go.mod .
+    mv -f ./function/go.sum .


It seems like this fails in case the file is not present, making the docker image construction fail

yes it currently assumes that you have a go.sum if you have a go.mod. Per the recommendations from the Go team, the file should exist and should be committed with your project https://github.com/golang/go/wiki/Modules#should-i-commit-my-gosum-file-as-well-as-my-gomod-file

Ah ok, that I did not know. Thank you very much, I have modified my functions!

LucasRoesler · 2021-05-06T06:33:37Z

Approved. Let's see how we get on with this.

Will the script work on Alpine Linux's shell (ash) and on Ubuntu's bash shell?

@alexellis the script is written and run using sh which i believe is a subset of both ash and bash, so it should be fine

alexellis reviewed Mar 10, 2021

View reviewed changes

template/golang-http/Dockerfile Outdated Show resolved Hide resolved

alexellis reviewed Mar 10, 2021

View reviewed changes

Remove go mod tidy

60d2b6d

**What** - Remove the tidy step because it is not needed and does not work as expected Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

LucasRoesler marked this pull request as ready for review March 20, 2021 09:41

Remove unneeded module check

4fb83f2

**What** - Remove check for modules = off from the middleware template. This isn't needed Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

LucasRoesler requested a review from alexellis March 20, 2021 09:44

alexellis mentioned this pull request Mar 31, 2021

Getting go: inconsistent vendoring error when a module is located within function #55

Closed

alexellis reviewed Mar 31, 2021

View reviewed changes

LucasRoesler requested a review from alexellis April 5, 2021 09:16

Handle case when function is not a module

28697c3

**What** - Add check and skip when the funciton folder is not a module Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

LucasRoesler force-pushed the improved-module-and-vendor-support branch from 129d3e5 to 28697c3 Compare April 5, 2021 09:29

alexellis reviewed May 5, 2021

View reviewed changes

alexellis approved these changes May 5, 2021

View reviewed changes

alexellis merged commit 5d6376e into openfaas:master May 5, 2021

XVincentX reviewed May 5, 2021

View reviewed changes

LucasRoesler deleted the improved-module-and-vendor-support branch May 6, 2021 06:33

DanielLee343 mentioned this pull request Jan 27, 2023

Inter-function Communication Feature Asking openfaas/faasd#316

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add short script to move vendor folder around #56

Add short script to move vendor folder around #56

LucasRoesler commented Mar 10, 2021 •

edited

alexellis Mar 10, 2021

LucasRoesler Mar 10, 2021

alexellis commented Mar 10, 2021

LucasRoesler commented Mar 10, 2021

alexellis commented Mar 24, 2021

alexellis commented Mar 24, 2021

LucasRoesler commented Mar 27, 2021

alexellis Mar 31, 2021

alexellis Mar 31, 2021

alexellis commented Mar 31, 2021

alexellis commented Mar 31, 2021

LucasRoesler commented Apr 5, 2021

LucasRoesler commented Apr 9, 2021

LucasRoesler commented Apr 18, 2021

alexellis May 5, 2021 •

edited

alexellis left a comment

XVincentX May 5, 2021

LucasRoesler May 6, 2021

XVincentX May 6, 2021

LucasRoesler commented May 6, 2021

Add short script to move vendor folder around #56

Add short script to move vendor folder around #56

Conversation

LucasRoesler commented Mar 10, 2021 • edited

Description

How Has This Been Tested?

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

Checklist:

alexellis Mar 10, 2021

Choose a reason for hiding this comment

LucasRoesler Mar 10, 2021

Choose a reason for hiding this comment

alexellis commented Mar 10, 2021

LucasRoesler commented Mar 10, 2021

alexellis commented Mar 24, 2021

alexellis commented Mar 24, 2021

LucasRoesler commented Mar 27, 2021

alexellis Mar 31, 2021

Choose a reason for hiding this comment

alexellis Mar 31, 2021

Choose a reason for hiding this comment

alexellis commented Mar 31, 2021

alexellis commented Mar 31, 2021

LucasRoesler commented Apr 5, 2021

LucasRoesler commented Apr 9, 2021

LucasRoesler commented Apr 18, 2021

alexellis May 5, 2021 • edited

Choose a reason for hiding this comment

alexellis left a comment

Choose a reason for hiding this comment

XVincentX May 5, 2021

Choose a reason for hiding this comment

LucasRoesler May 6, 2021

Choose a reason for hiding this comment

XVincentX May 6, 2021

Choose a reason for hiding this comment

LucasRoesler commented May 6, 2021

LucasRoesler commented Mar 10, 2021 •

edited

alexellis May 5, 2021 •

edited