-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 KEP-2227 for default container used by kubectl #2189
add KEP-2227 for default container used by kubectl #2189
Conversation
798f0b7
to
27cc286
Compare
a814f25
to
55154dd
Compare
@dougsland I have removed the initial proposal. The code kubernetes/kubernetes#97099 is updated as well. |
55154dd
to
74bb01e
Compare
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.
Few comments and a question is this updated with the latest KEP template (Example: I see missing Drawback)?
https://github.com/kubernetes/enhancements/blob/master/keps/NNNN-kep-template/README.md
not sure if you opened an issue in kep kubernetes/enhancements/issues but looks like it's required for tracking. |
f461cbb
to
6961479
Compare
@deads2k @johnbelamaric @wojtek-t |
I try to follow the new KEP template and add #2227 for tracking. |
/assign @johnbelamaric @seans3 |
more people need to review this one. Please squash the commits. |
c1b1c2a
to
8d145fd
Compare
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.
A few nits, but this looks good overall.
@soltysh @rikatz Thanks for your review. Updated per your comments.
|
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.
@pacoxu some small nits, overall lgtm :)
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.
/approve
@pacoxu get the last bits fixed and pls squash it all into a sigle commit.
/assign |
some PRR nits, the PRR looks ok otherwise. |
PRR looks good except you can fix David's nits. I won't approve yet though because it will approve everything; I will wait on SIG approval before putting in my approve for PRR. |
@johnbelamaric there's already approval from @soltysh :) |
56567b3
to
f92b25c
Compare
Thanks for your review. @deads2k updated per your comments. Add questions and simple answers for Dependencies/Scalability. |
/approve |
@@ -0,0 +1,3 @@ | |||
kep-number: 2227 | |||
alpha: | |||
approver: "@johnbelamaric" |
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.
if you get to it before your PR is lgtm'd by sig-cli, this ought to be @deads2k
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.
i'll wait with tagging this for that having fixed.
@@ -0,0 +1,3 @@ | |||
kep-number: 2227 | |||
alpha: | |||
approver: "@johnbelamaric" |
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.
i'll wait with tagging this for that having fixed.
Co-authored-by: rikatz <ricardo.katz@gmail.com> Co-authored-by: wojtekt <wojtekt@google.com> sd
f92b25c
to
0ae8e51
Compare
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, pacoxu, soltysh 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 |
#2227 is the issue to track this feature.
I opened kubernetes/kubernetes#97099
Fixes kubernetes/kubernetes#96986, a similar solution like kubernetes/kubernetes#87809
There is a discussion in #95293 which is in process for generic default container.
@mfojtik & @howardjohn
@dougsland @eddiezane @soltysh