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 client-go 6.0.0 changelog #341

Merged
merged 1 commit into from
Dec 18, 2017
Merged

Conversation

nikhita
Copy link
Member

@nikhita nikhita commented Dec 6, 2017

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2017
@k8s-ci-robot
Copy link
Contributor

@nikhita: GitHub didn't allow me to request PR reviews from the following users: timoreimann, markmandel.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

Ref: https://docs.google.com/spreadsheets/d/1PahRDTfiPbD1DEjtL-3ButW3U-FTKzS8r9NKZQ2kr2c/edit#

/cc @sttts @caesarxuchao @munnerz @timoreimann @markmandel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 6, 2017
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.

Great job @nikhita!

I guess you plan to do the following modifications anyway, but it doesn't hurt if I type too much :)

  • Can we categorize the entries into three categories, "Breaking changes", "New features", "Bug fixes and improvements"?
  • In each category, can we gather the k8s.io/apimachiner and k8s.io/api changes and prefix their entries with the repo name, e.g.,
    • [k8s.io/apimachinery] ObjectCopier interface was removed

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/43016](https://github.com/kubernetes/kubernetes/pull/43016)

* Certificate manager was moved to a shareable location
Copy link
Member

Choose a reason for hiding this comment

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

How about "Certificate manager was moved from kubelet to client-go"?

Copy link
Member

Choose a reason for hiding this comment

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

This is new feature.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/49654](https://github.com/kubernetes/kubernetes/pull/49654)

* `unstructued.Unstructured` got getters and setters
Copy link
Member

Choose a reason for hiding this comment

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

new feature

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/51940](https://github.com/kubernetes/kubernetes/pull/51940)

* Redirect behavior is restored for proxy subresources
Copy link
Member

Choose a reason for hiding this comment

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

Bug fix

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/52933](https://github.com/kubernetes/kubernetes/pull/52933)

* Swagger 1.2 support was removed from kubectl
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change. Swagger 1.2 retriever was removed from the discovery client.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/53441](https://github.com/kubernetes/kubernetes/pull/53441)

* Unset `creationTimestamp` field is output as `null`
Copy link
Member

Choose a reason for hiding this comment

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

Bug fix. Maybe "Unset creationTimestamp field is output as null if encoded from an unstructured object."

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/54748](https://github.com/kubernetes/kubernetes/pull/54748)

* Do not truncate body in glog output with log level 10
Copy link
Member

Choose a reason for hiding this comment

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

Improvement.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/54801](https://github.com/kubernetes/kubernetes/pull/54801)

* `+groupName`tag added to allow unique Go identifiers in clientsets and informers
Copy link
Member

Choose a reason for hiding this comment

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

It seems to not contain any code change. Remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This could be useful for the users since it adds a new tag. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I think that it belongs to the k8s.io/code-generator changelog. Unlike k8s.io/apimachiner or k8s.ioo/api, users of client-go doesn't have to learn code-generator.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/55272](https://github.com/kubernetes/kubernetes/pull/55272)

* The dynamic admission webhook is split into two kinds, mutating and validating.
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change, although the broken API was in alpha.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/55282](https://github.com/kubernetes/kubernetes/pull/55282)

* If you upgrade your client-go libs and use the `AppsV1()` interface, please note that the default garbage collection behavior is changed.
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change. Actually we should say AppsV1() or Apps() interface.

CHANGELOG.md Outdated
* If you upgrade your client-go libs and use the `AppsV1()` interface, please note that the default garbage collection behavior is changed.

* [https://github.com/kubernetes/kubernetes/pull/55148](https://github.com/kubernetes/kubernetes/pull/55148)

Copy link
Member

Choose a reason for hiding this comment

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

Can we add an action-required notice in the changelog saying that clientset's default versioned group interface (like https://github.com/kubernetes/kubernetes/blob/f875ee596d4eb0d5847cf2f0e6b13a80de7d6008/staging/src/k8s.io/client-go/kubernetes/clientset.go#L58-L59) will be removed in next release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should this go as a note for the next release or should it be included for this one? Since the removal will be done for the next one.

Makes sense to have it here as well, but just wanted to double check.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. Adding it as a note for the next release is better.

@markmandel
Copy link

I can't review this (which is fine), but I ❤️ the suggestions @caesarxuchao is putting forward. Will make it much easier for developers to see what is happening!

@timoreimann
Copy link
Contributor

This may be a silly question: what's the purpose of marking where a PR comes from (e.g., apimachinery), especially for the end user reading the changelog?

Moreover, what's the reason actually that we are noting down changes from those other repositories in the client-go repository? Shouldn't they belong in their respective repository change logs?

There's a chance I'm asking all of these questions because I'm not super familiar with the relationship between client-go, apimachinery, api, and possibly other related repositories eventually needed for a Kubernetes client. From a pure FOSS and Github perspective, however, it seems odd to me that the changelog refers to changes which are beyond the scope of the repository hosting the changelog.

@nikhita
Copy link
Member Author

nikhita commented Dec 7, 2017

Thanks for comments, @caesarxuchao! :)

I have addressed most of the comments and left a couple of questions.

CHANGELOG.md Outdated

**Breaking Changes:**

* Swagger 1.2 retriever was removed from the discovery client
Copy link
Contributor

Choose a reason for hiding this comment

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

Swagger 1.2 retriever DiscoveryClient.SwaggerSchema was removed...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@caesarxuchao
Copy link
Member

@timoreimann k8s.io/client-go depends on k8s.io/apimachinery and k8s.io/api, so changes in those two repos affect client-go users. We can create changelogs in those two repos, and add links to them. @sttts @nikhita WDYT?

@sttts
Copy link
Contributor

sttts commented Dec 7, 2017

k8s.io/client-go depends on k8s.io/apimachinery and k8s.io/api, so changes in those two repos affect client-go users. We can create changelogs in those two repos, and add links to them. @sttts @nikhita WDYT?

I would like to avoid complication and keep at least api and apimachinery changes in the client-go release notes. I have no strong opinion about code-generator.

@timoreimann
Copy link
Contributor

@sttts how about leaving a short note then on the relationship between the projects? Basically just what @caesarxuchao said to help users not familiar with the associated repositories.

I could do a quick commit if we agree.

@nikhita
Copy link
Member Author

nikhita commented Dec 7, 2017

How about creating a changelog for code-generator in it's own repo and adding notes there? Since code-generator doesn't affect client-go, I think we can avoid to mention it here.

@caesarxuchao
Copy link
Member

Ok. @nikhita could you add an explanation in the release note on why we include the apimachinery and api entries?

How about creating a changelog for code-generator in it's own repo and adding notes there? Since code-generator doesn't affect client-go, I think we can avoid to mention it here.

Agree.

@nikhita
Copy link
Member Author

nikhita commented Dec 7, 2017

@nikhita could you add an explanation in the release note on why we include the apimachinery and api entries?

Will do.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/54748](https://github.com/kubernetes/kubernetes/pull/54748)

* Informers got a NewFilteredSharedInformerFactory to e.g. filter by namespace
Copy link
Member

Choose a reason for hiding this comment

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

Could you sort the entries in "Breaking Changes", so that the client-go ones go first, then the [k8s.io/api] ones, and then the [k8s.io/apimachinery] ones? Same for the "New features" and "Bugs and Improvements" sections.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

CHANGELOG.md Outdated

* [k8s.io/api] Renamed `core/v1.StorageMediumHugepages` to `StorageMediumHugePages`

* [https://github.com/kubernetes/kubernetes/pull/54748](https://github.com/kubernetes/kubernetes/pull/54748)
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/54660](https://github.com/kubernetes/kubernetes/pull/54660)

* [k8s.io/api] The dynamic admission webhook is split into two kinds, mutating and validating.
Copy link
Contributor

Choose a reason for hiding this comment

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

... For that the kinds have changed completely and old code must be ported to admissionregistration.k8s.io/v1beta1 MutatingWebhookConfiguration and ValidatingWebhookConfiguration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

CHANGELOG.md Outdated

**New Features:**

* Certificate manager was moved from kubelet to client-go
Copy link
Contributor

Choose a reason for hiding this comment

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

... to k8s.io/client-go/util/certificates.

Can we sort this list according to importance? This is just a util package, not really of high prio.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we sort this list according to importance?

This would destroy consistency with the order: client-go, api and apimachinery.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/51940](https://github.com/kubernetes/kubernetes/pull/51940)

* [k8s.io/api] Workloads api types is promoted into new apps/v1 version
Copy link
Contributor

Choose a reason for hiding this comment

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

Workloads api types are promoted to apps/v1

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/53679](https://github.com/kubernetes/kubernetes/pull/53679)

* [k8s.io/apimachinery] Introduces a polymorphic scale client, which supports scaling of resources in arbitrary API groups
Copy link
Contributor

Choose a reason for hiding this comment

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

Added polymorphic scale client in k8s.io/client-go/scale, which ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/54463](https://github.com/kubernetes/kubernetes/pull/54463)

* [k8s.io/api] `core/v1.Taint.TimeAdded` became a pointer
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 a breaking change for the section above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/53720](https://github.com/kubernetes/kubernetes/pull/53720)

* [k8s.io/api] `core/v1.DefaultHardPodAffinitySymmetricWeight` type changed from int to int32
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 a breaking change technically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved.

CHANGELOG.md Outdated

* [https://github.com/kubernetes/kubernetes/pull/54950](https://github.com/kubernetes/kubernetes/pull/54950)

**Bug fixes and Improvements:**
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop all changes but the one about DefaultHardPodAffinitySymmetricWeight. IMO we should only list really important bug fixes. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO we should only list really important bug fixes. What do you think?

👍

With DefaultHardPodAffinitySymmetricWeight moved to breaking change, we would have no bug fixes and improvements.

@nikhita
Copy link
Member Author

nikhita commented Dec 12, 2017

@nikhita could you add an explanation in the release note on why we include the apimachinery and api entries?

Done.

@nikhita
Copy link
Member Author

nikhita commented Dec 12, 2017

How about creating a changelog for code-generator in it's own repo and adding notes there? Since code-generator doesn't affect client-go, I think we can avoid to mention it here.

@sttts Which version of code-generator would this be?

@munnerz
Copy link
Member

munnerz commented Dec 12, 2017

I'm not 100% convinced that all the release notes should end out in a single repo (client-go). Perhaps we can aggregate into client-go, but I think the sub-repos should also contain things specifically related to them in some way. If not, then simply in k/k might make most sense.

That said, having them all in one place is important as most people when they first start consuming these libs won't have the conceptual overview of how these projects relate, so definitely having a single aggregated changelog somewhere makes sense.

This really does relate to the discussions about whether client-go should include all these extra tools that it currently does to be honest. It's a mixture of generated and non generated code, which is a bit confusing. In my mind, client-go should really just be a product of the generators in code-generator, and we'd have some separate client-go-tools or the like. But maybe I'm looking at this from a 'purist' standpoint that makes sense once you've gotten familiar with the repos but not so much for an outsider 🤔

@munnerz
Copy link
Member

munnerz commented Dec 12, 2017

(just to clarify, I don't think what I'm saying above should block this PR being merged. It's important that these changes are documented, and right now client-go is where we store a lot of code related to api extensions)

@nikhita
Copy link
Member Author

nikhita commented Dec 12, 2017

I think the sub-repos should also contain things specifically related to them in some way.

Reminds me that apiextensions-apiserver also needs a CHANGELOG then.

@sttts Again, what version naming would we use for apiextensions-apiserver?

@sttts
Copy link
Contributor

sttts commented Dec 13, 2017

Which version of code-generator would this be?

@nikhita we will put it on the kubernetes-1.9.0 tag I would suggest. Wdyt?

@sttts
Copy link
Contributor

sttts commented Dec 13, 2017

Again, what version naming would we use for apiextensions-apiserver?

We could also re-use the kubernetes-1.9.0. I hesitate to introduce other tags as well as this might get more and more confusing with semver in client-go and non-semver elsewhere.

@nikhita
Copy link
Member Author

nikhita commented Dec 13, 2017

we will put it on the kubernetes-1.9.0 tag I would suggest. Wdyt?

Sgtm.

I hesitate to introduce other tags as well as this might get more and more confusing with semver in client-go and non-semver elsewhere.

👍 👍 👍

@nikhita
Copy link
Member Author

nikhita commented Dec 13, 2017

I'll create a CHANGELOG for those repos as well.

@nikhita
Copy link
Member Author

nikhita commented Dec 13, 2017

Any further changes need to be made here?

@nikhita
Copy link
Member Author

nikhita commented Dec 14, 2017

For apiextensions-apiserver: kubernetes/kubernetes#57206.

@sttts sttts changed the title [WIP] Add client-go 6.0.0 changelog Add client-go 6.0.0 changelog Dec 18, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 18, 2017
@sttts
Copy link
Contributor

sttts commented Dec 18, 2017

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2017
@sttts sttts merged commit 6cddcf6 into kubernetes:master Dec 18, 2017
@nikhita nikhita deleted the add-6.0.0-changelog branch December 18, 2017 12:32
@nikhita nikhita mentioned this pull request Dec 19, 2017
@nikhita nikhita mentioned this pull request May 25, 2018
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants