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 adoption checklist #68577

Closed
sigma opened this issue Sep 12, 2018 · 24 comments

Comments

@sigma
Copy link
Contributor

commented Sep 12, 2018

/kind feature

Description:

This issue is to track the list of issues for a go modules (aka vgo) adoption. The list itself is a work in progress, and will be updated as issues popup or get resolved.

First class of issues: the current godep snapshot of packages is not a valid solution as far as go modules are concerned

This list needs to be empty to actually perform the migration. Post-migration, the burden will move to the dependency update workflow (similar issues will block the update).

Some more issues are to be expected, due to mainly 2 factors:

  • go modules reason about test dependencies in addition to "regular" dependencies, which we currently completely ignore
  • a large chunk of our dependencies are not versioned at all, meaning that no semver guarantees apply. It'll be easy to get incompatibilities once our dependencies start using go modules themselves.

Second class of issues: tooling needed

  • it takes a non-negligible amount of effort to convince go to vendor everything we need at the top-level in k/k, given that some of our dependencies are actually only used by our staging sub-projects.
  • since go modules don't track modifications at the sub-package level, we need to maintain Godeps.json files for a while, and there's no official tooling for that.
  • our go modules dependencies need to be kept in sync across k/k and all staging repos.

https://github.com/sigma/vgo-k8s-tools has some (ugly) code to address all these, but is very much a work in progress

Third class of issues: go modules usability

  • extremely opaque about what it does, and why. Took me some grepping the cache to understand why golang.org/x/exp was bumped in the first place in #68478. With our kind of dependency graph complexity, lack of clarity is hard to handle.
  • we'll still need some scripting around go modules to maintain our dependency graph. Hopefully that should be after running regular module operations in the main or staging repos.
@sigma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2018

@kubernetes/sig-testing-proposals

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

@sigma: Reiterating the mentions to trigger a notification:
@kubernetes/sig-testing-proposals

In response to this:

@kubernetes/sig-testing-proposals

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Sep 12, 2018

/cc

@spiffxp

This comment has been minimized.

Copy link
Member

commented Sep 12, 2018

@bgrant0607

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@lavalamp

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

Excited to have someone looking at this!

@sdboyer

This comment has been minimized.

Copy link

commented Sep 28, 2018

It's worth noting that the path of least resistance (to the module system) will ultimately be to treat each currently-discrete staging repo as its own go module, and abandon the separate staging repos entirely.

ive also been thinking about how useful some kind of declaration would be to automatically treat all imports under a particular root as though there were a local replace declaration. As the number of Go modules within k8s increases, this will be increasingly useful, as it will obviate the need to litter replace statements everywhere.

@sigma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

a few thoughts:

  • I don't know that the organization of the repo is the main issue. I think the higher-level problem is that we need those modules to be kept in sync (in terms of dependency graph) regardless of whether they live in the same repo or not, short of changing our support model. And there's just very little tooling support for doing that right now
  • somehow the maintenance of the replace directives doesn't worry me so much, as it's one thing that we can pretty easily automate. But sure, a shortcut would be nicer.
@sdboyer

This comment has been minimized.

Copy link

commented Sep 28, 2018

I'd say keeping deps in sync is somewhat orthogonal to module nesting - given that everything is already developed in a single repo, you avoid a whole class of drift problems. My point was more that you have

idk what assumptions tooling make about having those separate repos because idk the tooling. my naive view is that it should be a fairly simple removal: pick a new import path within k/k for staging contents, move the dirs, add go.mod files, and update import paths. the other repos can then just go away; there might also be some redirect magic possible.

My concern with replace directives is having multiple PRs batches together for CI that both change replaces: an automated solution MAY be possible, but it would certainly require a fixup commit at the very least.

Having it declared once makes that easier. It also may make release prep easier - certainly less to clean up, but it also might be feasible to keep require statements more up to date throughout development. (I haven't thought this latter point through fully)

@fejta-bot

This comment has been minimized.

Copy link

commented Dec 27, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@sigma

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

/remove-lifecycle stale

@ixdy

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Has there been any progress on this? Go modules are likely to become the default in Go 1.13 (August 2019), and given the lifetime of Kubernetes releases, we should probably get ahead of that.

@ixdy

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

@sigma

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

As far as I'm concerned, I'm still waiting on heketi/heketi#1272 to be merged.

I would add that the difficulty of getting those ducks in a row seems like a systemic problem in the go modules approach. I'm not quite sure the incentives are stacked the right way.

liggitt referenced this issue in kubernetes/api Feb 6, 2019
Remove alpha InitializerConfiguration types, Initializers admission p…
…lugin

Kubernetes-commit: dc1fa870bff65c20f48a83ea3af54adb3f526e28
@andreykaipov

This comment has been minimized.

Copy link

commented Feb 20, 2019

Sorry for my ignorance, but as someone fairly new to Go and Kubernetes, what's the problem with just running go mod init? I cloned this repo outside my GOPATH, ran go mod init, Go found what it needed from the Godeps, and boom - make worked with no issue and I didn't even cry once. I was shocked it worked so smoothly.

Go modules are experimental in Go 1.11, but would having a go.mod and go.sum in the root directory of the project really affect anything dependent on just Godep? Why not just add support for it now?

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Go modules are experimental in Go 1.11, but would having a go.mod and go.sum in the root directory of the project really affect anything dependent on just Godep? Why not just add support for it now?

Yes it can affect them, adding a go.mod (previously on accident) broke some tooling that depends on Kubernetes being findable in GOPATH, it couldn't query the package location anymore. Many tools in & out of the repo depend on GOPATH pretty heavily unfortunately, we need to disentangle that.

We also shouldn't check it in unless we're going to keep it up to date, so we need to update all the godeps scripts / presubmit(s) and the docs.

We also need to consider the publishing robot for staging/, the cross build container, if we will retain vendor/ (and if not if we will run a module proxy to avoid CI flakiness).

We've started using go modules for some other smaller sub-projects, and ran into bugs with the checksums (they should and to seem to be fixed in >= go 1.11.4 now) ... it's probably worth shaking out the issues in these smaller projects first and letting it become stable before we switch the main repo.

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

With go1.12 coming soon it has to be about time, but I don't think we have all of those things figured out just yet.

cc @cblecker @dims

@sigma

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Sorry for my ignorance, but as someone fairly new to Go and Kubernetes, what's the problem with just running go mod init? I cloned this repo outside my GOPATH, ran go mod init, Go found what it needed from the Godeps, and boom - make worked with no issue and I didn't even cry once. I was shocked it worked so smoothly.

This is a bit misleading. It seems to work, but builds the wrong thing because staging/ is then ignored as the source of many of the k8s.io packages. Basically this allows go mod to reason about a much looser dependency graph, and ultimately claim "victory". When you enforce actual alignment of the dependency graph is when problems above start showing up.

@danielqsj

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

/cc

@munnerz

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

I've opened heketi/heketi#1542 against heketi/heketi that bumps Kubernetes dependencies to client-go 1.10 😄 hopefully that's sufficiently new to have compatibility with the HEAD of kubernetes/kubernetes at the moment, but at the very least it'll make it far easier to bump again in future.

edit: this seems to build fine: https://github.com/kubernetes/kubernetes/compare/master...munnerz:bump-heketi?expand=1

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

I'm writing up a KEP for the 1.15 release for this here: kubernetes/enhancements#908

I also have a WIP PR that uses go mod vendor to rebuild the vendor dir, populates go.mod files for each staging repo as a module.

The biggest unresolved question is what versioning strategy to use for the modules. Options are described in the KEP

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Accepted proposal for initial go module support is at https://github.com/kubernetes/enhancements/blob/master/keps/sig-architecture/2019-03-19-go-modules.md

#74877 contains the changes needed to reproducibly generate the vendor dir using go modules

kubernetes/publishing-bot#169 is a WIP PR to publish go.mod files for staging component

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

#74877 is merged and kubernetes/kubernetes vendoring is now managed using go modules

/close

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

@liggitt: Closing this issue.

In response to this:

#74877 is merged and kubernetes/kubernetes vendoring is now managed using go modules

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.