-
Notifications
You must be signed in to change notification settings - Fork 39.7k
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
Default extensions/v1beta1 Deployment's RevisionHistoryLimit to MaxInt32 #66605
Default extensions/v1beta1 Deployment's RevisionHistoryLimit to MaxInt32 #66605
Conversation
/assign @janetkuo |
35eec44
to
57cb575
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.
Thanks for the PR. Most parts of it look like a copy of #66581. Please follow the updated changes made in that PR.
You'll also need to regenerate API docs. Please also update unit test for cleanupDeployment in sync_test.go
57cb575
to
64b8268
Compare
64b8268
to
c582f3d
Compare
c582f3d
to
c4c6a10
Compare
c4c6a10
to
cc673a8
Compare
cc673a8
to
f1f5b67
Compare
1c4b63e
to
201a3ec
Compare
/retest |
@janetkuo Thanks! I've updated it accordingly. |
@@ -393,6 +394,16 @@ func TestDeploymentController_cleanupDeployment(t *testing.T) { | |||
revisionHistoryLimit: 0, | |||
expectedDeletions: 0, | |||
}, | |||
{ | |||
// with default revisionHistoryLimit |
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.
nit: change "default" to "unlimited", because "default values" can be different in different versions of Deployment API.
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.
@janetkuo Addressed. Thanks!
One last comment; LGTM otherwise. |
201a3ec
to
490d1b3
Compare
490d1b3
to
84ff6c3
Compare
/retest |
/test pull-kubernetes-e2e-kops-aws |
1 similar comment
/test pull-kubernetes-e2e-kops-aws |
/lgtm |
/test pull-kubernetes-e2e-kops-aws |
/assign @smarterclayton for approval |
@janetkuo: GitHub didn't allow me to assign the following users: for, approval. Note that only kubernetes members and repo collaborators can be assigned. In response to this:
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. |
/test all Tests are more than 96 hours old. Re-running tests. |
/test pull-kubernetes-verify |
/assign @thockin |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: islinwb, janetkuo, thockin 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 |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):#66582: fix extensions/v1beta1 Deployment .spec.revisionHistoryLimit
Special notes for your reviewer:
Release note: