Skip to content

Create a new common method to parse parameter from config #5228

Merged
zroubalik merged 5 commits intokedacore:mainfrom
dttung2905:refactor-get-param-from-config-util
Dec 22, 2023
Merged

Create a new common method to parse parameter from config #5228
zroubalik merged 5 commits intokedacore:mainfrom
dttung2905:refactor-get-param-from-config-util

Conversation

@dttung2905
Copy link
Contributor

@dttung2905 dttung2905 commented Nov 30, 2023 •
edited
Loading

Hi team,
This is the first steps in a two-step refactor process. We will first create a new util function to parse parameter from config . Then we move each scalers to use the new method

Assumption:

  • The input is always in string, hence the convertStringToType takes in 'raw' value of pameter as string and convert to whatever types we want
  • if its an optional parameter and we can not find from any config, return an empty string
  • if its an optional parameter and is a required parameters, we return a supplied default value

Checklist

Relates to #5037

@dttung2905 dttung2905 requested a review from a team as a code owner November 30, 2023 00:01
@github-actions
Copy link

github-actions bot commented Nov 30, 2023

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

@tomkerkhove tomkerkhove marked this pull request as draft December 1, 2023 08:44
@dttung2905 dttung2905 force-pushed the refactor-get-param-from-config-util branch from ea51ecd to 27c1c70 Compare December 4, 2023 22:26
@dttung2905 dttung2905 marked this pull request as ready for review December 4, 2023 22:29
@dttung2905 dttung2905 changed the title Draft: Create a new common method to parse parameter from config Create a new common method to parse parameter from config Dec 4, 2023
JorTurFer
JorTurFer reviewed Dec 7, 2023
View reviewed changes
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.

This looks really nice!
Why did you added in aziure_log_analytics_scaler.go file? We should move it to scaler.go as it's something shared, or to utils package inside scalers.

I'd add also a check for detecting if the parameters has been set in multiple places (eg: metadat and auth) to at least raise a warning (I think that error is better but just in case...) WDYT @kedacore/keda-core-contributors ?

@dttung2905 dttung2905 force-pushed the refactor-get-param-from-config-util branch from 27c1c70 to 6e6f16c Compare December 7, 2023 22:30
@dttung2905
Copy link
Contributor Author

dttung2905 commented Dec 7, 2023

@JorTurFer I think scaler.go is a better choice. Initially, I tried to park it under pkg/util/helpers.go but encounter cyclic import. Wdyt ?

@JorTurFer
Copy link
Member

JorTurFer commented Dec 8, 2023

I think scaler.go is a better choice

That's perfect!

Could you add a validation to ensure that the same key isn't read from different sources at same time? cloud that check be an overkill?

@dttung2905
Copy link
Contributor Author

dttung2905 commented Dec 8, 2023

@JorTurFer is it a validation that we are looking at? From my understanding, validation will reject/ fail the new object while what I have in mind is just throwing out warning

@JorTurFer
Copy link
Member

JorTurFer commented Dec 8, 2023

Yeah, I don't have troubles just with a warning at this moment, but I guess that notifying to users somehow that for instance, the key potato is present in trigger.metadata and also in triggerauthentication is useful to prevent issues when both have different values

@dttung2905 dttung2905 force-pushed the refactor-get-param-from-config-util branch from 6e6f16c to 8711f2d Compare December 11, 2023 22:58
@dttung2905
Copy link
Contributor Author

dttung2905 commented Dec 11, 2023

Yeah, I don't have troubles just with a warning at this moment, but I guess that notifying to users somehow that for instance, the key potato is present in trigger.metadata and also in triggerauthentication is useful to prevent issues when both have different values

@JorTurFer I have made some changes that throw errors when users set values in more than 1 place. Could you help to take a look when you have some time?

JorTurFer
JorTurFer reviewed Dec 11, 2023
View reviewed changes
JorTurFer
JorTurFer approved these changes Dec 12, 2023
View reviewed changes
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!
WDYT @zroubalik ?

@JorTurFer
Copy link
Member

JorTurFer commented Dec 12, 2023

@dttung2905 , static checks are failing due some style issue: https://github.com/kedacore/keda/actions/runs/7187276659/job/19574505804?pr=5228

@dttung2905
Copy link
Contributor Author

dttung2905 commented Dec 12, 2023

@JorTurFer thanks for pointing that out. I have fixed it and everything passed

zroubalik
zroubalik reviewed Dec 13, 2023
View reviewed changes
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.

Looking good, what if we go a step further and instead of parsing from string allow parsing from any type to allow things describe here: #2299 and proposed in this staled PR: #2364

@dttung2905 dttung2905 force-pushed the refactor-get-param-from-config-util branch 2 times, most recently from e0ef4df to 93a14bd Compare December 16, 2023 10:58
@dttung2905
Copy link
Contributor Author

dttung2905 commented Dec 16, 2023

@zroubalik could you help to give it another review when you have time?

zroubalik
zroubalik reviewed Dec 21, 2023
View reviewed changes
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.

Looking good, let's resolve the Changelog and merge this

@dttung2905
First commit. Add two new helpers and tests for one method
c98dbcf 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
finish adding test cases
243ac66 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Fix handle default value
b9c4531 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
move util function to scaler.go file
76f5394 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Move changelog to others section
aed7512 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905 dttung2905 force-pushed the refactor-get-param-from-config-util branch from 93a14bd to aed7512 Compare December 21, 2023 21:27
@zroubalik zroubalik merged commit 64e7012 into kedacore:main Dec 22, 2023
@dttung2905
Copy link
Contributor Author

dttung2905 commented Dec 23, 2023

I just realised one of my commit is dropped when I did the last rebase 🤦 . The commit was to address the feedback from @zroubalik #5228 (review). Hence, the current PR still only accept string type as an input instead of interface{} type. Really sorry for that as I did not check thoroughly 😮‍💨

I managed to recover the lost commit. Will author an PR shortly

@dttung2905 dttung2905 mentioned this pull request Dec 23, 2023
Merged
4 tasks
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
@dttung2905 @toniiiik
Create a new common method to parse parameter from config (kedacore#5228
b836554 
)

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: anton.lysina <alysina@gmail.com>
@wozniakjan wozniakjan mentioned this pull request Jan 16, 2024
Closed
4 tasks
@wozniakjan wozniakjan mentioned this pull request Feb 5, 2024
Merged
4 tasks
@wozniakjan wozniakjan mentioned this pull request Apr 10, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zroubalik zroubalik zroubalik approved these changes

@JorTurFer JorTurFer JorTurFer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dttung2905 @JorTurFer @zroubalik