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

Make improvement to getParameterFromConfigV2 #5391

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

dttung2905
Copy link
Contributor

@dttung2905 dttung2905 commented Jan 16, 2024

From the slack discussion, we should make some improvements to the getParameterFromConfigV2 util function to make it more readable.
I create a new PR instead of adding on to #5319 as I want to make this change modular, just showcasing the alternative without refactoring hundreds of lines on the other PR.
How we use the function would be something like

 getParameterFromConfigV2(
			&scalersconfig.ScalerConfig{TriggerMetadata: testData.metadata, AuthParams: testData.authParams, ResolvedEnv: testData.resolvedEnv},
			testData.parameter,
			testData.targetType,
			UseMetadata(testData.useMetadata),
			UseAuthentication(testData.useAuthentication),
			UseResolvedEnv(testData.useResolvedEnv),
			IsOptional(testData.isOptional),
			WithDefaultVal(testData.defaultVal),
		)

Checklist

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.

this is looking promising, I wouldn't necessarily make every argument to getParameterFromConfigV2 an option because some are actually required, otherwise the invocation of the function doesn't make much sense.

overall pretty good!

pkg/scalers/scaler.go Outdated Show resolved Hide resolved
pkg/scalers/scaler.go Outdated Show resolved Hide resolved
@dttung2905 dttung2905 force-pushed the improve-getparam-from-config-v2 branch from ca6b29a to 4f7495c Compare January 28, 2024 22:16
@dttung2905 dttung2905 force-pushed the improve-getparam-from-config-v2 branch from cb1ed61 to 49571cc Compare January 29, 2024 20:59
CHANGELOG.md Outdated Show resolved Hide resolved
@dttung2905 dttung2905 force-pushed the improve-getparam-from-config-v2 branch from e4b884f to 4f0c4af Compare January 30, 2024 13:02
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.

This is looking good, I like the direction.

pkg/scalers/scaler.go Show resolved Hide resolved
pkg/scalers/scaler.go Show resolved Hide resolved
pkg/scalers/scaler.go Show resolved Hide resolved
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
@dttung2905 dttung2905 force-pushed the improve-getparam-from-config-v2 branch from f24d974 to 08c472c Compare February 5, 2024 18:13
Signed-off-by: dttung2905 <[email protected]>
pkg/scalers/scaler.go Show resolved Hide resolved
@zroubalik zroubalik enabled auto-merge (squash) February 6, 2024 14:33
@zroubalik zroubalik merged commit cdbcb9f into kedacore:main Feb 6, 2024
21 of 22 checks passed
ArunYogesh pushed a commit to ArunYogesh/keda that referenced this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants