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.

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

e2e test for memory scaler #2646

Merged
merged 8 commits into from
Feb 26, 2022
Merged

Conversation

Ritikaa96
Copy link
Contributor

@Ritikaa96 Ritikaa96 commented Feb 15, 2022

This PR add E2E tests for memory scaler

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #2220

@Ritikaa96 Ritikaa96 requested a review from a team as a code owner February 15, 2022 19:46
@Ritikaa96
Copy link
Contributor Author

Hi @tomkerkhove , @JorTurFer
PTAL.

@JorTurFer
Copy link
Member

JorTurFer commented Feb 16, 2022

/run-e2e memory.test.ts
Update: You can check the progres here

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.

Hi @Ritikaa96
First, thanks a lot for this ❤️
There are some notation inconsistencies, I have suggested 2 changes but you should review all the places where it happens.

Related with the flow, I'd propose to set less resources to ensure the scaling process (1% instead of 5% for example) and also I'd propose to instead of deleting the first SO and creating another one totally different, patching current SO to update the value. You could achieve it basically creating a variable with the SO template and calling it twice with different parameters to update it.
WDYT?

tests/scalers/memory.test.ts Outdated Show resolved Hide resolved
tests/scalers/memory.test.ts Outdated Show resolved Hide resolved
@JorTurFer JorTurFer requested a review from a team February 16, 2022 11:58
Copy link
Member

@zroubalik zroubalik 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 this, @JorTurFer have some great comments.

CHANGELOG.md Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

zroubalik commented Feb 17, 2022

/run-e2e memory.test.ts
Update: You can check the progres here

@Ritikaa96
Copy link
Contributor Author

Ritikaa96 commented Feb 17, 2022

Hi @Ritikaa96 First, thanks a lot for this heart There are some notation inconsistencies, I have suggested 2 changes but you should review all the places where it happens.

Related with the flow, I'd propose to set less resources to ensure the scaling process (1% instead of 5% for example) and also I'd propose to instead of deleting the first SO and creating another one totally different, patching current SO to update the value. You could achieve it basically creating a variable with the SO template and calling it twice with different parameters to update it. WDYT?

Hi @JorTurFer , Done with the changes, Thanks for the suggestions. PTAL.

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! ❤️
Only 1 small nit about one variable

tests/scalers/memory.test.ts Outdated Show resolved Hide resolved
@Ritikaa96
Copy link
Contributor Author

Hi @JorTurFer , I apologize for the delay , was stuck because of an emergency. I corrected the variable PTAL.

@JorTurFer
Copy link
Member

JorTurFer commented Feb 26, 2022

/run-e2e memory.test.ts
Update: You can check the progres here

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 this, let's wait till e2e passes

@JorTurFer
Copy link
Member

Could you update the changelog @Ritikaa96 ?

@Ritikaa96
Copy link
Contributor Author

Could you update the changelog @Ritikaa96 ?

Sure

@Ritikaa96
Copy link
Contributor Author

Ritikaa96 commented Feb 26, 2022

@JorTurFer @zroubalik PTAL

@JorTurFer JorTurFer merged commit 8b6a001 into kedacore:main Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add e2e test for Memory Scaler
3 participants