-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Update values.yaml defaults to match real default values #3777
Conversation
Welcome @Niksko! |
Hi, thanks for the PR! As you mention, I think it's worth linking out to the CA's FAQ for the flags and their default values for any other flags that aren't already listed in the values file to minimise us having to keep the values file up to date. |
/assign |
Updated with a link to the FAQ section |
/retest |
@Niksko: Cannot trigger testing until a trusted user reviews the PR and leaves an 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, can somebody please re-run the integration tests for me. They seem to have failed due to Docker rate limiting:
|
/ok-to-test |
/retest |
/lgtm I can't re-trigger the Travis build, however, you're not touching any of the code that would affect the Travis build so I'm happy enough to merge as is. I'll see if I can get someone who can retrigger it to do so. /approve |
@Niksko The other option we have is you retriggering a Travis build by effectively pushing a new event to trigger it. Can you do a squash of your commits and force push that up? That should be enough to trigger a rebuild. |
Conventionally, commented out values in the values.yaml file should match the default values for the commented out parameters. Some of the parameters here didn't match, so I've corrected them, as per the documented defaults in [the FAQ](https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/FAQ.md). It's likely worth discussing whether this section should either be supplemented to include all of the defaults, or whether it should just link to the documentation of the flags in the FAQ. However for now, the defaults listed match their real default values. Bump version Add note documenting where to find full list of CA parameters This adds a note that sends users to the full list of cluster autoscaler parameters in the readme. This is preferable to adding the full list with defaults here, because then we'd have to keep both places in sync with each other. Update README.md to match values.yaml comments
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gjtempleton, Niksko 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 |
Conventionally, commented out values in the values.yaml file should match the default values for the commented out parameters. Some of the parameters here didn't match, so I've corrected them, as per the documented defaults in the FAQ.
It's likely worth discussing whether this section should either be supplemented to include all of the defaults, or whether it should just link to the documentation of the flags in the FAQ. However for now, the defaults listed match their real default values.