sig-architecture: document Go API guidelines#8975
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pohly The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
area/developer-guide
BenTheElder
left a comment
BenTheElder
left a comment
There was a problem hiding this comment.
First pass: Thank you and generally +1
I think surfacing this will be tricky, maybe we can, for example, emit a link alongside errors in any related presubmit tooling?
I'm also really wondering if we should shift to kubernetes.dev for docs like this, so it will be more readily search indexed, rendered, etc.
like https://www.kubernetes.dev/docs/code/cri-api-version-skew-policy/
Yes, absolutely.
Personally I don't care about rendering. I prefer simple Markdown over a more complex publishing pipeline. Right now I am not even sure where the source of kubernetes.dev is. Indexing in search engines might be relevant for some people. I know that you asked about kubernetes.dev and as far as I remember, there was no conclusion what to do about it. Until there is, I prefer to keep this doc closer to https://github.com/kubernetes/community/blob/main/contributors/devel/sig-architecture/api_changes.md. This makes it clear that this is the SIG Architecture guidance on this topic. |
pohly
force-pushed
the
go-api
branch
2 times, most recently
from
6b570d9 to
9fd9430
Compare
Maybe as an interim, incremental solution we should more aggressively cross-reference https://github.com/kubernetes/community/contributors/devel from kubernetes.dev? That page current links to https://github.com/kubernetes/community/tree/main/contributors/devel on the start page under "developer" guides, but it's easy to miss and once a developer reaches https://www.kubernetes.dev/docs/ it's nowhere in sight. |
| All staging repositories get published separately and may be used by other | ||
| projects. Changing their Go API must consider the impact on the ecosystem. In | ||
| particular, `k8s.io/client-go` and `k8s.io/apimachinery` are so widely used | ||
| that breaking Go API changes must be avoided as much as possible. |
There was a problem hiding this comment.
| that breaking Go API changes must be avoided as much as possible. | |
| that breaking Go API changes must be avoided as much as practical. |
We break compatibility even when alternatives exist like defining entirely new packages. It's less practical to create entirely new discovery packages, so we're ok doing things like adding to interfaces.
There was a problem hiding this comment.
Agreed, I meant the same thing.
There was a problem hiding this comment.
I also think that within these two packages the special attention is more on the client related code, we have a bunch of libraries that are not widely used, but a change on the generated code for the clients have a higher impact on consumers
| https://github.com/kubernetes/kubernetes/pull/129109 for an example using | ||
| this approach. | ||
|
|
||
| Because changing interfaces is a breaking change, prefer using concrete |
There was a problem hiding this comment.
this advice sounds strange.
There was a problem hiding this comment.
What I mean here is that if a package itself doesn't need an interface, it shouldn't define one and instead work with the specific type.
Some existing code was written using an interface although there is only one implementation of that interface. That could have been avoided and then e.g. kubernetes/kubernetes#137161 wouldn't have come up at all.
There was a problem hiding this comment.
I'm pretty sure I had a branch that extended exactly that interface for creating SharedInformers based on not-Kube APIs. I went in a different direction, but I went there first. Why intentionally break that or make it impossible?
There was a problem hiding this comment.
So you implemented the StoreController interface out-of-tree and then used your implementation with in-tree code? Or did you change the interface in-tree?
Changing that interface is perfectly fine exactly because it's only used internally with a single implementation. But that's not clear from the code. Not having a Store interface type and only a Store struct would have made that obvious.
Why intentionally break that or make it impossible?
Using an interface when there's currently no need for it has several disadvantages:
- The docs are usually worse, with terse comments in the interface and the actual comments on the implementation. The interface docs on gopkg render poorly and the implementation comments are not visible.
- Each change of the interface becomes a Go API break. If the interface is really widely used, at some point it becomes impossible to change it.
What I am arguing for is to keep the code as simple as possible. If there's an actual or expected need for the interface, then use it, otherwise don't.
There was a problem hiding this comment.
As I recall, the controller is used as an interface by the informer or sharedinformer. I'm not clear on why we'd restrict the original desire/intent to allow that as a pluggable unit with advice that says, "don't accept interfaces".
There was a problem hiding this comment.
I don't know if that was the original desire. It it was, fine, but was it really necessary? In practice, returning * Controller (pointer to struct) would have worked and it would have been better because of the reasons above.
I can remove the advice and replace it with a warning about the downsides of using interfaces. Is that better? Then someone reading it is free to choose either way.
There was a problem hiding this comment.
"don't accept interfaces"
I think that's exactly the point with Controller: I think we don't have a single API which accepts it, only APIs which return it. But changing it is still an API break because out-of-tree code might be using the type definition in their own code.
There was a problem hiding this comment.
I've replaced with a new "Interfaces" section which also includes some additional advice, like the trick with a private() method.
|
|
||
| # Stability goals | ||
|
|
||
| ## Staging repositories |
There was a problem hiding this comment.
IMHO within staging repositories there are two categories
k8s.io/apimachinery , k8s.io/client-go, and k8s.io/api ... those are the mandatory packages for any golang project using the kubernetes golang standard API , those are the ones with a large blast radius, and incompatible changes on those cause consumers (very large number) do do manual refactos during modules updates.
k8s.io/api is safe because it follows the api guidelines for the types, so things work just fine ... the problem is in k8s.io/apimachinery , k8s.io/client-go that in addition to the client it has helpers and tooling and people use it to create libraries that extend the surface and makes it hard to maintain or later to push back on keep adding functionalities
There was a problem hiding this comment.
Do you have some specific suggestion what we should add to the section? k8s.io/apimachinery and k8s.io/client-go already get called out explicitly. I'm not sure whether we need to mention k8s.io/api.
That the impact varies is covered by "may be used by other projects" and "consider the impact on the ecosystem". Code owners should know enough about usage to decide. If a contributor is uncertain, they should ask upfront or be ready to rework a PR. I don't think we can or should try to answer that here.
There was a problem hiding this comment.
I'm mostly dumping thoughts for the sake of completeness and to trigger discussions to gather feedback ... but not for blocking, we have zero today so current proposal is a big net improvement ... I don't have now a good way to rationalize this unfortunately
There was a problem hiding this comment.
Good, just wanted to be sure that I am not missing anything.
|
|
||
| This is enforced by https://github.com/kubernetes/kubernetes/pull/138351. | ||
|
|
||
| TODO: document here how that works?! |
There was a problem hiding this comment.
some references can be good
https://go.dev/talks/2016/refactor.article
https://go.dev/blog/compat
There was a problem hiding this comment.
I'll keep that in mind, but for now I want to merge this PR because then I can reference it properly in kubernetes/kubernetes#138351. Once that is also merged, I'll update this section. For now I have removed the TODOs to make it readable without further changes:
| https://github.com/kubernetes/kubernetes/pull/129109 for an example using | ||
| this approach. | ||
|
|
||
| ## Interfaces |
This documents (so far) informal best practices. The goal is to minimize the impact of accidental or intentional Go API changes on the wider ecosystem.
pohly
changed the title
| adapt. Use a tracking issue to make sure that future actions are not forgotten | ||
| and to provide further information about the deprecation plan. | ||
|
|
||
| For soft deprecations, consider whether the impact on downstream consumers |
There was a problem hiding this comment.
soft deprecation refers here to the case where we deprecate something for something new that didn't implement the full functionality? I think we need to be more strict here and only use the deprecated tag when to goal is to completely remove the deprecated thing and the entire functionality is provided ... I think is a binary decision, or you deprecate or not
There was a problem hiding this comment.
soft deprecation refers here to the case where we deprecate something for something new that didn't implement the full functionality?
No, that's a botched deprecation 😅 If we recommend using something else that doesn't actually work as a replacement without major effort during the migration, then we screwed up.
A soft deprecation is when we want consumers to do something else, but they are not required to switch because what they use right now will continue to work and be supported.
I think we need to be more strict here and only use the deprecated tag when to goal is to completely remove the deprecated thing
Agreed. That's a hard deprecation ("you must take action or... !") and deserves the formal deprecation comment.
5 participants
This documents (so far) informal best practices. The goal is to minimize the impact of accidental or intentional Go API changes on the wider ecosystem.
I intend to continue working on kubernetes/kubernetes#138351, then present both PRs in a SIG Architecture meeting. In the meantime, comments are welcome.
/cc @aojea @liggitt @dims @BenTheElder