client-go needs backwards compatibility test

[caesarxuchao]

Client-go needs backwards compatibility test, i.e, checking if a PR changes the public interfaces of client-go. The benefits are:

• Ensuring release-note is added if a PR has backwards incompatible changes.
• Making breaking changes more obvious, so developers can avoid them if possible.
• Helping determine if the major version of client-go needs to be bumped when we cut a release.

The backwards compatibility test is useful no matter if we develop client-go in the kubernetes repo or in this repo.

Special requirements:

• Need to check backwards compatibility of the underlying k8s.io repos, including k8s.io/apimachinery and k8s.io/api, because client-go user deals with their interfaces as well.

Action items:

• find existing tool that looks for breaking changes.
• alternatively write a tool using go2idl.

[caesarxuchao]

cc @lavalamp @mbohlool @ericchiang @hongchaodeng

Do you know if there is an existing tool that checks for changes in public interfaces? I couldn't find any.

[lavalamp]

If repos were split I would instead want to solve this via requiring release-note: sections in changes to client-go. Changes in downstream repos (e.g., api types) would be handled by requiring the same release-note tag in the PRs that vendor in the changes. It's this last step that isn't really feasible in the current monorepo--it's hard to programmatically tell what PRs are going to ultimately have an effect on the client's public interface.

[caesarxuchao]

The release-note approach relies on the author and the reviewer to identify the breaking changes. I'd rather relying on the tool to force the author to add release-note.

[deads2k]

The release-note approach relies on the author and the reviewer to identify the breaking changes. I'd rather relying on the tool to force the author to add release-note.

I agree. An automated process makes more sense. Given that the stable API surface is smaller than all packages, I think I would pursue the idea of labeling certain packages as exported (similar to an osgi bundle). Then a tool could check

1. have you listed all the packages that are actually API, since transitive dependencies matter.
2. have you changed any of those packages in compatible ways (minor update).
3. have you changed any of those packages in incompatible ways (major update).

I think that identification of those cases is necessary no matter how we decide to handle such changes.

@kubernetes/sig-api-machinery-misc @sttts

[sttts]

👍 for the labeling idea

[lavalamp]

An automated process can require the release note. This is how it works in the main repo. We need to duplicate this functionality for each library.

If someone wants to write a tool that walks back the import tree from the perspective of each library and requires a release-note-xxx for each staging/xxx whose public interface is affected, I'm fine with that. If we stay in the main repo a single change might require a release note / changelog entry for 5 different libraries.

[caesarxuchao]

Given that the stable API surface is smaller than all packages,

Could you elaborate on what packages should be excluded from the API surface? The packages that don't have public functions/variables? @deads2k

[lavalamp]

package examplelib

import (
  "k8s.io/flunder" // used but not exported 
  "k8s.io/argle" // used and exported 
)

func DoIt(b *argle.Bargle) {
  f := flunder.Flund(b)
}

In this short program, changes to k8s.io/argle potentially affect the public interface of this package, but changes to k8s.io/flunder don't. (It may still require certain versions of the flunder library to compile or work correctly, of course.)

[sttts]

Could you elaborate on what packages should be excluded from the API surface?

As another example, I consider many of the util/ packages as not part of the API surface. E.g. HomeDir and MkTmpdir, maybe the cert utils.

To make that clear, we should move everything that is not part of the API surface to pkg/. And IMO we should be very strict about what we leave in the API.

[ericchiang]

Do you know if there is an existing tool that checks for changes in public interfaces? I couldn't find any.

@caesarxuchao Go has some internal tooling used to track the standard library API https://github.com/golang/go/tree/master/api

I whipped up some tooling that mimics that, maybe that'd be a good place to start? https://github.com/ericchiang/gopkgapi

edit: here's an example document for k8s.io/client-go/rest https://gist.github.com/ericchiang/5667fdab28a8d6c01d0a656fad34f88c

[caesarxuchao]

This issue turned out to be bigger than client-go. @lavalamp opened up kubernetes/test-infra#3415. I'll keep this issue open until kubernetes/test-infra#3415 has an owner.

Thanks @ericchiang. That's a good start. I'll mention it in kubernetes/test-infra#3415.

[caesarxuchao]

@lavalamp @sttts thanks for the answers. I think @lavalamp's point is that not all public interface of a vendored repo should be count as the API of client-go; and @sttts's point is that anything in client-go/pkg should not be considered as an API. I agree with both.

[lavalamp]

I do not agree that we can make things not part of the public interface by putting them under pkg/. We can use go's internal package feature to do that. I think any public variable, function, constant etc is part of the interface. If it was possible for a consumer of the library to reference a thing, then that thing is part of the public interface.

[sttts]

@lavalamp technically this is true. But nothing stops us from declaring conventions. It's more about pragmatism in the light of Golang's limited visibility features and our bad history of maintaining any kind of compatibility. I would pretty much prefer to have a limited set of packages declared stable and maintained stable than going the extreme way considering every small bit of code as stable and increasing the major version on every release.

[lavalamp]

The deliverable here is, when do users have to change their code to upgrade? We cannot stop them from referencing items in /pkg, therefore if we don't increment the major version number on changes there we are basically falsely claiming that their code needn't change, and they will be in for an unpleasant surprise.

(I am a consequentialist, so the fact that it would be their own damn fault doesn't move me at all--it certainly wouldn't stop them from filing support issues.)

We can either put such code in .../internal/... packages, move it to a separate 100% unstable repo, or version it.

TL;DR: the problem isn't the frequency with which we increment the version number. The problem is the frequency at which folks have to change their code on upgrades.

[caesarxuchao]

cc @jennybuckley @roycaihw who might be interested in expanding the tool Eric built.

[fejta-bot]

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.

Prevent issues from auto-closing with an /lifecycle frozen comment.

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

[sttts]

/lifecycle frozen

[spiffxp]

/remove-help
I don't think this meets the criteria for help wanted. Please add back via /help if I'm incorrect

[nikhita]

/remove-lifecycle stale

[smola]

I'm working on a tool to check Go APIs compatibility. It still lacks some important features, such as saving the API data at a given point for later comparison, it currently compares two git references, but I'll be working on expanding this. If this looks like something useful to you, I'll be glad to receive feedback about your use case:

https://github.com/smola/gocompat

[liggitt]

https://github.com/golang/exp/blob/master/apidiff might be a possibility as well

[liggitt]

sample verify script with API package compatibility checks here: https://github.com/liggitt/kubernetes/commits/apidiff-master

comparison of master against 1.14 and 1.15:

k8s.io/apimachinery ==========

v1.14.0 =====
pkg.api.meta:
- List.GetRemainingItemCount: added
- List.SetRemainingItemCount: added
pkg.apis.meta.v1:
- (*ObjectMeta).GetInitializers: removed
- (*ObjectMeta).SetInitializers: removed
- Initializer: removed
- Initializers: removed
- ListInterface.GetRemainingItemCount: added
- ListInterface.SetRemainingItemCount: added
- Object.GetInitializers: removed
- Object.SetInitializers: removed
- ObjectMeta.Initializers: removed
pkg.apis.meta.v1.unstructured:
- (*Unstructured).GetInitializers: removed
- (*Unstructured).SetInitializers: removed
pkg.apis.meta.v1beta1:
- PartialObjectMetadataList.Items: changed from []*PartialObjectMetadata to []k8s.io/apimachinery/pkg/apis/meta/v1.PartialObjectMetadata
pkg.apis.testapigroup:
- (*ObjectMeta).GetInitializers, method set of *Carp: removed
- (*ObjectMeta).SetInitializers, method set of *Carp: removed
- Carp.Initializers: removed
pkg.apis.testapigroup.v1:
- (*ObjectMeta).GetInitializers, method set of *Carp: removed
- (*ObjectMeta).SetInitializers, method set of *Carp: removed
- Carp.Initializers: removed
pkg.runtime:
- Unstructured.NewEmptyInstance: added
pkg.runtime.serializer.protobuf:
- NewRawSerializer: changed from func(k8s.io/apimachinery/pkg/runtime.ObjectCreater, k8s.io/apimachinery/pkg/runtime.ObjectTyper, string) *RawSerializer to func(k8s.io/apimachinery/pkg/runtime.ObjectCreater, k8s.io/apimachinery/pkg/runtime.ObjectTyper) *RawSerializer
- NewSerializer: changed from func(k8s.io/apimachinery/pkg/runtime.ObjectCreater, k8s.io/apimachinery/pkg/runtime.ObjectTyper, string) *Serializer to func(k8s.io/apimachinery/pkg/runtime.ObjectCreater, k8s.io/apimachinery/pkg/runtime.ObjectTyper) *Serializer
pkg.runtime.serializer.versioning:
- DirectDecoder: removed
- DirectEncoder: removed
pkg.watch:
- NewStreamWatcher: changed from func(Decoder) *StreamWatcher to func(Decoder, Reporter) *StreamWatcher

v1.15.0 =====
pkg.apis.meta.v1:
- (*ObjectMeta).GetInitializers, method set of *PartialObjectMetadata: removed
- (*ObjectMeta).GetInitializers: removed
- (*ObjectMeta).SetInitializers, method set of *PartialObjectMetadata: removed
- (*ObjectMeta).SetInitializers: removed
- Initializer: removed
- Initializers: removed
- Object.GetInitializers: removed
- Object.SetInitializers: removed
- ObjectMeta.Initializers: removed
- PartialObjectMetadata.Initializers: removed
pkg.apis.meta.v1.unstructured:
- (*Unstructured).GetInitializers: removed
- (*Unstructured).SetInitializers: removed
pkg.apis.testapigroup:
- (*ObjectMeta).GetInitializers, method set of *Carp: removed
- (*ObjectMeta).SetInitializers, method set of *Carp: removed
- Carp.Initializers: removed
pkg.apis.testapigroup.v1:
- (*ObjectMeta).GetInitializers, method set of *Carp: removed
- (*ObjectMeta).SetInitializers, method set of *Carp: removed
- Carp.Initializers: removed
pkg.runtime.serializer.versioning:
- DirectDecoder: removed
- DirectEncoder: removed