Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add InitialCooldownPeriod for ScaledObjects #5478

Merged
merged 13 commits into from
Apr 12, 2024
Merged

add InitialCooldownPeriod for ScaledObjects #5478

merged 13 commits into from
Apr 12, 2024

Conversation

RonaldFletcher
Copy link
Contributor

@RonaldFletcher RonaldFletcher commented Feb 4, 2024 •
edited
Loading

… of CooldownPeriod

·

Add a new field InitialDelayCooldownPeriod The purpose of adding a new field InitialDelayCooldownPeriod is to delay the effective time of the CooldownPeriod, so that when the service is started for the first time, it needs to wait for the time of InitialDelayCooldownPeriod to expire before destroying the service.

Checklist

Fixes #5008

Relates to #

@RonaldFletcher RonaldFletcher requested a review from a team as a code owner February 4, 2024 09:31
JorTurFer
JorTurFer reviewed Feb 9, 2024
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!
Looking good! I've kept some comments inline, but as global comment, could you add some coverage to the change? Maybe an e2e test or maybe unit test

pkg/scaling/executor/scale_executor.go Outdated Show resolved Hide resolved
pkg/scaling/scale_handler.go Outdated Show resolved Hide resolved
@shubham-kaushal
Copy link

Hi, @JorTurFer is this PR getting merged anytime soon? Our current workaround for handling the delay at the time of deployment is causing challenges. This feature will be very helpful for many of our use cases.

@JorTurFer
Copy link
Member

JorTurFer commented Feb 26, 2024 •
edited
Loading

Hi, @JorTurFer is this PR getting merged anytime soon? Our current workaround for handling the delay at the time of deployment is causing challenges. This feature will be very helpful for many of our use cases.

I hope so, but the feature isn't ready to merge yet because test coverage is required. IDK the state of it, @xrwang8 ?

@RonaldFletcher
Copy link
Contributor Author

I've added e2e testing. @JorTurFer

@RonaldFletcher
Copy link
Contributor Author

What's wrong with that now @JorTurFer

@shubham-kaushal
Copy link

Hi @JorTurFer , could you plz. check this PR. E2E test has been added by @xrwang8

@JorTurFer
Copy link
Member

JorTurFer commented Mar 12, 2024 •
edited by github-actions bot
Loading

/run-e2e initial_delay_cooldownperiod
Update: You can check the progress here

@JorTurFer
Copy link
Member

Hi @JorTurFer , could you plz. check this PR. E2E test has been added by @xrwang8

Sorry, I've been quite disconnected :( I have left some comments inline. Apart from them ,we need to fix DCO check for merging the PR, you can find the steps to fix it in the check itself
image

wangxingrui and others added 2 commits March 13, 2024 16:15
add new a filed InitialDelayCooldownPeriod delay the effective time o…
fb1f901 
…f CooldownPeriod and e2e

Signed-off-by: wangxingrui <xrwang8@gamil.com>
test:simplify initial-delay-cooldownperiod e2e test
cecbf7a 
Signed-off-by: wangxingrui <xrwang8@gamil.com>
@RonaldFletcher
Copy link
Contributor Author

I have simplified e2e @JorTurFer

JorTurFer
JorTurFer approved these changes Mar 15, 2024
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks for the improvement!

@JorTurFer
Copy link
Member

JorTurFer commented Mar 15, 2024 •
edited by github-actions bot
Loading

/run-e2e internal
Update: You can check the progress here

@JorTurFer
Copy link
Member

Could you open a PR against docs repo too, adding this new parameter to the ScaledObject docs?

@RonaldFletcher
Copy link
Contributor Author

Could you open a PR against docs repo too, adding this new parameter to the ScaledObject docs?

ok

@RonaldFletcher RonaldFletcher mentioned this pull request Mar 18, 2024
Merged
1 task
@RonaldFletcher
Copy link
Contributor Author

I have modified the document kedacore/keda-docs#1337 @JorTurFer

@JorTurFer
Copy link
Member

I have modified the document kedacore/keda-docs#1337 @JorTurFer

Let me check them, sorry for the delay... last week was KubeCon EU

JorTurFer
JorTurFer approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Could you add a record in changelog.md file too? I think that #New section is the best place for this feature

RonaldFletcher and others added 3 commits March 26, 2024 09:16
@RonaldFletcher
Merge branch 'kedacore:main' into main
998eaed
Add entry to CHANGELOG.md for adding initialDelayCooldownPeriod param…
4a2a8db 
…eter to ScaledObject

Signed-off-by: wangxingrui <xrwang8@gamil.com>
mend
0e8b394 
Signed-off-by: wangxingrui <xrwang8@gamil.com>
@RonaldFletcher
Copy link
Contributor Author

LGTM! Could you add a record in changelog.md file too? I think that #New section is the best place for this feature

i hava already done @JorTurFer

JorTurFer
JorTurFer reviewed Mar 26, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer
Update CHANGELOG.md
5fae394 
Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member

JorTurFer commented Mar 26, 2024 •
edited by github-actions bot
Loading

/run-e2e internal
Update: You can check the progress here

@JorTurFer
Copy link
Member

@zroubalik PTAL

@RonaldFletcher
Copy link
Contributor Author

What is the current status of this PR? @JorTurFer

@JorTurFer
Copy link
Member

What is the current status of this PR?

It's ready to merge IMHO, but I'd like to read @zroubalik thoughts too :)

@aj-ya
Copy link

aj-ya commented Apr 8, 2024

Hey! @zroubalik , could you please take a look at this. This will save a bit of development for us. Thank you.
cc: @JorTurFer

zroubalik
zroubalik reviewed Apr 10, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_types.go Outdated Show resolved Hide resolved
RonaldFletcher and others added 2 commits April 11, 2024 09:00
@RonaldFletcher @zroubalik
Update CHANGELOG.md
97eacb9 
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: xrwang <68765051+xrwang8@users.noreply.github.com>
@RonaldFletcher
refactor: rename InitialDelayCooldownPeriod to InitialCooldownPeriod
52839d5 
Signed-off-by: wangxingrui <xrwang8@gmail.com>
@RonaldFletcher
Copy link
Contributor Author

@zroubalik done

@RonaldFletcher
Merge branch 'main' into main
b49d01d 
Signed-off-by: xrwang <68765051+xrwang8@users.noreply.github.com>
@RonaldFletcher RonaldFletcher changed the title add new a filed InitialDelayCooldownPeriod delay the effective time… add new a filed InitialCooldownPeriod delay the effective time… Apr 11, 2024
@RonaldFletcher
Update CHANGELOG to sort entries according to project requirements
092f868 
Signed-off-by: wangxingrui <xrwang8@gmail.com>
@RonaldFletcher
Change InitialCooldownPeriod from pointer to non-pointer int32
fc58691 
Signed-off-by: wangxingrui <xrwang8@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented Apr 11, 2024 •
edited by github-actions bot
Loading

/run-e2e internals
Update: You can check the progress here

@RonaldFletcher
Copy link
Contributor Author

@JorTurFer can it be merged ?

@zroubalik zroubalik changed the title add new a filed InitialCooldownPeriod delay the effective time… add InitialCooldownPeriod for ScaledObjects Apr 12, 2024
@zroubalik zroubalik merged commit 80806a7 into kedacore:main Apr 12, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer approved these changes

@zroubalik zroubalik zroubalik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@RonaldFletcher @shubham-kaushal @JorTurFer @aj-ya @zroubalik