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

Add Provider for Google Pub/Sub Topic #543

Merged
merged 1 commit into from
Jun 16, 2023
Merged

Add Provider for Google Pub/Sub Topic #543

merged 1 commit into from
Jun 16, 2023

Conversation

matheuscscp
Copy link
Member

@matheuscscp matheuscscp commented Jun 6, 2023 •
edited
Loading

Fixes #539

@matheuscscp matheuscscp force-pushed the gcloud-pubsub branch 3 times, most recently from 22cdfd8 to de992b5 Compare June 6, 2023 17:47
@matheuscscp matheuscscp marked this pull request as ready for review June 6, 2023 17:51
@matheuscscp matheuscscp force-pushed the gcloud-pubsub branch 3 times, most recently from d8b2e66 to 508e814 Compare June 6, 2023 18:07
darkowlzz
darkowlzz reviewed Jun 7, 2023
View reviewed changes
internal/notifier/google_pubsub_topic.go Outdated Show resolved Hide resolved
internal/server/event_server.go Outdated Show resolved Hide resolved
makkes
makkes reviewed Jun 7, 2023
View reviewed changes
internal/controller/provider_controller.go Outdated Show resolved Hide resolved
internal/controller/provider_controller.go Outdated Show resolved Hide resolved
internal/notifier/google_pubsub_topic_test.go Outdated Show resolved Hide resolved
internal/notifier/google_pubsub_topic_test.go Outdated Show resolved Hide resolved
internal/notifier/google_pubsub_topic_test.go Outdated Show resolved Hide resolved
@matheuscscp matheuscscp force-pushed the gcloud-pubsub branch 7 times, most recently from 03cb325 to 5801ba9 Compare June 7, 2023 13:44
@matheuscscp matheuscscp force-pushed the gcloud-pubsub branch 5 times, most recently from 6300d88 to 5148b5c Compare June 8, 2023 00:57
@matheuscscp matheuscscp force-pushed the gcloud-pubsub branch 4 times, most recently from 9c377df to 83f6109 Compare June 8, 2023 13:35
@matheuscscp matheuscscp mentioned this pull request Jun 8, 2023
Merged
@matheuscscp matheuscscp force-pushed the gcloud-pubsub branch 3 times, most recently from 81428fc to 72549b8 Compare June 13, 2023 08:06
darkowlzz
darkowlzz reviewed Jun 14, 2023
View reviewed changes
docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
internal/notifier/google_pubsub_topic.go Outdated Show resolved Hide resolved
internal/notifier/google_pubsub_topic.go Outdated Show resolved Hide resolved
internal/notifier/google_pubsub_topic_test.go Outdated Show resolved Hide resolved
internal/notifier/google_pubsub_topic.go Outdated Show resolved Hide resolved
internal/notifier/google_pubsub_topic.go Outdated Show resolved Hide resolved
@darkowlzz darkowlzz added the area/alerting Alerting related issues and PRs label Jun 14, 2023
@matheuscscp matheuscscp force-pushed the gcloud-pubsub branch 10 times, most recently from a7f236f to 1be2577 Compare June 16, 2023 09:47
@darkowlzz
Copy link
Contributor

darkowlzz commented Jun 16, 2023 •
edited
Loading

TestProviderReconciler_Reconcile/finalizes_suspended_object seems to fail a lot. Most likely due to the usage of Update() method

notification-controller/internal/controller/receiver_controller_test.go

Line 180 in 3944f2e

g.Expect(k8sClient.Update(context.Background(), resultR)).To(Succeed())
.
It may be better to use a patch helper, similar to https://github.com/fluxcd/source-controller/blob/a0ff0cfa885e1e5f506a593a9de39174cf1dfeb8/internal/controller/common_test.go#L137-L140 .
Receiver test has a lot of usage of Update() but that specific test fails with version conflict. It should be okay to update just that test for now.
@matheuscscp can you give it a try as a separate commit? Just create a patch helper, set suspend directly on the object and call patch to update it.

@matheuscscp matheuscscp force-pushed the gcloud-pubsub branch 2 times, most recently from d51fd94 to 888bf5d Compare June 16, 2023 11:54
darkowlzz
darkowlzz approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!
The receiver related test failure is unrelated to these changes. That can be addressed here or separately.

@matheuscscp matheuscscp mentioned this pull request Jun 16, 2023
Merged
@matheuscscp @somtochiama
@makkes @darkowlzz
Add Provider for Google Pub/Sub Topic
04cea9d 
Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>
Co-authored-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
Co-authored-by: Max Jonas Werner <makkes@users.noreply.github.com>
Co-authored-by: Sunny <darkowlzz@protonmail.com>
makkes
makkes approved these changes Jun 16, 2023
View reviewed changes
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

Good job, @matheuscscp! lgtm

@makkes makkes merged commit 508e8f1 into fluxcd:main Jun 16, 2023
@matheuscscp matheuscscp deleted the gcloud-pubsub branch June 16, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@somtochiama somtochiama somtochiama left review comments

@darkowlzz darkowlzz darkowlzz approved these changes

@makkes makkes makkes approved these changes

Assignees
No one assigned
Labels
area/alerting Alerting related issues and PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: Provider for Google Cloud Pub/Sub
4 participants
@matheuscscp @darkowlzz @makkes @somtochiama