Refactor redis scaler config by SpiritZhou · Pull Request #5997 · kedacore/keda

SpiritZhou · 2024-07-30T03:36:41Z

Add multi name support in config type
Refactor redis scaler config

Provide a description of what has been changed

Checklist

When introducing a new scaler, I agree with the scaling governance policy
I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
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)
Commits are signed with Developer Certificate of Origin (DCO - learn more)

Relates to # #5797

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

SpiritZhou · 2024-07-30T03:37:53Z

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

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

tomkerkhove · 2024-08-13T11:36:12Z

@wozniakjan @dttung2905 mind reviewing?

dttung2905

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

pkg/scalers/redis_scaler.go

dttung2905 · 2024-08-13T20:55:26Z

pkg/scalers/redis_scaler.go

+	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"`


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 ? 🤔

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?

pkg/scalers/redis_scaler.go

wozniakjan

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

pkg/scalers/redis_streams_scaler.go

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

rickbrouwer · 2024-08-20T07:25:34Z

@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 · 2024-08-20T07:49:53Z

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 · 2024-08-20T08:49:24Z

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

zroubalik · 2024-08-20T13:55:57Z

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

SpiritZhou · 2024-09-24T02:02:41Z

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

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

SpiritZhou · 2024-09-24T03:30:12Z

@zroubalik Could you help to review it again?

SpiritZhou · 2024-09-24T05:52:56Z

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

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com> Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com> Signed-off-by: michael pechner <mike.pechner@akasa.com>

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com> Signed-off-by: Chirag Bhatia <chiragbhatia8@gmail.com>

Update

6ad9cd9

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

Fix

fc666bd

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

SpiritZhou marked this pull request as ready for review July 30, 2024 07:33

SpiritZhou requested a review from a team as a code owner July 30, 2024 07:33

SpiritZhou added 2 commits August 13, 2024 14:12

Merge branch 'main' into spiritzhou/redisrefactor

56a27db

Update

fdfebfa

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

dttung2905 reviewed Aug 13, 2024

View reviewed changes

wozniakjan approved these changes Aug 14, 2024

View reviewed changes

pkg/scalers/redis_streams_scaler.go Outdated Show resolved Hide resolved

SpiritZhou added 2 commits August 19, 2024 13:15

Update

c73441c

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

Update

3afc165

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

rickbrouwer mentioned this pull request Sep 23, 2024

Support multiple queues at the IBMMQ scaler #6181

Closed

Merge branch 'main' into spiritzhou/redisrefactor

51d7498

Update

3ee3de4

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

zroubalik approved these changes Sep 26, 2024

View reviewed changes

zroubalik merged commit 0b470a6 into kedacore:main Sep 26, 2024

wozniakjan mentioned this pull request Sep 27, 2024

scaler typed config: simplify missing parameter error #6194

Merged

2 tasks

rickbrouwer pushed a commit to rickbrouwer/keda that referenced this pull request Oct 2, 2024

Refactor redis scaler config (kedacore#5997)

e678c8c

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

rickbrouwer pushed a commit to rickbrouwer/keda that referenced this pull request Oct 2, 2024

Refactor redis scaler config (kedacore#5997)

5de36fd

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

JorTurFer pushed a commit to JorTurFer/keda that referenced this pull request Oct 7, 2024

Refactor redis scaler config (kedacore#5997)

e7b736e

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com> Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

wozniakjan mentioned this pull request Oct 21, 2024

Refactor Scalers with new declarative scaler config #5797

Open

58 tasks

rickbrouwer pushed a commit to rickbrouwer/keda that referenced this pull request Oct 21, 2024

Refactor redis scaler config (kedacore#5997)

cddc9ce

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>

mpechner-akasa pushed a commit to nrichardson-akasa/keda that referenced this pull request Nov 29, 2024

Refactor redis scaler config (kedacore#5997)

6c7432a

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com> Signed-off-by: michael pechner <mike.pechner@akasa.com>

chiragbhatia8 pushed a commit to chiragbhatia8/keda that referenced this pull request Jan 9, 2025

Refactor redis scaler config (kedacore#5997)

51fc150

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com> Signed-off-by: Chirag Bhatia <chiragbhatia8@gmail.com>

rickbrouwer mentioned this pull request Mar 10, 2025

JetStream: add accountID param for when an account's name and id differ #6611

Merged

7 tasks

Conversation

SpiritZhou commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

SpiritZhou commented Jul 30, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomkerkhove commented Aug 13, 2024

Uh oh!

dttung2905 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dttung2905 Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

wozniakjan Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wozniakjan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rickbrouwer commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wozniakjan commented Aug 20, 2024

Uh oh!

rickbrouwer commented Aug 20, 2024

Uh oh!

zroubalik commented Aug 20, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SpiritZhou commented Sep 24, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SpiritZhou commented Sep 24, 2024

Uh oh!

SpiritZhou commented Sep 24, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SpiritZhou commented Jul 30, 2024 •

edited

Loading

SpiritZhou commented Jul 30, 2024 •

edited by github-actions bot

Loading

wozniakjan left a comment •

edited

Loading

rickbrouwer commented Aug 20, 2024 •

edited

Loading

zroubalik commented Aug 20, 2024 •

edited by github-actions bot

Loading

SpiritZhou commented Sep 24, 2024 •

edited by github-actions bot

Loading

SpiritZhou commented Sep 24, 2024 •

edited by github-actions bot

Loading