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
Define CAMARA Event using cloudevents in OAS notification #43
Define CAMARA Event using cloudevents in OAS notification #43
Changes from all commits
3dd803c
8a11ac5
339a8e2
e2418d3
0c6c11f
a97bb67
9226178
234cfdb
8922567
950efcd
7904ec2
fd986cd
2420cf3
2eb7b7d
b7eb630
248e442
14b8650
441360c
21711ac
e90cdd7
338a4a2
3b19bae
fec372a
430f4ca
984794b
2d4683a
e5ced0d
429a7e3
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.
I think this is correct but I think someone complained about lack of support by certain code generators. We could stick to just
application/json
if it is saferThere 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.
According to CloudEvents discussion format of "+json" is defined in RFC6839, so it should be supported by code generators.
I propose to follow the CloudEvents requirement for JSON format:
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.
Generic500 can be used
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.
Done
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.
We should also add a discriminator explicitly to this base class schema
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 long the id MUST be a non-empty string - I also recommend the constraints "minLength" 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.
Done
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 the format agreed is org.camaraproject...
recommend to increase to "25" -> org.camaraproject.qod.v0.
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.
There is a discussion in camaraproject/QualityOnDemand#224 (comment), about whether 1) to define
data
in the base class, as optional and with a generictype: object
, or 2) to definedata
only in the child events which has data. Both would be correct, but not sure about which one would work better with code generators.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 do not have experience with code generators, but currently we do not have any example of subproject using events without
data
object. So for simplicity I prefer approach 1) where we do not have to define child objectChildEventWithData
in each subproject using eventing, butdata
is in base class as optional.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.
Indeed for that reason we have to define "a ChildEventWithData" schema, including
data
for each Child event. The name is just a templated one, real names we already have, among others:QoD:
DeviceStatus:
so we have to define the data per child event mandatorily always. The question was whether we should have to define it also in the base schema as optional.
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.
@jlurien, I agree with you, from an OO perspective we don't need to define it in the base schema.
I'm undecided because optional data field can also be considered a placeholder for what to extend in child classes.
But as you are the first that use this template in QoD and don't find it so useful, I will remove it from base class.
@rartych, both versions works with code generators.
I think defining ChildEventWithData add unnecessary layer in model, I just added a comment in CloudEvent description. WDYT about?
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.
So far, it is kept as optional in the CloudEvent base class. I think this is what get more consensus. If both work well with code generators, they are totally equivalent and the final schema once the composition is applied is the same.
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 fine with both approaches. API Design Guidelines are "normative" for CAMARA, here we provide example that can be used by subprojects and API Consumers, but other definitions can be used if they conform to API Design Guidelines and CloudEvents specification.
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.
Please see: #43 (comment)
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.
To make the template even more useful, I would add also a sample schema for children events something like: