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

Introduce ClusterCloudEventSource #5816

Merged
merged 6 commits into from
Aug 12, 2024
Merged

Introduce ClusterCloudEventSource #5816

merged 6 commits into from
Aug 12, 2024

Conversation

SpiritZhou
Copy link
Contributor

@SpiritZhou SpiritZhou commented May 21, 2024 •
edited by tomkerkhove
Loading

Introduce ClusterCloudEventSource

Checklist

Relates to #3533

@SpiritZhou SpiritZhou requested a review from a team as a code owner May 21, 2024 06:11
tomkerkhove
tomkerkhove reviewed May 27, 2024
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Did a quick skim and think it looks OK but not a go expert. @JorTurFer Can you share your wisdom?

CHANGELOG.md Show resolved Hide resolved
tomkerkhove
tomkerkhove reviewed May 27, 2024
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Did a quick skim and think it looks OK but not a go expert. @JorTurFer Can you share your wisdom?

This was referenced May 28, 2024
Merged
Merged
@wozniakjan wozniakjan mentioned this pull request Jun 28, 2024
Closed
1 task
wozniakjan
wozniakjan reviewed Jun 28, 2024
View reviewed changes
Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

I think this is on the right track, but I would like us to try to reduce the code duplication a bit ideally.

I created a sample PR to communicate my ideas better in #5924, most importantly I think we should explore some options to not copy-paste the reconciler. #5924 discusses one potential way how this might be achievable, ptal

controllers/eventing/clustercloudeventsource_controller.go Outdated Show resolved Hide resolved
apis/eventing/v1alpha1/cloudeventsource_types.go Outdated Show resolved Hide resolved
apis/eventing/v1alpha1/cloudeventsource_types.go Outdated Show resolved Hide resolved
cmd/operator/main.go Show resolved Hide resolved
@SpiritZhou
Copy link
Contributor Author

I think this is on the right track, but I would like us to try to reduce the code duplication a bit ideally.

I created a sample PR to communicate my ideas better in #5924, most importantly I think we should explore some options to not copy-paste the reconciler. #5924 discusses one potential way how this might be achievable, ptal

Thanks @wozniakjan, I think triggerauthentication & clustertriggerauthentication can be refactor as well.

@wozniakjan
Copy link
Member

Thanks @wozniakjan, I think triggerauthentication & clustertriggerauthentication can be refactor as well.

That is a good point, I will try to find some time later this month to check for any code duplication candidates for TA and CTA

@SpiritZhou SpiritZhou force-pushed the spiritzhou/seletor branch 2 times, most recently from 370fefe to bdee6c9 Compare July 15, 2024 03:15
@tomkerkhove
Copy link
Member

tomkerkhove commented Jul 15, 2024 •
edited by github-actions bot
Loading

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

SpiritZhou and others added 6 commits August 5, 2024 10:15
@SpiritZhou
Update
1ab68cd 
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou
Update ChangeLog
54ec930 
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou
Update
8457b62 
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou @tomkerkhove
Update CHANGELOG.md
1f7444a 
Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou
Refactor
a3f7122 
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou
Update
50a19f0 
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou
Copy link
Contributor Author

@wozniakjan Could you help to review it again?

wozniakjan
wozniakjan approved these changes Aug 5, 2024
View reviewed changes
Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

lgtm, very well done!

@zroubalik
Copy link
Member

zroubalik commented Aug 7, 2024 •
edited by github-actions bot
Loading

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

@tomkerkhove tomkerkhove merged commit 0f638f8 into kedacore:main Aug 12, 2024
22 checks passed
fira42073 pushed a commit to fira42073/keda that referenced this pull request Aug 25, 2024
@SpiritZhou @tomkerkhove @fira42073
Introduce ClusterCloudEventSource (kedacore#5816)
10ee5bd 
Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Signed-off-by: Fira Curie <fira42073@gmail.com>
JorTurFer pushed a commit to JorTurFer/keda that referenced this pull request Oct 7, 2024
@SpiritZhou @tomkerkhove @JorTurFer
Introduce ClusterCloudEventSource (kedacore#5816)
0edbfd7 
Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomkerkhove tomkerkhove tomkerkhove approved these changes

@wozniakjan wozniakjan wozniakjan approved these changes

Assignees

@wozniakjan wozniakjan

@JorTurFer JorTurFer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SpiritZhou @wozniakjan @tomkerkhove @zroubalik @JorTurFer