Skip to content

Refactor redis scaler config#5997

Merged
zroubalik merged 8 commits into
kedacore:mainfrom
SpiritZhou:spiritzhou/redisrefactor
Sep 26, 2024
Merged

Refactor redis scaler config#5997
zroubalik merged 8 commits into
kedacore:mainfrom
SpiritZhou:spiritzhou/redisrefactor

Conversation

@SpiritZhou

@SpiritZhou SpiritZhou commented Jul 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor
  1. Add multi name support in config type
  2. Refactor redis scaler config

Provide a description of what has been changed

Checklist

Relates to # #5797

@SpiritZhou
Update
6ad9cd9 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou

SpiritZhou commented Jul 30, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor Author

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

@SpiritZhou
Fix
fc666bd 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou SpiritZhou marked this pull request as ready for review July 30, 2024 07:33
@SpiritZhou SpiritZhou requested a review from a team as a code owner July 30, 2024 07:33
@tomkerkhove

tomkerkhove commented Aug 13, 2024

Copy link
Copy Markdown
Member

@wozniakjan @dttung2905 mind reviewing?

dttung2905
dttung2905 reviewed Aug 13, 2024
View reviewed changes

@dttung2905 dttung2905 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the PR. I just have a few minor comments, mostly on the support of multiple key name in our util function

Comment thread pkg/scalers/redis_scaler.go Outdated
Comment thread pkg/scalers/redis_scaler.go Outdated
Ports []string `keda:"name=port;ports, order=triggerMetadata;resolvedEnv;authParams"`
EnableTLS bool `keda:"name=enableTLS;tls, order=triggerMetadata;authParams, optional, default=false"`
UnsafeSsl bool `keda:"name=unsafeSsl, order=triggerMetadata, optional, default=false"`
Cert string `keda:"name=Cert;cert, order=authParams"`

@dttung2905 dttung2905 Aug 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar question there ? Are you trying to cover all cases where user can mistakenly input a value into the manifest or such cases have been supported in the current code ? 🤔

@wozniakjan wozniakjan Aug 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think multi-name keys is a good feature, although I wasn't able to find Cert (with capital C) anywhere in the original code. Maybe this is a typo?

Comment thread pkg/scalers/redis_scaler.go Outdated
wozniakjan
wozniakjan approved these changes Aug 14, 2024
View reviewed changes

@wozniakjan wozniakjan left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, hopefully multi-named keys won't complicate the API spec generator but we will cross that bridge once we have some tooling foundation.

Comment thread pkg/scalers/redis_streams_scaler.go Outdated
@SpiritZhou
Update
c73441c 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Update
3afc165 
Signed-off-by: SpiritZhou <[email protected]>
@rickbrouwer

rickbrouwer commented Aug 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

@wozniakjan what about the usernameFromEnv and passwordFromEnv? These are mentioned in the documentation as being usable, but are no longer supported via this PR. I also have this problem with the IBM scaler and wanted to see here how it was solved. This issue will also appear by the MySQL scaler. What do you think how to solve this?

@wozniakjan

wozniakjan commented Aug 20, 2024

Copy link
Copy Markdown
Member

what about the usernameFromEnv and passwordFromEnv? These are mentioned in the documentation as being usable, but are no longer supported via this PR.

in this specific PR it should continue to work with usernameFromEnv, unless I'm missing something.

keda/pkg/scalers/redis_scaler.go

Line 50 in 3afc165

Username string `keda:"name=username, order=triggerMetadata;resolvedEnv;authParams"`

this contains order field tag key with value resolvedEnv (among other), and the way it's implemented internally is it appends FromEnv to the name of the parameter and then fetches key from TriggerMetadata map. It works this way because that was the convention in KEDA before #5676 and we didn't want to introduce any breaking changes with this refactor, goal is just to standardize the parsing implementations and reduce code duplication.

keda/pkg/scalers/scalersconfig/typed_config.go

Lines 403 to 405 in 3afc165

case ResolvedEnv:
m = sc.ResolvedEnv
key = sc.TriggerMetadata[fmt.Sprintf("%sFromEnv", key)]

It might be a bit more understandable in this unit test

keda/pkg/scalers/scalersconfig/typed_config_test.go

Lines 27 to 63 in 36cc226

func TestBasicTypedConfig(t *testing.T) {
RegisterTestingT(t)
sc := &ScalerConfig{
TriggerMetadata: map[string]string{
"stringVal": "value1",
"intVal": "1",
"boolValFromEnv": "boolVal",
"floatValFromEnv": "floatVal",
},
ResolvedEnv: map[string]string{
"boolVal": "true",
"floatVal": "1.1",
},
AuthParams: map[string]string{
"auth": "authValue",
},
}
type testStruct struct {
StringVal string `keda:"name=stringVal, order=triggerMetadata"`
IntVal int `keda:"name=intVal, order=triggerMetadata"`
BoolVal bool `keda:"name=boolVal, order=resolvedEnv"`
FloatVal float64 `keda:"name=floatVal, order=resolvedEnv"`
AuthVal string `keda:"name=auth, order=authParams"`
}
ts := testStruct{}
err := sc.TypedConfig(&ts)
Expect(err).To(BeNil())
Expect(ts.StringVal).To(Equal("value1"))
Expect(ts.IntVal).To(Equal(1))
Expect(ts.BoolVal).To(BeTrue())
Expect(ts.FloatVal).To(Equal(1.1))
Expect(ts.AuthVal).To(Equal("authValue"))
}

@rickbrouwer

rickbrouwer commented Aug 20, 2024

Copy link
Copy Markdown
Member

@wozniakjan Thanks for the explanation! Then it should work fine for the rest :)

@zroubalik

zroubalik commented Aug 20, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

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

@rickbrouwer rickbrouwer mentioned this pull request Sep 23, 2024
Closed
@SpiritZhou

SpiritZhou commented Sep 24, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor Author

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

@SpiritZhou
Update
3ee3de4 
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou

SpiritZhou commented Sep 24, 2024

Copy link
Copy Markdown
Contributor Author

@zroubalik Could you help to review it again?

@SpiritZhou

SpiritZhou commented Sep 24, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor Author

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

@zroubalik zroubalik merged commit 0b470a6 into kedacore:main Sep 26, 2024
@wozniakjan wozniakjan mentioned this pull request Sep 27, 2024
Merged
2 tasks
rickbrouwer pushed a commit to rickbrouwer/keda that referenced this pull request Oct 2, 2024
@SpiritZhou @rickbrouwer
Refactor redis scaler config (kedacore#5997)
e678c8c 
Signed-off-by: SpiritZhou <[email protected]>
rickbrouwer pushed a commit to rickbrouwer/keda that referenced this pull request Oct 2, 2024
@SpiritZhou @rickbrouwer
Refactor redis scaler config (kedacore#5997)
5de36fd 
Signed-off-by: SpiritZhou <[email protected]>
JorTurFer pushed a commit to JorTurFer/keda that referenced this pull request Oct 7, 2024
@SpiritZhou @JorTurFer
Refactor redis scaler config (kedacore#5997)
e7b736e 
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
@wozniakjan wozniakjan mentioned this pull request Oct 21, 2024
Closed
58 tasks
rickbrouwer pushed a commit to rickbrouwer/keda that referenced this pull request Oct 21, 2024
@SpiritZhou @rickbrouwer
Refactor redis scaler config (kedacore#5997)
cddc9ce 
Signed-off-by: SpiritZhou <[email protected]>
mpechner-akasa pushed a commit to nrichardson-akasa/keda that referenced this pull request Nov 29, 2024
@SpiritZhou @mpechner-akasa
Refactor redis scaler config (kedacore#5997)
6c7432a 
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: michael pechner <[email protected]>
chiragbhatia8 pushed a commit to chiragbhatia8/keda that referenced this pull request Jan 9, 2025
@SpiritZhou @chiragbhatia8
Refactor redis scaler config (kedacore#5997)
51fc150 
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: Chirag Bhatia <[email protected]>
@rickbrouwer rickbrouwer mentioned this pull request Mar 10, 2025
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wozniakjan wozniakjan wozniakjan approved these changes
@zroubalik zroubalik zroubalik approved these changes
+1 more reviewer
@dttung2905 dttung2905 dttung2905 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@SpiritZhou @tomkerkhove @rickbrouwer @wozniakjan @zroubalik @dttung2905