SDK-Issues : Refactoring the code to render as CloudEvent and schema validation #35
Conversation
There was a problem hiding this comment.
Thanks @rjalander!
I'm still reviewing but I thought I'd share my feedback this far.
| PipelineRunFinishedCDEvent pipelineRunFinishedCDEvent = new PipelineRunFinishedCDEvent(); | ||
| return pipelineRunFinishedCDEvent; |
There was a problem hiding this comment.
return new PipelineRunFinishedCDEvent(); ?
| try { | ||
| return objectMapper.writeValueAsString(cdEvent); | ||
| } catch (JsonProcessingException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
What is the benefit here of catching a JsonProcessingException and re-throwing it a RuntimeException?
There was a problem hiding this comment.
Apologies, It was autogenerated and in my TODO list to replace with the error log
| */ | ||
| public static CloudEvent asCloudEvent(CDEvent cdEvent) { | ||
|
|
||
| String cdEventJson = cdEventToJson(cdEvent); |
There was a problem hiding this comment.
This may throw exceptions, shouldn't they be caught or declared in the method signature?
There was a problem hiding this comment.
Now it will be catching in cdEventAsJson() itself, handling empty String error check before proceeding here..
| PipelineRunStartedEvent("dev.cdevents.pipelinerun.started." + VERSION), | ||
| PipelineRunStartedEvent("dev.cdevents.pipelinerun.started." + CDEVENTS_SPEC_VERSION), |
There was a problem hiding this comment.
The version here is wrong. Event types must include the event version, not the spec version - see https://cdevents.dev/docs/primer/#versioning
There was a problem hiding this comment.
I guess the enum here could define all the unversioned even types and the actual event version could be set in the event class instead.
There was a problem hiding this comment.
yes, looked at the CDEvents versioning. Keeping un-versioned types with the Enum and setting event version in the event class itself here.
one clarification on Deployment Type event versions, in the spec it is mentioned 0.1-draft but the go-sdk uses 0.1.0 which needs to be updated in the v0.1.0 spec ?
There was a problem hiding this comment.
Yes, having the -draft in the event version was a bug in release v0.1 of the spec - it is fixed in patch release v0.1.2
There was a problem hiding this comment.
Since then I automated the start and end of releases, so that hopefully we can avoid those kind of bugs in the future.
There was a problem hiding this comment.
Perhaps these classes could be generated with something like https://github.com/joelittlejohn/jsonschema2pojo?
It's also fine to define them by hand for now, but it's quite some work, it's error-prone and hard to maintain.
For the go-sdk I initially used a bash script to generate the skeleton for all even types.
Now I switched to using go templates so that the code is generated at dev time using go.
| * @return the pipeline-run-finished-event schema Json | ||
| */ | ||
| @Override | ||
| public String eventSchema() { |
There was a problem hiding this comment.
Very nice, thank you!
In future, it would be great to be able to produce this automatically from the schemas in the spec repo.
There was a problem hiding this comment.
Thanks @rjalander - this looks good as a first step.
We'll need to implement all other events after this.
It's good that the new test runs validation, that gives us more confidence on the correctness of the SDK.
|
@rjalander it looks like this needs to be rebased |
I think If we do Squash and merge, all commits will be treated as single commit and merged right. |
|
🎉 This issue has been resolved in |
This PR has fixes for couple of Issues
#26
#29
#30
#31
This PR has the Refactoring code changes to create CDEvents using an object model,
Creating a smaller PR to have this for only one CDEvent - PipelineRunFinishedCDEvent and the similar pattern can be extended to all other events from the specification.