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.

Sign up for GitHub

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

Jump to bottom

External Scaler: Add tls options in TriggerAuth metadata #4407

Merged
merged 16 commits into from
May 18, 2023
Merged

External Scaler: Add tls options in TriggerAuth metadata #4407

merged 16 commits into from
May 18, 2023

Conversation

dttung2905
Copy link
Contributor

@dttung2905 dttung2905 commented Mar 26, 2023 •
edited by zroubalik
Loading

Hi team,
This PR will fix the issue #3565

Checklist

Relates: #4549

@dttung2905 dttung2905 requested a review from a team as a code owner March 26, 2023 15:41
@JorTurFer
Copy link
Member

Hi @dttung2905
(Not pressure at all) Is this ready to review? I haven't done it yet because I have seen the WIP prefix but maybe it's ready and you are expecting the review

@dttung2905
Copy link
Contributor Author

Hi @JorTurFer , my bad on the misleading title. Yes, please help to give me some early feedback on my PR 😛

@JorTurFer
Copy link
Member

Hi @JorTurFer , my bad on the misleading title. Yes, please help to give me some early feedback on my PR 😛

I'll check it tomorrow morning

JorTurFer
JorTurFer reviewed Apr 4, 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.

Looking good, I have left some inline comments.
Will you add and e2e test to check this feature?

pkg/scalers/external_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/external_scaler.go Outdated Show resolved Hide resolved
@dttung2905 dttung2905 force-pushed the external-scaler-tls branch from 23bf0f7 to f662140 Compare April 22, 2023 06:58
JorTurFer
JorTurFer reviewed Apr 24, 2023
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking good, nice job! Only small nits inline

pkg/scalers/external_scaler.go Outdated Show resolved Hide resolved
pkg/util/tls_config_test.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Apr 24, 2023 •
edited by github-actions bot
Loading

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

@dttung2905 dttung2905 force-pushed the external-scaler-tls branch from cba5f5a to 378fa3b Compare April 25, 2023 14:36
@JorTurFer
Copy link
Member

JorTurFer commented Apr 25, 2023 •
edited by github-actions bot
Loading

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

@dttung2905 dttung2905 force-pushed the external-scaler-tls branch from 2244321 to 28fc2b5 Compare April 30, 2023 10:21
@dttung2905 dttung2905 force-pushed the external-scaler-tls branch from 28fc2b5 to 2cd91d2 Compare May 9, 2023 15:13
@JorTurFer
Copy link
Member

JorTurFer commented May 9, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member

JorTurFer commented May 10, 2023 •
edited by github-actions bot
Loading

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

@dttung2905 dttung2905 force-pushed the external-scaler-tls branch 2 times, most recently from 20beacd to d2de7b6 Compare May 13, 2023 14:02
@JorTurFer
Copy link
Member

JorTurFer commented May 17, 2023 •
edited by github-actions bot
Loading

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

@dttung2905 dttung2905 force-pushed the external-scaler-tls branch from d2de7b6 to 6a1aabe Compare May 17, 2023 10:55
@dttung2905 dttung2905 mentioned this pull request May 17, 2023
Merged
1 task
zroubalik
zroubalik reviewed May 17, 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.

@dttung2905 please let us know when is this ready for a review.

@dttung2905 dttung2905 changed the title WIP: Add tls options in TriggerAuth metadata Add tls options in TriggerAuth metadata May 17, 2023
@dttung2905
Copy link
Contributor Author

@dttung2905 please let us know when is this ready for a review.

Hi @zroubalik , its ready for review 🙏

zroubalik
zroubalik approved these changes May 17, 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.

LGTM, great job, just a minor nit on deprecation issue.

pkg/scalers/external_scaler.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented May 17, 2023 •
edited by github-actions bot
Loading

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

@dttung2905 dttung2905 mentioned this pull request May 18, 2023
Closed
dttung2905 added 3 commits May 18, 2023 19:10
@dttung2905
Add tls options in TriggerAuth metadata
5e92fa3 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Fix semgrep
d1d9564 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Add insecureSkipVerify config in ScaleObject
28f4528 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
dttung2905 added 13 commits May 18, 2023 19:10
@dttung2905
Fix precommit CI check
25b747c 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Refactor code to use tls util helper function
13f32a0 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Add test for the new function in tls package
42af24d 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Fix minor typo and CI failure from code review
1bb11e2 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Fix static check CI failure
fd537d2 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Fix failing unittest
0f18725 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
revert the method to check for CA
83f6570 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Fix semgrep
c6a4268 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
dummy commit to trigger CI build
5f8fa08 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Set ignore semgrep
3e6d4a4 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Add CHANGELOG file
35dcbef 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Change the argument name to
6ffa49d 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Link deprecation comment to an issue
2af0f96 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905 dttung2905 force-pushed the external-scaler-tls branch from 99ce5e4 to 2af0f96 Compare May 18, 2023 11:11
@zroubalik
Copy link
Member

zroubalik commented May 18, 2023 •
edited by github-actions bot
Loading

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

zroubalik
zroubalik approved these changes May 18, 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.

LGTM, great job!

@zroubalik zroubalik changed the title Add tls options in TriggerAuth metadata External Scaler: Add tls options in TriggerAuth metadata May 18, 2023
@zroubalik zroubalik merged commit dcfcddc into kedacore:main May 18, 2023
@zroubalik zroubalik mentioned this pull request May 22, 2023
Closed
@wsugarman wsugarman mentioned this pull request Jul 30, 2023
Closed
@JorTurFer JorTurFer mentioned this pull request Oct 16, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

@JorTurFer JorTurFer JorTurFer left review comments

@zroubalik zroubalik zroubalik 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