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

refactor liklus scaler #6433

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactor liklus scaler #6433

wants to merge 3 commits into from

Conversation

omerap12
Copy link
Contributor

@omerap12 omerap12 commented Dec 20, 2024 •
edited by wozniakjan
Loading

refactor liiklus scaler metadata parsing

Checklist

Relates to #5797

@omerap12 omerap12 requested a review from a team as a code owner December 20, 2024 09:00
@omerap12
refactor liklus scaler
56f8d4a 
Signed-off-by: Omer Aplatony <[email protected]>
@omerap12 omerap12 force-pushed the refactor_liklus_scaler branch from cfe52d7 to 56f8d4a Compare December 20, 2024 11:11
wozniakjan
wozniakjan reviewed Dec 20, 2024
View reviewed changes
Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

great work! just a few minor details below

pkg/scalers/liiklus_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/liiklus_scaler_test.go Outdated Show resolved Hide resolved
pkg/scalers/liiklus_scaler.go Show resolved Hide resolved
@omerap12
Fix unit test for scaling
4934fd0 
Signed-off-by: Omer Aplatony <[email protected]>
JorTurFer
JorTurFer reviewed Dec 22, 2024
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Are these const still required?

const (
	defaultLiiklusLagThreshold           int64 = 10
	defaultLiiklusActivationLagThreshold int64 = 0
)
const (
	liiklusLagThresholdMetricName           = "lagThreshold"
	liiklusActivationLagThresholdMetricName = "activationLagThreshold"
)

@omerap12
Copy link
Contributor Author

Are these const still required?

const (
	defaultLiiklusLagThreshold           int64 = 10
	defaultLiiklusActivationLagThreshold int64 = 0
)
const (
	liiklusLagThresholdMetricName           = "lagThreshold"
	liiklusActivationLagThresholdMetricName = "activationLagThreshold"
)

I only use them in _test.go, so I believe they can be removed.

@omerap12
Removed unused constants
9924ef1 
Signed-off-by: Omer Aplatony <[email protected]>
@omerap12
Copy link
Contributor Author

omerap12 commented Jan 2, 2025

friendly ping @JorTurFer @wozniakjan :)

@wozniakjan
Copy link
Member

/skip-e2e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer left review comments

@wozniakjan wozniakjan wozniakjan approved these changes

@zroubalik zroubalik zroubalik approved these changes

Assignees
No one assigned
Labels
skip-e2e
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@omerap12 @wozniakjan @zroubalik @JorTurFer @keda-automation