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

KEP 4402: Go workspaces for k/k (first draft) #4403

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

thockin
Copy link
Member

@thockin thockin commented Jan 14, 2024

The main Kubernetes git repo (github.com/kubernetes/kubernetes, colloquially
called "k/k") is the empdiment of evolution. When it was created, the only
way to build Go applications was GOPATH. Over time we built tooling which
understood GOPATH, and it leaked into many parts of our build, test, and
release systems. Then Go added modules, in part because GOPATH was unpleasant
and had a tendency to leak into tools. We adopted modules, but we also added
the idea of "staging" repositories - a way to eat our cake and have, too. We
get the benefits of a monorepo (atomic commits, fast iteration across repos)
and then publish those to standalone repos for downstream consumption. To make
this work with modules, we abused GOPATH even harder and wrote even more tools.

The Go project saw what we (and others) were doing and did not like it. So
they created workspaces. They
basically created a solution that is purpose-built for us.

This KEP proposes that we adopt Go workspaces and bring our tooling up to
modern standards. In fact, the author started this work in 2021 or 2022,
discovering issues along the way that the Go team has worked to resolve.

This is not a user-facing change. No k8s cluster-user or cluster-admin should
know or care that this happened. On the surface, it seems like something that
should not warrant a KEP, but:
a) KEPs are how we communicate big changes;
b) This is a big change;
c) There's some ecosystem impact to developers

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2024
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2024
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

Let's do this... 👍 from me.

Some small nits on the text itself.

keps/sig-architecture/4402-go-workspaces/README.md Outdated Show resolved Hide resolved
keps/sig-architecture/4402-go-workspaces/README.md Outdated Show resolved Hide resolved
keps/sig-architecture/4402-go-workspaces/README.md Outdated Show resolved Hide resolved
keps/sig-architecture/4402-go-workspaces/README.md Outdated Show resolved Hide resolved
keps/sig-architecture/4402-go-workspaces/README.md Outdated Show resolved Hide resolved
keps/sig-architecture/4402-go-workspaces/README.md Outdated Show resolved Hide resolved
@thockin
Copy link
Member Author

thockin commented Jan 14, 2024

Most comments addressed, added details about sequencing PRs.

TBD:

  1. Break k8s.io/code-generator for legacy users (tag it and tll them to pin), vs. code-generator/v2 (in staging), vs. fork legacy-code-generator in staging.
  2. Change kube-openapi in place or kube-openapi/v2

@pohly
Copy link
Contributor

pohly commented Jan 15, 2024

Break k8s.io/code-generator for legacy users (tag it and tll them to pin), vs. code-generator/v2 (in staging), vs. fork legacy-code-generator in staging.

I doubt that we have the necessary tooling to publish a k8s.io/code-generator/v2 from staging/src/k8s.io/code-generator: the next tagged release will be v0.30.0, not some v2.0.0.

Forking it as legacy-code-generator would work, but not help consumers who are using k8s.io/code-generator without pinning to a version.

We could fork it as code-generator-v2 and leave the old staging/src/k8s.io/code-generator more or less unchanged from now on (minimal maintenance forever) or declare it as deprecated and remove it at some point in the future. We can also do this in stages:

  • in 1.30, fork it internally as staging/src/k8s.io/code-generator-v2 without publishing it as k8s.io/code-generator-v2
  • in 1.31, publish it and declare k8s.io/code-generator as deprecated
  • at some point later, remove staging/src/k8s.io/code-generator

I don't know how disruptive the changes will be. If we want to be nice to the eco system, then playing it safely with fork/deprecate might be better. If we want to be done quickly (= "rip off the bandage"), then we don't fork and just warn consumers about the change.

Change kube-openapi in place or kube-openapi/v2

No strong opinion here. Just beware that if we start versioning it, then someone has to remember to do releases from now on because "latest" would refer to the latest release, not the master branch as it does now.

@thockin
Copy link
Member Author

thockin commented Jan 15, 2024

I doubt that we have the necessary tooling to publish a k8s.io/code-generator/v2

Agree

@thockin
Copy link
Member Author

thockin commented Jan 16, 2024

@sttts @soltysh @deads2k - can you bring more eyes to this?

@thockin thockin mentioned this pull request Jan 28, 2024
5 tasks
@MikeSpreitzer
Copy link
Member

@MikeSpreitzer

@thockin
Copy link
Member Author

thockin commented Jan 30, 2024

With KEP freeze coming, I would love to have the KEP approved, so that the implementation has a chance of landing in .30

@jpbetz and @liggitt are listed as approvers. I'd appreciate even an initial ACK from @soltysh if possible?

@jpbetz I took the liberty of making you PRR for this one, is it a problem if you are also approver?

@jpbetz
Copy link
Contributor

jpbetz commented Jan 31, 2024

@jpbetz I took the liberty of making you PRR for this one, is it a problem if you are also approver?

Not a problem! I've seen this being done elsewhere.

@jpbetz
Copy link
Contributor

jpbetz commented Jan 31, 2024

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2024
@liggitt
Copy link
Member

liggitt commented Jan 31, 2024

/lgtm
/approve

v2-experiment.md Outdated Show resolved Hide resolved
@liggitt
Copy link
Member

liggitt commented Jan 31, 2024

/lgtm cancel

the v2-experiment file is in the wrong folder. move that, then lgtm

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2024
@thockin
Copy link
Member Author

thockin commented Jan 31, 2024

File moved

@liggitt
Copy link
Member

liggitt commented Feb 1, 2024

/lgtm
/approve
/assign @dims

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2024
@dims
Copy link
Member

dims commented Feb 1, 2024

/lgtm
/approve
/hold ... please remove hold when ready (need more reviewers?)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, jpbetz, liggitt, thockin

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 Feb 1, 2024
@thockin
Copy link
Member Author

thockin commented Feb 1, 2024

If it's OK, I'd like merge and iterate. If @soltysh or someone else can weigh in, it would be awesome, but I have pretty high confidence this is the right direction and any niggles will emerge in the PR.

@thockin thockin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2024
@k8s-ci-robot k8s-ci-robot merged commit 94c7184 into kubernetes:master Feb 1, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Feb 1, 2024
participating-sigs:
- sig-release
- sig-api-machinery
status: provisional
Copy link
Member

Choose a reason for hiding this comment

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

need this updated to implementable by next Thursday

@thockin
Copy link
Member Author

thockin commented Feb 1, 2024 via email

@soltysh
Copy link
Contributor

soltysh commented Feb 7, 2024

If it's OK, I'd like merge and iterate. If @soltysh or someone else can weigh in, it would be awesome, but I have pretty high confidence this is the right direction and any niggles will emerge in the PR.

Sorry was out last week, but I don't have any objections. I'll keep on looking through the appropriate PRs.

@thockin
Copy link
Member Author

thockin commented Feb 7, 2024

You'll have time to object as PRs queue up. The first one is for gengo, which probably doesn't affect you. Once the k/k PR(s) stabilize, you will have something proper to try.

Now that 1.22 is released, I will rebase the k/k PR and it should mostly pass.

Once KEPs freeze, I will push more to get PRs approved to keep the pipeline moving.

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/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.