Skip to content

initial go modules draft #908

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

Merged
merged 7 commits into from
Mar 22, 2019
Merged

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Mar 20, 2019

Initial go modules draft KEP.

/sig architecture testing release api-machinery

/cc @lavalamp @sttts

@k8s-ci-robot k8s-ci-robot requested review from lavalamp and sttts March 20, 2019 17:28
@k8s-ci-robot k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 20, 2019
Copy link
Member

@cblecker cblecker left a comment

Choose a reason for hiding this comment

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

initial kep lgtm. thumbs up to merge and iterate from me.

Copy link
Contributor

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

@liggitt Nicely written and the timing works well. Thanks for putting this together.

@mattfarina
Copy link
Contributor

/hold
/lgtm

Adding hold until an approver on the KEP pokes at it.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 20, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2019
@liggitt liggitt requested a review from thockin March 21, 2019 04:22
@liggitt liggitt requested a review from smarterclayton March 21, 2019 04:22
@dims
Copy link
Member

dims commented Mar 21, 2019

LGTM, +1 to merge and iterate!

@idealhack
Copy link

/cc

@k8s-ci-robot k8s-ci-robot requested a review from idealhack March 21, 2019 12:29
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

this is cool. lgtm

@mattfarina
Copy link
Contributor

@smarterclayton @thockin can either of you approve this so we can merge?

* uses latest commits of transitive non-module-based dependencies (likely to break, as today)
* module-based consumers
* reference published modules versions as a consistent `vX.y.z` version (e.g. `v15.0.0`)
* import `k8s.io/apimachinery/v15/...` (have to rewrite kubernetes component imports on every major version bump)
Copy link
Contributor

Choose a reason for hiding this comment

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

Conversely, if we actually did the right thing (as defined by go) and kept the old versions functioning correctly (e.g. v14.x.y itself imports v15 and adapts), then this would only be necessary when people actually wanted the new interface.

...I'm not claiming the demand go makes is reasonable, but if old packages stop working or stop working with new packages, we are not doing it the way they intended.

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'm not necessarily opposed to adopting semantic import versioning, but I see that as a discrete step, and one that should wait at least until we don't have to worry about GOPATH consumers as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

- "@mattfarina"
approvers:
- "@smarterclayton"
- "@thockin"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also willing to be on either of these lines

@smarterclayton
Copy link
Contributor

/approve

For initial status. Agreed with Jordan’s assessment on semantic versioning being a discrete step and orthogonal to short term inprovements to tooling.

@liggitt
Copy link
Member Author

liggitt commented Mar 22, 2019

unholding, will fold in updates for some comments in the next PR

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2019
@dims
Copy link
Member

dims commented Mar 22, 2019

/lgtm

(@mattfarina and @cblecker indicated their LGTM earlier plus mine as well)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt, mattfarina, smarterclayton

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 merged commit 3dd7391 into kubernetes:master Mar 22, 2019
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

As part of this, perhaps we can also simplify some of the hack/* tools (e.g. get rid of run-in-gopath.sh) and maybe simplify the _output/... mess

@liggitt
Copy link
Member Author

liggitt commented Mar 25, 2019

As part of this, perhaps we can also simplify some of the hack/* tools (e.g. get rid of run-in-gopath.sh) and maybe simplify the _output/... mess

I'd like to see those things get cleaned up, definitely, but we're sitting in between GOPATH and go modules at the moment. The output of this proposal produces a k8s.io/kubernetes repo that can work with go modules or with GOPATH. Once we're solidly in go modules territory (go 1.13+, possibly?) it will be more reasonable to require using go modules to build.

@liggitt
Copy link
Member Author

liggitt commented Mar 25, 2019

next PR updating this KEP is in #916

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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.