-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Apply fixes to in place support VPA AEP #5877
Apply fixes to in place support VPA AEP #5877
Conversation
Looks like the first PR merged a bit too early, while there were open coments
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jbartosik The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[in-place update feature](https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources) | ||
[available in Kubernetes 1.27.](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#api-change-3) | ||
[in-place update feature]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources | ||
[available in Kubernetes 1.27.]: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#api-change-3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbartosik can we mention the following link instead of the above link:
https://github.com/kubernetes/kubernetes/pull/102884
because it redirects to the Changelog of k8s v1.27.0-alpha.3, and we have to search the PR for the in-place update feature
.
So I think we can mention the direct link of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog says that the feature is available in 1.27, which is what the doc is talking about in this place
In another place I link to KEP tracking the feature. I'm not sure if it's a good idea to link to the PR - the feature will (hopefully) change and go to beta so I'd rater refer to places that might but up to date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Thanks, @jbartosik for the clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So can we change the link to the
https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#api-change-1
instead of api-change-3 it should api-change-1 because it is for the stable release i.e. k8s v1.27 and the corresponding text is mentioned there and not in the api-change-3 which is for v1.27.0-alpha.3. other than that is it mentioned under v1.27.0-alpha.2. IMO we should go with k8s v1.27, not with the pre-releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbartosik, your thoughts?
/lgtm |
/kind cleanup
/kind documentation
Apply comments from review of #5755 that I couldn't apply directly in the PR before it merged
If there are any other comments please leave them on either PR and I'll address them here.
#4016