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

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

Merged

Conversation

dttung2905
Copy link
Contributor

@dttung2905 dttung2905 commented Nov 30, 2023

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
Copy link

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

pkg/scalers/azure_log_analytics_scaler.go Outdated Show resolved Hide resolved
@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

@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

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

@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

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

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?

pkg/scalers/scaler.go Outdated Show resolved Hide resolved
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

@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

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

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

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

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

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
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
18 of 19 checks passed
@dttung2905
Copy link
Contributor Author

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

toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
)

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

Successfully merging this pull request may close these issues.

3 participants