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.