-
Notifications
You must be signed in to change notification settings - Fork 25
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 scaling threshold to crd #180
Conversation
Pull Request Test Coverage Report for Build 8361454969Details
💛 - Coveralls |
/ok-to-test sha=c956d0f |
/ok-to-test sha=1e93c26 |
/ok-to-test sha=53d11ad |
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.
Approved from naming/description perspective
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.
few changes requested. I know we’ve got fixture tests so our bases should be covered but is there any feasible and useful way we can test for this in e2e to ensure our output values are correct all the way through to hpa deployment?
/ok-to-test sha=2012fa4 |
not really that useful to check this in e2e. the code for this addition is all pure functions so unit tests do a good job. e2e for this would look like provisioning a crd then checking the backing hpa numbers which is exactly what we do in unit tests. instead these numbers will be validated at a higher level in scale tests along with the full functionality |
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.
Changes look good
also want to note that we do add e2e tests for the one thing that we can't validate in unit tests which is crd validations (the rejection test ensuring that we can only use one of the enums) |
/ok-to-test sha=bcbe73d |
Description
adds scaling threshold to crd to configure how fast hpa scales. The options provided are known good values. We use enums to control the actual values which we scale test internally. We control cpu requests so that is another reason why these are enums.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
locally, e2e, unit
Checklist: