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

go mod support for new operators #1001

Merged
merged 76 commits into from
May 9, 2019
Merged

Conversation

estroz
Copy link
Member

@estroz estroz commented Jan 25, 2019

Description of the change: add go mod support by default for new operator projects with the option of using dep by setting --dep-manager=dep. print-deps will print dependencies of whichever dependency manager type is being used.

Motivation for the change: see #992.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 25, 2019
pkg/scaffold/project/tools.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2019
@lilic
Copy link
Member

lilic commented Feb 1, 2019

General comments about this. From my experience with go modules you cannot have the project in a $GOPATH, so with this change, we would need to move away from that. We rely heavily on the $GOPATH in all the scripts and docs, but also here

func CheckAndGetProjectGoPkg() string {
gopath := MustSetGopath(MustGetGopath())

So I would think about how to first migrage out of needing to be in $GOPATH and also focus on go module support.

@nicknezis
Copy link

@lilic Perhaps CheckAndGetProjectGoPkg() can be replaced (or somehow amended) with a function that pulls the package name from go.mod (perhaps alternative logic if go.mod exists?)

@davidovich
Copy link

This might be interesting: from the Go modules Wiki:

The full fix is to move programs that load packages off of go/build and onto golang.org/x/tools/go/packages, which understands how to locate packages in a module-aware manner. This will likely eventually become go/packages.

https://godoc.org/golang.org/x/tools/go/packages

@ghost
Copy link

ghost commented Feb 18, 2019

Hi @estroz ,

Just a notice, according to the Go WiKi - Modules

go mod vendor resets the main module's vendor directory to include all packages needed to build and test all of the module's packages based on the state of the go.mod files and Go source code.

go mod vendor doesn't vendor unused packages or files, and I think you may have to integrate the code generation package for operator-sdk generate as well.

Thanks for your great work to make this happening!

@estroz
Copy link
Member Author

estroz commented Feb 18, 2019

This PR is on the backburner for now, but I'll be getting back on it soon.

@lilic I agree we should eliminate GOPATH requirements first then tackle modules.
@davidovich interesting, do you know how far off that feature is?
@jeffreystoke we'll use the tools.go file to get generators working. Seem my comment.

@davidovich
Copy link

@davidovich interesting, do you know how far off that feature is?

I'm not sure I understand the question, do you mean supported by the go team? (Yes) When it is available? (Now) Or it's API stability? (It is stated as stable as of December 1st 2018).

@davidovich
Copy link

@lilic

From my experience with go modules you cannot have the project in a $GOPATH,

They can, if the GO111MODULE=on environment variable is set. This env var can take three values auto, on and off. auto activates go modules when NOT in GOPATH, and deactivates them when on GOPATH. on obviously forces go modules independently of where the code is located. See the relevant entry in the go Modules wiki.

@davidovich
Copy link

I have come accross additional info: https://godoc.org/github.com/rogpeppe/go-internal/modfile can be used to parse a modfile (which this PR implements additionnally). According to golang/go#30147 (which suggests to use the rogpeppe modfile fork) and golang/go#28101 the modfile should eventually graduate outside of internal.

@lilic
Copy link
Member

lilic commented Mar 18, 2019

@estroz FYI there is an upstream PR to add go.mod to kubernetes, that might be of some help here -> kubernetes/kubernetes#74877

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 18, 2019
@estroz
Copy link
Member Author

estroz commented Mar 18, 2019

Note: prow CI jobs will fail until we update the CI base image to go 1.11+

@AlexNPavel
Copy link
Contributor

/retest

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 18, 2019
@estroz estroz changed the title [WIP] go mod support go mod support for new operators Mar 25, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2019
doc/user-guide.md Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2019
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM after addressing the above comment

Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

lgtm

@estroz estroz merged commit adb253d into operator-framework:master May 9, 2019
@estroz estroz deleted the go-mod branch May 9, 2019 16:42
@lilic lilic mentioned this pull request May 10, 2019
NicolasT added a commit to scality/metalk8s that referenced this pull request Jul 8, 2019
Using an unreleased version of `operator-sdk` which permits to build/run
the operator outside of `GOPATH` using Go modules properly.

See: operator-framework/operator-sdk#1001
NicolasT added a commit to scality/metalk8s that referenced this pull request Jul 8, 2019
Using an unreleased version of `operator-sdk` which permits to build/run
the operator outside of `GOPATH` using Go modules properly.

Note: the CRD `scope` is manually changed to `Cluster` after
code/YAML-generation.

See: operator-framework/operator-sdk#1001
NicolasT added a commit to scality/metalk8s that referenced this pull request Jul 8, 2019
Using an unreleased version of `operator-sdk` which permits to build/run
the operator outside of `GOPATH` using Go modules properly.

Note: the CRD `scope` is manually changed to `Cluster` after
code/YAML-generation.

See: operator-framework/operator-sdk#1001
NicolasT added a commit to scality/metalk8s that referenced this pull request Jul 9, 2019
Using an unreleased version of `operator-sdk` which permits to build/run
the operator outside of `GOPATH` using Go modules properly.

Note: the CRD `scope` is manually changed to `Cluster` after
code/YAML-generation.

See: operator-framework/operator-sdk#1001
slaperche-scality pushed a commit to scality/metalk8s that referenced this pull request Jul 12, 2019
Using an unreleased version of `operator-sdk` which permits to build/run
the operator outside of `GOPATH` using Go modules properly.

Note: the CRD `scope` is manually changed to `Cluster` after
code/YAML-generation.

See: operator-framework/operator-sdk#1001
slaperche-scality pushed a commit to scality/metalk8s that referenced this pull request Jul 12, 2019
Using an unreleased version of `operator-sdk` which permits to build/run
the operator outside of `GOPATH` using Go modules properly.

Note: the CRD `scope` is manually changed to `Cluster` after
code/YAML-generation.

See: operator-framework/operator-sdk#1001
slaperche-scality pushed a commit to scality/metalk8s that referenced this pull request Jul 12, 2019
Using an unreleased version of `operator-sdk` which permits to build/run
the operator outside of `GOPATH` using Go modules properly.

Note: the CRD `scope` is manually changed to `Cluster` after
code/YAML-generation.

See: operator-framework/operator-sdk#1001
slaperche-scality pushed a commit to scality/metalk8s that referenced this pull request Jul 15, 2019
Using an unreleased version of `operator-sdk` which permits to build/run
the operator outside of `GOPATH` using Go modules properly.

Note: the CRD `scope` is manually changed to `Cluster` after
code/YAML-generation.

See: operator-framework/operator-sdk#1001
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet