-
Notifications
You must be signed in to change notification settings - Fork 71
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
feat(vertical-pod-autoscaler): Support recommender only deployment #853
Conversation
eff4fea
to
62e903c
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 @agershman. This makes sense but I've added some minor syntactical change requests. Could you also update the README and add a new CHANGELOG entry.
charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml
Outdated
Show resolved
Hide resolved
charts/vertical-pod-autoscaler/templates/admission-controller/pdb.yaml
Outdated
Show resolved
Hide resolved
charts/vertical-pod-autoscaler/templates/admission-controller/serviceaccount.yaml
Outdated
Show resolved
Hide resolved
charts/vertical-pod-autoscaler/templates/admission-controller/servicemonitor.yaml
Outdated
Show resolved
Hide resolved
charts/vertical-pod-autoscaler/templates/updater/serviceaccount.yaml
Outdated
Show resolved
Hide resolved
charts/vertical-pod-autoscaler/templates/updater/servicemonitor.yaml
Outdated
Show resolved
Hide resolved
Sure thing. I'll get those addressed and amend. Thank you! |
8d00ef9
to
d0c4c2a
Compare
d0c4c2a
to
31aeab4
Compare
28e34f5
to
542b03d
Compare
Should be all set on these changes. Let me know if you want any further mods. Thanks 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.
Just a minor change required.
542b03d
to
d3d039c
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.
@agershman if you can remove the version line from CHANGELOG we can run the tests and hopefully get this merged.
Supports a mode of deployment where only the recommender is deployed and the updater and admission controller and not. This provides for a cleaner deployment as opposed to the current state where you can only scale these deployments down to 0. E.g. In the case that alerting has been setup, special casing isn't needed to account for a deployment that's meant to have replicas set at 0. Signed-off-by: Andrew Gershman <[email protected]>
d3d039c
to
7e1a8a9
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
Supports a mode of deployment where only the recommender is deployed and the updater and admission controller and not. This provides for a cleaner deployment as opposed to the current state where you can only scale these deployments down to 0. E.g. In the case that alerting has been setup, special casing isn't needed to account for a deployment that's meant to have replicas set at 0.