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

Support go modules #194

Conversation

LucasRoesler
Copy link
Member

Description

Add support for nested go modules by supporting the GO_REPLACE.txt file and the build args for GO111MODULE and GOFLAGS.

The version of Go is upgraded to the latest major version: 1.13

This largely replicates the module support and structure of the go-middleware-template

Motivation and Context

  • I have raised an issue to propose this change (required)

Which issue(s) this PR fixes

Fixes #189

How Has This Been Tested?

I have created a demo repo to show that the template works as expected. https://github.com/LucasRoesler/gomodule-template-test

It specifically show how the nested module support works. I have also manually tested the simpler flat module and non-module mode.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Version change (see: Impact to existing users)

Impact to existing users

Existing functions should continue to work as expected. The only changes should be controlled by the build arg

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

**What**
- Add support for nested go modules by supporting the GO_REPLACE.txt
  file and the build args for GO111MODULE and GOFLAGS

Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
RUN CGO_ENABLED=${CGO_ENABLED} GOOS=linux \
go build --ldflags "-s -w" -a -installsuffix cgo -o handler . && \
go test $(go list ./... | grep -v /vendor/) -cover
go build --ldflags "-s -w" -a -installsuffix cgo -o handler .

Choose a reason for hiding this comment

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

Should the $GOFLAGS arg be used here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This ENV variable is read by the go command automatically https://golang.org/cmd/go/#hdr-Environment_variables

WORKDIR /go/src/handler
COPY . .

# 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

Choose a reason for hiding this comment

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

I find the GO_REPLACE pattern a bit confusing, have you consider merging go.mod instead? That way the function writer still has a regular go.mod that can be linted / validated. This project seems like it could do the trick: https://github.com/brendanjryan/modmerge

Choose a reason for hiding this comment

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

Or maybe even go the multi-module approach and have the main code can be a module and the function code a different one? If a package name is enforced we might not even need the replace. https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories has an overview of that approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

This multi-module approach is what we are doing here, but ... go does not follow replaces in modules, so if you need to use a replace in your function handler module, it will not be respected in the parent template module. This GO_REPLACE is a way for us to explicitly allow overriding the replaces in the parent function module.

Copy link
Member

Choose a reason for hiding this comment

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

@alexellis
Copy link
Member

The approach is being tested and is documented. This was socialised on GitHub and slack with no concerns raised. This PR copies the approach

https://github.com/openfaas-incubator/golang-http-template/blob/master/README.md#advanced-usage---go-sub-modules-via-go_replacetxt

Concatenating a go.mod may work but we should try not to introduce mandatory dependencies that would slow down the build.

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.

Approved. Thank you @LucasRoesler 🙏 and @rberrelleza for taking a look over the work.

@alexellis
Copy link
Member

@LucasRoesler just to check, do you need to update the README and the docs page for the classic go template?

@alexellis alexellis merged commit 360077e into openfaas:master Feb 4, 2020
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.

Add optional support for Go modules via build-arg for "go" template
3 participants