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

feat: eventsource service metadata #3257

Merged
merged 79 commits into from
Sep 28, 2024
Merged

feat: eventsource service metadata #3257

merged 79 commits into from
Sep 28, 2024

Conversation

MenD32
Copy link
Contributor

@MenD32 MenD32 commented Aug 24, 2024

Checklist:

added apicommon.Metadata as a field to the Service in EventSourceSpec, to allow custom configuration of labels and annotations.

inspired by issue #3242

@MenD32 MenD32 requested a review from whynowy as a code owner August 24, 2024 15:04
@MenD32 MenD32 marked this pull request as draft August 24, 2024 15:04
@MenD32 MenD32 force-pushed the feat/eventsource-service-metadata branch from 3dc3fd5 to 44d940c Compare August 24, 2024 15:05
@MenD32 MenD32 marked this pull request as ready for review August 24, 2024 15:07
@MenD32 MenD32 marked this pull request as draft August 24, 2024 15:44
@MenD32
Copy link
Contributor Author

MenD32 commented Aug 24, 2024

i am not sure why kafka E2E fails, since the code changes shouldn't affect it at all. if someone has a clue of why this would happen, I'm open to suggestions

@MenD32 MenD32 marked this pull request as ready for review August 25, 2024 07:01
whynowy
whynowy reviewed Aug 29, 2024
View reviewed changes
controllers/eventsource/resource.go Outdated
@@ -397,6 +398,8 @@ func buildService(args *AdaptorArgs) (*corev1.Service, error) {
Selector: args.Labels,
},
}
svc.ObjectMeta.SetAnnotations(args.EventSource.Spec.Service.Metadata.Annotations)
Copy link
Member

Choose a reason for hiding this comment

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

nil check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an empty dict by default, using kubebuilder comments, I will add a comment in the function denoting that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an if statement because there is a change that the yaml will be nil explicitly

controllers/eventsource/resource.go Outdated
@@ -397,6 +398,8 @@ func buildService(args *AdaptorArgs) (*corev1.Service, error) {
Selector: args.Labels,
},
}
svc.ObjectMeta.SetAnnotations(args.EventSource.Spec.Service.Metadata.Annotations)
svc.ObjectMeta.SetLabels(args.EventSource.Spec.Service.Metadata.Labels)
Copy link
Member

Choose a reason for hiding this comment

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

nil check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an empty dict by default, using kubebuilder comments, I will add a comment in the function denoting that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an if statement because there is a change that the yaml will be nil explicitly

@whynowy whynowy changed the title Feat/eventsource service metadata feat: eventsource service metadata Aug 29, 2024
@MenD32 MenD32 force-pushed the feat/eventsource-service-metadata branch from 08efc9b to 7e2be6b Compare September 4, 2024 12:16
@MenD32
Copy link
Contributor Author

MenD32 commented Sep 4, 2024 •
edited
Loading

i added a 3 second sleep in the e2e test, since the test was inconsistent with Kafka, even though the changes are completely irrelevant

whynowy
whynowy reviewed Sep 10, 2024
View reviewed changes
controllers/eventsource/resource.go Outdated

if args.EventSource.Spec.Service.Metadata != nil {
if args.EventSource.Spec.Service.Metadata.Labels != nil {
svc.ObjectMeta.SetLabels(args.EventSource.Spec.Service.Metadata.Labels)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of SetLabels and SetAnnotations, can we do for loops and copy the labels and annotations over? Set means original labels and annotations will be replaced.

@MenD32 MenD32 force-pushed the feat/eventsource-service-metadata branch 2 times, most recently from 3ee7f4d to 046b481 Compare September 12, 2024 19:31
@MenD32 MenD32 marked this pull request as draft September 12, 2024 21:45
omerap12 and others added 15 commits September 23, 2024 18:43
@omerap12
Added log level option configuration (argoproj#3201)
736ddc4 
Signed-off-by: Omer Aplatony <[email protected]>
Signed-off-by: MenD32 <[email protected]>
@dependabot
chore(deps): bump github.com/minio/minio-go/v7 from 7.0.73 to 7.0.74 (a…
7330f7c 
…rgoproj#3211)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: MenD32 <[email protected]>
@dependabot
chore(deps): bump github.com/xanzy/go-gitlab from 0.106.0 to 0.107.0 (a…
9934d82 
…rgoproj#3212)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: MenD32 <[email protected]>
@dependabot
chore(deps): bump github.com/slack-go/slack from 0.13.0 to 0.13.1 (ar…
2cf92da 
…goproj#3213)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: MenD32 <[email protected]>
@sdidier-dev
Fix tuto exemple 02-parameterization/sensor-05.yaml (argoproj#3218)
deb7770 
Signed-off-by: MenD32 <[email protected]>
added service metadata field
a7b6bce 
Signed-off-by: MenD32 <[email protected]>
added Metadata to buildService
49d4ea6 
Signed-off-by: MenD32 <[email protected]>
crds and codegen
ccec29f 
Signed-off-by: MenD32 <[email protected]>
removed empty lines from bad codegen
b02acdf 
Signed-off-by: MenD32 <[email protected]>
debugging kafka e2e
936d0f1 
Signed-off-by: MenD32 <[email protected]>
debugging kafka e2e
ce2eeaf 
Signed-off-by: MenD32 <[email protected]>
debugging kafka e2e
49f7131 
Signed-off-by: MenD32 <[email protected]>
debugging kafka e2e
07d9161 
Signed-off-by: MenD32 <[email protected]>
debugging kafka e2e
da9c203 
Signed-off-by: MenD32 <[email protected]>
debugging kafka e2e
e819bf2 
Signed-off-by: MenD32 <[email protected]>
MenD32 and others added 9 commits September 23, 2024 18:43
debugging
06b5727 
Signed-off-by: MenD32 <[email protected]>
changed labels and annotations logic to merge
4c814a2 
Signed-off-by: MenD32 <[email protected]>
changed labels and annotations logic to merge
fadb2a6 
Signed-off-by: MenD32 <[email protected]>
changed labels and annotations logic to merge
de5b2f0 
Signed-off-by: MenD32 <[email protected]>
changed labels and annotations logic to merge
2f82d8a 
Signed-off-by: MenD32 <[email protected]>
changed labels and annotations logic to merge
3876f33 
Signed-off-by: MenD32 <[email protected]>
changed labels and annotations logic to merge
bc9e848 
Signed-off-by: MenD32 <[email protected]>
changed labels and annotations logic to merge
719646f 
Signed-off-by: MenD32 <[email protected]>
@Daimanta
fix:eventsource crash on cast argoproj#3238 (argoproj#3282)
ef78747 
Signed-off-by: MenD32 <[email protected]>
@MenD32 MenD32 force-pushed the feat/eventsource-service-metadata branch from e06fc6e to ef78747 Compare September 23, 2024 15:43
@MenD32 MenD32 marked this pull request as ready for review September 23, 2024 15:58
@MenD32 MenD32 requested a review from whynowy September 24, 2024 08:12
whynowy
whynowy approved these changes Sep 26, 2024
View reviewed changes
controllers/eventsource/resource.go Outdated
labels = mergeLabels(args.EventSource.Spec.Service.Metadata.Labels, labels)
}
if args.EventSource.Spec.Service.Metadata.Annotations != nil {
annotations = mergeLabels(args.EventSource.Spec.Service.Metadata.Annotations, annotations)
Copy link
Member

Choose a reason for hiding this comment

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

The function mergeLabels is weird, but we can refactor it later.

@whynowy
Copy link
Member

whynowy commented Sep 26, 2024

@MenD32 - can you resolve the conflict?

@MenD32
Copy link
Contributor Author

MenD32 commented Sep 26, 2024

Yeah

MenD32 added 4 commits September 26, 2024 22:39
merged with master
892fff0 
Signed-off-by: MenD32 <[email protected]>
merged with master
4b04021 
Signed-off-by: MenD32 <[email protected]>
merged with master
8e84bff 
Signed-off-by: MenD32 <[email protected]>
merged with master
e9ef164 
Signed-off-by: MenD32 <[email protected]>
whynowy
whynowy reviewed Sep 27, 2024
View reviewed changes
test/e2e/functional_test.go Outdated
@@ -189,6 +189,8 @@ func (s *FunctionalSuite) TestMetricsWithWebhook() {
Contains("argo_events_event_processing_duration_milliseconds").
Contains("argo_events_events_processing_failed_total")

time.Sleep(3 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i had some weird behavior with the kafka e2e test, and for some reason it helped, but i can remove it.

api/event-source.html Outdated
Copy link
Member

Choose a reason for hiding this comment

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

We don't have this file any more.

api/event-source.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

same for this file.

@whynowy
Copy link
Member

whynowy commented Sep 28, 2024

@MenD32 - unfortunately you have to resolve the conflicts again, sorry for the inconvenience.

@whynowy whynowy merged commit a6199ce into argoproj:master Sep 28, 2024
7 checks passed
@ryancurrah
Copy link
Contributor

@MenD32 why would you want to allow custom configuration of labels and annotations on a Service?

@MenD32
Copy link
Contributor Author

MenD32 commented Sep 30, 2024

Interactions with other controllers

F.e. backendconfig in gke
https://cloud.google.com/kubernetes-engine/docs/how-to/ingress-configuration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whynowy whynowy whynowy 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.
7 participants
@MenD32 @whynowy @ryancurrah @omerap12 @sdidier-dev @undeadops @Daimanta