Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(cdevents-notification): Implementing produce CDEvents using Notification #1295
feat(cdevents-notification): Implementing produce CDEvents using Notification #1295
Changes from 14 commits
483a461
3b759ee
576e7d1
b943650
68ff04e
174e51b
25e6b7a
09237b9
de76a8c
02a6610
f687deb
34c40c9
6ff8064
71f5c8f
e7cf714
cb9e3a7
6470546
ac7262a
65e8b33
f573844
a24cabd
f9fa051
2eecfe0
af917bb
7b832ee
fc31963
0ebb3ce
1f783f6
621da6b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
null
valid? We probably want to throw an exception here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, throwing an existing FieldNotFoundException here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd too, considering both these fields could not be present, but we would still try to build an
executionUrl
from it. I think we should probably throw an exception, unless we have proper defaultsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, throwing an existing FieldNotFoundException here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these defined in the CDEvents SDK? I'd rather pull the enums from there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I can pull that, there are Enum constants created with event types in the CDEvents SDK,
May be I need to compare with substring of a
cdEventsType
to use with Enum constantsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please do that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we only interesting int he
pipelinerun
andtaskrun
events?Also, most of these have the same parameter, is there an interface that can be used here? That way we can just have a map of those interfaces. Should simplify this some and makes adding to that map easier in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per my comment in the PR description, I will update the same PR/create another PR for all other events once this review is closed.
Current PR has the implementation to produce [Core CDEvents](https://github.com/cdevents/spec/blob/v0.1.2/core.md) for now, other events will be implemented after this PR review and discussion.
My question here is does Spinnaker needs all types of events which CDEvents spec provides(https://cdevents.dev/docs/) or needs a subset of event types based on any Spinnaker requirements.
Like I am not sure whether Source Code Control Events are required for Spinnaker to send.
There is no such interface to create CDEvents(might be a good idea to propose this change from SDK itself as that interface/implementations can be used as common),
I can update the current switch case to a map for flexibility and extensibility purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was meaning though is:
Then in this class
That way the switch statement goes away and instead becomes something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I'm in the same direction, was thinking aloud rather implementing in the Spinnaker is that a good idea to create these interface and classes in the CDEvents Java-SDK itself,
but I know It will restrict to create CDEvents the way Spinnaker wants with any additional changes.
Will create these classes with little modifications here.
Implement the classes for each event
creating a map of different events in this class,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then all these classes can just implement that interface I mentioned above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A concern here is this is REALLY raw java layer for HTTP communications. The standard in spinnaker is to use retrofit & ok-http client libs. These automatically add things LIKE timeouts, URL validation, etc. that are pretty key settings for most environments. I've not looked at the CloudEvents MessageWriter system... so may HAVE to do this, but definitely have concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on this. Let's use an actual HTTP client, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to use Retrofit client to send CDEvent but facing difficulties in deserializing as Retrofit doesn't directly use
CloudEventHttpMessageConverter
is the issue.Can I use spring RestTemplate to send the CDEvent instead, please share your ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With retrofit and a return type of Response (e.g. here), I believe you'll get flexibility on converting the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could able to solve this issue by creating a custom CDEventsHTTPMessageConverter as retrofit doesn't directly support CloudEventHttpMessageConverter, this converter uses Jackson converter to map the CloudEvent as String and send this String with the Requestbody
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2023 Nordix
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a rule, we should be throwing spinnaker exceptions not custom exceptions for these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to call out here, is unless you are modifying the retrofit exception handler, these custom exceptions may not be handled properly (I dont think). One thing we could consider doing is having some base spinnaker exception that custom exceptions could inherit from which could make throwing custom exceptions a little more flexible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDEventsException
is not needed now as using the retrofit request/response to send CDEvents notification