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 modules support #414

Merged
merged 3 commits into from
May 13, 2019

Conversation

DirectXMan12
Copy link
Contributor

@DirectXMan12 DirectXMan12 commented May 4, 2019

This should be fine, as long as minimal version selection holds and you
don't call go get -u on other direct dependencies.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 4, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 4, 2019
@DirectXMan12 DirectXMan12 changed the title [WIP] Go modules support [WIP] :feature: Go modules support May 4, 2019
@DirectXMan12 DirectXMan12 changed the title [WIP] :feature: Go modules support [WIP] ✨ Go modules support May 4, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 4, 2019
@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 4, 2019
@DirectXMan12 DirectXMan12 force-pushed the feature/modules branch 2 times, most recently from bcc0136 to 18e722c Compare May 6, 2019 22:35
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
gopkg.in/yaml.v2 v2.2.1 // indirect
k8s.io/api v0.0.0-20190222213804-5cb15d344471
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to use replace to pin specific versions of k8s deps? k/k uses replace to pin all deps, btw.

Copy link
Member

Choose a reason for hiding this comment

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

IIUC k/k uses replace to make the staging directory work.
Unlike k/k, controller-runtime always uses a published version of k8s deps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using it for staging is part of the reason I think. They use it to pin specific versions for all dependencies. Anyway, just wanted to make sure if we want to force specific k8s versions for a given c-r release or just let minimum version selection do its thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have been playing with Go mod. It seemed to me that until k8s.io supports go mod, you have to pin using replace to pick up the correct version. So, to set dependency on k8s.io 1.14.x we are using replace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replace only works on the highest-level repository, so when people consume this repo, replace won't help them.

Also, these should be kubernetes-1.14.x -- if you say "k8s.io/api kubernetes-1.14.x", go mod tidy will convert it to a version-ish thing like this, but it still should be equivalent afaict (check the date on the commit -- it should match the corresponding version from Gopkg.toml)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, go mod tidy will convert to a "pseudo version". Also, your point about downstream consumers of this library makes sense. Thanks for clarifying.

@@ -25,6 +25,14 @@ if [ -n "$TRACE" ]; then
set -x
fi

(go mod edit -json &>/dev/null)
Copy link
Member

Choose a reason for hiding this comment

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

Quoting the help text: "The -json flag prints the final go.mod file in JSON format instead of writing it back to go.mod."
What is the purpose of this? Just printing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's just a handy way to check if modules is enabled for the repo, given the next line grabs exit code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, exactly! I'll add a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(note that we're using the command's return code immediately below :-) )

@mengqiy
Copy link
Member

mengqiy commented May 8, 2019

you don't call go get -u on other direct dependencies.

why this is a problem when using go modules?

@rajathagasthya
Copy link
Contributor

@mengqiy I think that's because Go mod authors recommend not to use go get -u when upgrading direct dependencies.

From https://github.com/golang/go/wiki/Modules#how-to-upgrade-and-downgrade-dependencies:

A common mistake is thinking go get -u foo solely gets the latest version of foo. In actuality, the -u in go get -u foo or go get -u foo@latest means to also get the latest versions for all of the direct and indirect dependencies of foo. A common starting point when upgrading foo is instead to do go get foo or go get foo@latest without a -u (and after things are working, consider go get -u=patch foo, go get -u=patch, go get -u foo, or go get -u).

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2019
@DirectXMan12 DirectXMan12 changed the title [WIP] ✨ Go modules support ✨ Go modules support May 10, 2019
@k8s-ci-robot k8s-ci-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 10, 2019
@DirectXMan12 DirectXMan12 force-pushed the feature/modules branch 4 times, most recently from eef34c5 to 85f88c2 Compare May 10, 2019 20:16
This should be fine, as long as minimal version selection holds and you
don't call `go get -u` on other direct dependencies.
It's a maintained, non-archived version of gometalinter that's also
faster.
Now based off the go.mod file.
@mengqiy
Copy link
Member

mengqiy commented May 13, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2019
@k8s-ci-robot k8s-ci-robot merged commit dfc2508 into kubernetes-sigs:master May 13, 2019
@DirectXMan12 DirectXMan12 deleted the feature/modules branch May 13, 2019 20:52
DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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.

5 participants