[kn-admin]Add support to enable or disable scale-to-zero #26
Conversation
knative-prow-robot
added
approved
zhanggbj
changed the title
There was a problem hiding this comment.
Looks good for me, but in order to provide a smooth UX we should stick to the convention to use --no-autoscale-to-zero and --scale-to-zero for the true/false values.
There is some specific handling for this in the client, and I wonder whether we should move our 'special' handling of option into a reusable library (like also how to deal with maps + arrays)
|
Thanks for your code review! @rhuss Thanks for point it out. I'll refine this PR to follow the same UX with cli to use @maximilien Same with above will refine it^^ for |
knative-prow-robot
added
size/XXL
zhanggbj
force-pushed
the
scale-to-zero
branch
2 times, most recently
from
e4575da to
88500a5
Compare
|
All comments addressed, it's ready for review, thanks! |
| k8s.io/api v0.17.4 | ||
| k8s.io/apimachinery v0.17.4 | ||
| k8s.io/client-go v0.17.4 | ||
| knative.dev/client v0.14.0 |
There was a problem hiding this comment.
why do you need a dependency on knative.dev/client ?
There was a problem hiding this comment.
@rhuss
I noticed there's a method in knative.dev/client to create flags like --scale-to-zero and --no-scale-to-zero, so reused it.
flags.AddBothBoolFlagsUnhidden
There was a problem hiding this comment.
I think this is ok for now but for the long run we should extract the parts that can be reused by plugins within the client into at least a dedicated package. Because the client coders will not be aware that this method is used by anybody outside the client itself, so its very likely that this can be changed by a refactoring or by accident.
|
Hi @rhuss @maximilien, All comments addressed, would you please help to review again, thanks! |
|
will wait for #14 to add more UT and e2e test |
knative-prow-robot
added
the
needs-rebase
knative-prow-robot
removed
the
needs-rebase
- Init autoscaling subcommand - Support scale-to-zero config - Update examples in README
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" |
There was a problem hiding this comment.
Format Go code:
| "fmt" | |
| "fmt" | |
|
|
||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Format Go code:
|
As @rhuss have approved it and I filled up UT just now, @rhuss @maximilien would you please help to take a review, thanks! |
|
@zhanggbj: you cannot LGTM your own PR. 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. |
|
Hi @rhuss @maximilien , Would you please help to take a review, the last update is to fill up UT, thank you! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhuss, zhanggbj 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 |
User Story: As a Knative administrator, I want to enable and disable scale to zero for Knative platform.
It might be desirable for Knative admins to enable or disable scale-to-zero for a specific Knative installation frequently, which is also discussed in #18. The kn-admin plugin will enable/disable this feature via the CLI instead of modifying ConfigMap. We can use this PR for further discussion on it. Thanks!
Note: please merge PR #25 first and then I'll rebase this one on it.
PR: