-
Notifications
You must be signed in to change notification settings - Fork 263
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 changes for --scale-init support #990
Conversation
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.
@hemanrnjn: 0 warnings.
In response to this:
Description
Changes
- Add support for --scale-init option
Reference
Fixes #959
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.
Hi @hemanrnjn. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
@rhuss I'm adding test cases so till then I'll put this PR in WIP. |
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.
@hemanrnjn thanks for the contribution. Are you ready for some feedback or would you rather we wait for when you remove the WIP tag?
Sure. Why not! I'd also implement those feedback while the PR is in WIP. |
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.
Left some initial. Happy to do another round when addressed and more tests added (unit and e2e).
Thanks for contribution again.
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 a lot for this PR ! Looks good to me, with some minor suggestions (see below)
@rhuss : IIUC, yes, this would scale the number of replicas to match given init-scale and mark the revision ready, and then it'll ignore this value (for eg in no-traffic scale to zero case). This property applies to per revision, so setting this for to-be-created revision in an update should be a valid use case. |
/retest |
4 similar comments
/retest |
/retest |
/retest |
/retest |
@navidshaikh and @rhuss I have made all the changes and added all test cases. Please take one final look at it, if they look fine to you guys too? |
The following is the coverage report on the affected files.
|
/retest |
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
/approve
thanks @hemanrnjn ! The changelog entry is missing but you can add that in a separate PR.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hemanrnjn, navidshaikh 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 |
* Add changes for --scale-init support * Added test cases * Add test case in e2e tests * minor fix * Test case failure fix * Incorporated review comments * Add service update test cases * Incorporate review comments
Description
Now that we have support for defining the initial scale for a service, implementing
--scale-init
with kn as a top-level option instead of just as an annotation.Changes
Reference
Fixes #959