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

Add Dependency matrix to README #337

Closed
wants to merge 1 commit into from

Conversation

markmandel
Copy link

@markmandel markmandel commented Nov 29, 2017

This makes it far more explicit about all the libraries that are used in conjunction with client-go and which
versions you require for each version of Kubernetes.

Context: Friction Log

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 29, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 29, 2017
README.md Outdated
| tag: v2.0.x / branch: release-2.0 | - | - | - |
| tag: v3.0.x / branch: release-3.0 | branch: release-1.6 | - | - |
| tag: v4.0.x / branch: release-4.0 | branch: release-1.7 | branch: release-1.7 | branch: release-1.7 |
| tag: v5.0.x / branch: release-5.0 | branch: release-1.8 | branch: release-1.8 | branch: release-1.8 |
Copy link
Contributor

Choose a reason for hiding this comment

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

we do not support v5.0.x with release-1.8 in general. We support branch HEADs together, and we support v5.0.x with kubernetes-1.8.x (note: the same x).

Copy link
Author

Choose a reason for hiding this comment

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

Deleted previous comment after reading what you wrote 3 times. This is very confusing.

I'll rewrite the table, see if I can capture what you are saying.

Copy link
Contributor

Choose a reason for hiding this comment

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

You sentence in the previous comment was good. v5.0.x with kubernetes-1.8.x of the other repos. v6.0.y with kubernetes-1.9.

If you talk about branches, release-5.0 HEAD (i.e. the current top commit) matches release-1.8 HEAD.

Copy link
Author

Choose a reason for hiding this comment

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

Cool - thanks for explaining.

The only question I have (and I'd like to capture this) - is what you feel the reason why someone would use the version tag vs using the release branch?

Do we want to push users to using one over the other?

Copy link
Author

Choose a reason for hiding this comment

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

To make matters worse https://github.com/kubernetes/apimachinery doesn't have any 1.6.x or 1.7.x tags, it only has a release-1.6 and release-1.7 branches.

So for older versions - use the release branches, but for newer ones use the tags?

Copy link
Author

Choose a reason for hiding this comment

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

I actually realised this is even more confusing.

client-go also has kubernetes-1.8.x tags... which don't line up with the client go 5.0.x tags

Should we be pushing people towards that?

@sttts
Copy link
Contributor

sttts commented Nov 30, 2017 via email

@@ -124,6 +125,20 @@ Key:
* `=` Maintenance is manual, only severe security bugs will be patched.
* `-` Deprecated; please upgrade.

#### Dependency Matrix
Since there are multiple libraries that client-go is either required to, or can work
with, this is a list of all the libraries and versions that work with each other.
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not to make this in the form of a chart. I would say, "here are the dependency libraries you might need: <list>; corresponding commits are all tagged with kubernetes-x.x.x tags. If you use godep or glide you should not need to know this."

I would especially avoid mentioning e.g. release-1.6 branch in the apimachinery repo, as IMO it is not versioned yet and I don't want people to expect that pattern to continue--it should switch to a semver system like client-go.

Copy link
Author

Choose a reason for hiding this comment

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

But we need to give people an approach that works now, and for older (1.7 and lower) releases, the release branch is the only thing that exists in apimachinery.

Copy link
Member

Choose a reason for hiding this comment

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

People starting fresh shouldn't use the older releases. People not starting fresh already have something that works and don't have this problem.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I don't understand this logic - are we assuming that there are no <= 1.7 kubernetes clusters that want to use client-go in the future?

Copy link
Member

Choose a reason for hiding this comment

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

Newer client releases are backwards compatible, so basically, yes, I don't see why you would start out using something not the latest.

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough - then the simplification we've discussed further down will make more sense.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 30, 2017
@markmandel
Copy link
Author

@sttts @lavalamp - updated to provide clear separation between the more recent, and previous versions of client-go, and how to manage those dependencies.

PTAL

README.md Outdated

##### Branch Versions Dependencies

For versions of client-go based on Kubernetes <= 1.7, the HEAD of each release
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also the case for 1.8+. Those branch HEADs always match and are smoke tested by our scripts, plus they are CI tested in Kubernetes itself. They don't belong to an official client-go release though. So technically we do not support them. I.e. they are just like the master branch or the release branches of kube itself.

Copy link
Author

Choose a reason for hiding this comment

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

So that makes an interesting problem for people who may need go-client <= kubernetes-1.7 - there is no way to get a supported tag / branch combo between client-go and api-machinery or other repositories, since you are telling me that release branches are not supported, and it's not supported to have a tag release + a branch release.

(This is because apimachinery, etc don't have version tags <= 1.7 on them)

Maybe they should have those tags?

What should we tell users who may need to use that specific version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe they should have those tags?

We could do that. There was an issue due to too few changes in the 1.7 branches which broke the "new" godep algorithm. So I did not continue with the 1.7 branch (1.8 was around the corner and hi prio).

What should we tell users who may need to use that specific version?

As @lavalamp wrote elsewhere: the Godeps.json has valid sha1s which are proven to work. Godep and glide will pick them up to resolve the versions correctly.

Dep won't do that, so dep users have not much help – and as you say no tags for 1.7 either. Compare @ncdc's blog https://medium.com/@andy.goldstein/upgrading-kubernetes-client-go-from-v4-to-v5-bbd5025fe381. Andy uses the branch approach there plus the semver tag. This is formally unsupported.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, so should I say that "for <= 1.7 you will need to use Glide or godep to get a supported package set of libraries or look directly at the Godep.json" ?

And then change the branch dependency matrix to say that it is tested, but isn't supported (or drop it altogether - really, I just want to make sure it's clear what is supported - I'm leaning towards dropping it, it may make things cleaner and easier to understand)

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so should I say that "for <= 1.7 you will need to use Glide or godep to get a supported package set of libraries or look directly at the Godep.json" ?

Sounds good.

And then change the branch dependency matrix to say that it is tested, but isn't supported (or drop it altogether - really, I just want to make sure it's clear what is supported - I'm leaning towards dropping it, it may make things cleaner and easier to understand)

I would just drop it.

Copy link
Author

Choose a reason for hiding this comment

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

SGTM. Will make the changes and ping when ready.

@sttts
Copy link
Contributor

sttts commented Dec 1, 2017

One comment about the branches. Otherwise it looks good 👍

README.md Outdated
Using the git tag versions is the preferred way of managing dependencies for newer
releases of client-go

| client-go | [apimachinery](https://github.com/kubernetes/apimachinery) | [apiextensions-apiserver](https://github.com/kubernetes/apiextensions-apiserver)<sup>*</sup> | [code-generator](https://github.com/kubernetes/code-generator)<sup>+</sup> |
Copy link
Member

@lavalamp lavalamp Dec 1, 2017

Choose a reason for hiding this comment

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

Why do people need both client-go and apiextensions-apiserver linked in the same binary?

Copy link
Author

Choose a reason for hiding this comment

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

If you wanted to programmatically access CRDs and regular Kubernetes resources in the same binary.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

apiextensions-apiserver is doing it wrong, then, its api needs to be available separately. It is madness to require clients to vendor server code just to get the client.

README.md Outdated

| client-go | [apimachinery](https://github.com/kubernetes/apimachinery) | [apiextensions-apiserver](https://github.com/kubernetes/apiextensions-apiserver)<sup>*</sup> | [code-generator](https://github.com/kubernetes/code-generator)<sup>+</sup> |
| ----------------------------------| -----------------------------------------------------------|----------------------------------------------------------------------------------------------|----------------------------------------------------------------------------|
| tag: v5.0.x | tag: kubernetes-1.8.x | tag: kubernetes-1.8.x | tag: kubernetes-1.8.x |
Copy link
Member

Choose a reason for hiding this comment

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

client-go also has the kubernetes-1.8.x tags in addition to the v5.0.x tags. Why is this table necessary?

Copy link
Author

Choose a reason for hiding this comment

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

From the slack conversation we had - we want developers to use the semver version on client-go rather than the kubernetes-1.x tags.

A table makes this relationship explicit, and very easy to understand.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 1, 2017
@markmandel
Copy link
Author

markmandel commented Dec 1, 2017

Updated again - @lavalamp @ncdc @sttts - I think this feel very close. Down to one table, and made comments about earlier versions requiring Glide/godep if for whatever reason the developer has to use an older version, but also making a comment that people should use the latest version.

Once I have a LGTM, I'll rebase it down to a single commit.

| tag: v6.0.x | tag: kubernetes-1.9.x | tag: kubernetes-1.9.x | tag: kubernetes-1.9.x |

<sup>*</sup> optional library that exposes the Extensions API, for example, for
Custom Resource Definitions.
Copy link
Member

Choose a reason for hiding this comment

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

Add: "This appears here temporarily, as the API types / client are not yet available separately."

Copy link
Author

Choose a reason for hiding this comment

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

How about:
"This appears here temporarily, as apiextensions-apiserver will eventually be merged into client-go"
(or as close to that as is accurate)

It means a developer doesn't have to know what an API type / client means in this context.

How does that sound?

Copy link
Author

Choose a reason for hiding this comment

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

I'm hesitant to add this here, at least until such a change is finalised and documented. There is already developer concern over client-go's high rate of breaking changes.

Is there a specific reason we should warn people that a change is coming in the future, before we know what that change is?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't pre-announce any future change here either. When we move the API, we will annoucne that in the release notes. Let's describe the current situation.

@lavalamp
Copy link
Member

lavalamp commented Dec 1, 2017 via email

@lavalamp
Copy link
Member

lavalamp commented Dec 1, 2017 via email

@markmandel
Copy link
Author

markmandel commented Dec 1, 2017

@lavalamp but from an outsiders perspective - all I'm going to see is that one library that I use, is going to be replaced by another library that isn't the same as before - and unless they understand Kubernetes' internals (which isn't most of our audience), all they are going to see is a breaking change.

I personally have no idea what apiextensions-apiserver actually does, or why it's bad that developers have to reference it, and I'm relatively familiar with client-go, I don't see how we can expect anyone else to be.

README.md Outdated
developers always use the latest version.

If you are using [Glide](https://glide.sh) or [godep](https://github.com/tools/godep)
to manage your Go dependencies, this will manage the non-optional libraries automatically.
Copy link
Contributor

Choose a reason for hiding this comment

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

godep is not automatic. Instead, you have to do godep restore from client-go and godep save from your 3rdparty code base.

I would prefer something along these lines:

Client-go specifies non-optional libraries in its Godeps/Godeps.json file. While using glide will respect these automatically, with glide you have to manually godep restore them from client-go's directory and then godep save them in your project.

Copy link
Contributor

Choose a reason for hiding this comment

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

And please add dep:

Note that golang/dep currently does not respect the specified dependencies in client-go. You have to manually add them as constraints in your Gopkg.toml for all its dependencies (not only for k8s.io/api and k8s.io/apimachinery!).

Copy link
Author

Choose a reason for hiding this comment

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

Was that original comment meant to say:

Client-go specifies non-optional libraries in its Godeps/Godeps.json file. While using glide will respect these automatically, with godep you have to manually godep restore them from client-go's directory and then godep save them in your project.

Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

The one quoted? Yes.

Copy link
Author

Choose a reason for hiding this comment

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

Updated and rebased. PTAL.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 11, 2017
@sttts
Copy link
Contributor

sttts commented Dec 11, 2017

lgtm
/approve no-issue

Please squash into one commit. Then it's ready to go.

This makes it far more explicit about all the libraries
that are used in conjunction with client-go and which
versions you require for each version of Kubernetes.
@markmandel
Copy link
Author

SQUASHED! 🔨

@sttts
Copy link
Contributor

sttts commented Dec 11, 2017

/lgtm

@caesarxuchao can you merge?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2017
@sttts
Copy link
Contributor

sttts commented Dec 11, 2017

I believe travis-ci is broken. Will take a look tomorrow.

Copy link
Member

@caesarxuchao caesarxuchao left a comment

Choose a reason for hiding this comment

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

@markmandel sorry I have a few late comments. I'll be OOO tomorrow until the new year, could you get @lavalamp to approve the PR on my behalf?

Since there are multiple libraries that client-go is either required to, or can work
with, this is a list of all the libraries and versions that work with each other.

| client-go | [apimachinery](https://github.com/kubernetes/apimachinery) | [apiextensions-apiserver](https://github.com/kubernetes/apiextensions-apiserver)<sup>*</sup> | [code-generator](https://github.com/kubernetes/code-generator)<sup>+</sup> |
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer splitting the form into libraries that depend on client-go (e.g., apiextensio-apiserver, apiserver, kube-aggregator), and libraries that client-go depends on (apimachinery, api). For the latter, this README should point out that the versions recorded in Godeps/Godeps.json is the ground truth.

And code-generator is worth its own "generated-by" section.

Copy link
Member

Choose a reason for hiding this comment

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

In addition to agreeing with everything Chao says here ( :) ) I am worried that this documentation may quickly diverge.

Instead of listing a table, can we say something more along the lines of:

Kubernetes publishes multiple repos for piecemeal inclusion; commits that work with
each other are tagged with tags like `kubernetes-1.<minor_version>.<patch_version>`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of listing a table, can we say something more along the lines of:

I read that just as an example. So it cannot really diverge. I think a table is easier to consume, but I am fine with @lavalamp's proposed sentence as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we have something very similar already:

Kubernetes tags

As of October 2017, client-go is still a mirror of k8s.io/kubernetes/staging/src/client-go, the code development is still done in the staging area. Since Kubernetes 1.8 release, when syncing the code from the staging area, we also sync the Kubernetes version tags to client-go, prefixed with "kubernetes-". For example, if you check out the kubernetes-v1.8.0 tag in client-go, the code you get is exactly the same as if you check out the v1.8.0 tag in kubernetes, and change directory to staging/src/k8s.io/client-go. The purpose is to let users quickly find matching commits among published repos, like sample-apiserver, apiextension-apiserver, etc. The Kubernetes version tag does NOT claim any backwards compatibility guarantees for client-go. Please check the semantic versions if you care about backwards compatibility.

Only the link to the semver tags is missing in that paragraph.


| client-go | [apimachinery](https://github.com/kubernetes/apimachinery) | [apiextensions-apiserver](https://github.com/kubernetes/apiextensions-apiserver)<sup>*</sup> | [code-generator](https://github.com/kubernetes/code-generator)<sup>+</sup> |
| ----------------------------------| -----------------------------------------------------------|----------------------------------------------------------------------------------------------|----------------------------------------------------------------------------|
| tag: v5.0.x | tag: kubernetes-1.8.x | tag: kubernetes-1.8.x | tag: kubernetes-1.8.x |
Copy link
Member

Choose a reason for hiding this comment

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

I have mixed feelings about making client-go v5.0.2 if kubernetes-1.8.2 and kuberentes-1.8.1 don't have any diff regarding the client. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would create it just to simplify the versioning, even if just saying that nothing changed.

Copy link
Contributor

@sttts sttts Dec 12, 2017

Choose a reason for hiding this comment

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

At the very least I do not want to see v5.0.2 against kubernetes-1.8.4 because nothing changed. These versions must stay in sync for simplicity.


Using the git tag versions is the preferred way of managing dependencies for newer
releases of client-go, and since client-go is backward compatible, it is recommended that
developers always use the latest version.
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph doesn't seem to be necessary? The compatibility matrix states what version of client-go a user needs.

releases of client-go, and since client-go is backward compatible, it is recommended that
developers always use the latest version.

For releases <= v4.0.x of client-go (kubernetes-1.7.x), it is required to use one of these
Copy link
Member

Choose a reason for hiding this comment

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

These three paragraphs seem to duplicate the https://github.com/kubernetes/client-go/blob/master/INSTALL.md. If you take my earlier suggestion regarding splitting the form into libraries that depend on client-go and client-go's dependencies, we don't need these paragraphs.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. INSTALL.md has much more info already than I thought. What does it mean? Is it too hard to find?

Copy link
Author

Choose a reason for hiding this comment

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

So, I have to admit - I totally missed that there was a INSTALL.md and the links to it in the page.

Maybe a dependency matrix is overkill given the INSTALL.md - having at least a list of the potential dependent libraries (apimachinery, apiextensions-apiserver, code-generator) somewhere for discoverability is useful - maybe just add the extra links to the Kubernetes tags section. WDYT? If so, I may just close this PR, and start a fresh one.

Or should there be a more expansive list - something like (with links):

  • apimachinery - core types and machinery for the Kubernetes api
  • apiextensions-apiserver - client for extensions, such as Custom Resource Definitions
  • code-generator - the code generator for Custom Resource Definitions

That could make it easier to know what pieces to look for where. I think this maybe a nice addition. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

A list below "What's included" would certainly fit.

Client-go specifies non-optional libraries in its Godeps/Godeps.json file. While
[Glide](https://glide.sh) will respect these automatically, with
[godep](https://github.com/tools/godep) you have to manually `godep restore`
them from client-go's directory and then `godep save` them in your project.
Copy link
Member

Choose a reason for hiding this comment

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

Why is a godep save necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same we say in INSTALL.md. Of course, one can work in package in the GOPATH alone, then no godep save is necessary. But that's not the pattern people use nowadays in Golang.

@sttts sttts removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2017
@markmandel
Copy link
Author

Closing pull request, as many of my concerns are solved elsewhere.

@markmandel markmandel closed this Feb 14, 2018
@markmandel markmandel deleted the dependency-matrix branch February 14, 2018 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants