-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Support paused-replicas being false #7264
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
base: main
Are you sure you want to change the base?
Support paused-replicas being false #7264
Conversation
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
e40276c to
35acbe9
Compare
|
I understand what you're saying, but I personally don't support the word (boolean) If you have an incident, what value do you use for paused-replicas? Is it always |
Yeah I indicated in the issue that Our extension sets the value if we pause to whatever the user decides. Then unsetting just involves removing the value alltogether. The issue then is that you can end up in a state where you dont see that the app is out of sync due to what I described in the description. |
|
The documentation states the following: I almost think we should reverse this. That way, you can keep setting the paused-replicas, but with the additional annotation, indicate whether it should actually be paused or not. If someone changes that and also the paused itself, I think you'll get what you want. However, this would be a kind of breaking change in behavior for people who both specify that pause should be For now, if I had to choose a solution, I think I'd go with |
I would fully agree with this. We actually built our tool at first assuming it worked the way you are proposing - one annotation determines the fixed value, the other determines if that fixed value should be used or not. This would be ideal and would solve it all but as you say would be a breaking change. As I say, happy to make the change to support empty string instead if that is preferred. Will wait for some feedback. |
|
Yeah, I also prefer empty value over |
JorTurFer
left a comment
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.
nice job! small nitpick inline
|
Could you also create a PR in keda-docs so we can have this documented? 🙂 |
8178195 to
2ceb242
Compare
Signed-off-by: Ben White <[email protected]>
2ceb242 to
a2bf0ae
Compare
|
:( this DCO stuff is killing me 😅 - edit - Claude to the rescue :D |
Signed-off-by: Ben White <[email protected]>
4a19f5e to
f04c14b
Compare
Signed-off-by: Ben White <[email protected]>
@rickbrouwer Not super clear to me where that should be done? Like for an unreleased version as the docs all seem to be version coded. Can you link me to the page I should be editing? |
|
/run-e2e pause |
@rickbrouwer so i should modify the docs for the existing version? That feels off. |
We always have the new unreleased version in the docs, so 2.19. It's still to be released, so that's the version you can edit. Feel free to create a PR there and mention me, and I'll check if it went well and give you some tips and feedback if needed. |
Adds support for the
annotation autoscaling.keda.sh/paused-replicas: ''This is particularly useful Argo users where you want to explicitly include the annotation in your git manifest, so that Argo can track it as a managed field and you can see the diff if someone manually modifies it (we do this a lot!).
Checklist
Fixes #7263