🐛 Fix incorrect handling when removing from the end of the triggers array #1768
Conversation
|
This probably will fail in CI since I need to add stuff to the GHA config to install the envtest binaries for functional tests, but I will do that tomorrow. The fix itself can be cherrypicked if getting the envtest stuff in order ends up being a blocker. |
Thanks a lot for the fix! Add an entry to the changelog as well please. |
| @@ -111,7 +111,8 @@ func (r *ScaledObjectReconciler) updateHPAIfNeeded(logger logr.Logger, scaledObj | |||
| return err | |||
| } | |||
|
|
|||
| if !equality.Semantic.DeepDerivative(hpa.Spec, foundHpa.Spec) { | |||
| // DeepDerivative ignores extra entries in arrays which makes removing the last trigger not update things, so trigger and update any time the metrics count is different. | |||
|
@zroubalik I'm guessing I'll need to split that last commit into it's own PR, merge that, then rebuild this? |
|
This PR needs to be rebased in order to get the new build tools image (the location was changed from |
…dacore#1699. Test is currently failing, as expected. Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
…rray. DeepDerivative considers it fine if the found HPA has extra elements in the array as long as the ones that are there in the computed HPA match the found. In our case, that causes updating the HPA to be skipped if you remove the last trigger (or any number as long as they are at the end). Checking the lengths first is quick and should cover all of these wonky cases. Fixes kedacore#1699. Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
|
Looks like the tests are happy, so we should be all set to write more controller functional tests in the future as needed :) |
Fixes #1699 and includes a test to make sure it doesn't come back.
Checklist