Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
Masa8106
left a comment
There was a problem hiding this comment.
Thank you for creating this PR.
Overall, it seems good. As there is several open questions, let us discuss them (Please find my comments.). Thank you.
| | $.sink | exists only if provided in the request body and with the same value | | ||
|
|
||
| # Open questions: | ||
| # serviceArea has to have same value as in the request body or implementations can make adjustments |
There was a problem hiding this comment.
Could you elaborate what you assume "implementations can make adjustments" is? Is there possibility that different serviceArea value with in the request body may be responded?
There was a problem hiding this comment.
It's whether the value in the response has to be strictly the same as in the request or implementation may adjust the serviceArea to facilitate the implementation. I assume that it should be the same, but it would be good to make it more clear in the spec.
There was a problem hiding this comment.
I understood what you pointed out. In my opinion, the same value would be better. But I became to recognise that implemantation may response different serviceArea value through breakout session. Therefore, it seems to be ok that there is no $.serviceArea in examples of the scenario outline.
There was a problem hiding this comment.
How should we handle this?
(1) We assume it should be the same: Write it out in the spec and adding $.serviceArea as example.
(2) We allow implementation adjust the serviceArea: (Write "should be the same" our in the spec, and approve this as it is).
My preference is (2). What do you think?
|
|
||
| # Open questions: | ||
| # serviceArea has to have same value as in the request body or implementations can make adjustments | ||
| # startTime has to have same value as in the request body or implementations can make adjustments, to be returned when status = UNAVAILABLE? |
There was a problem hiding this comment.
Same comment with the above. And it is good question regarding a response when status = UNAVAILABLE.
I think startTime shold be returened even when status = UNAVAILABLE if the booking is created. Because status can be changed to Available at the startTime, even if the status is UNAVAILABLE at the time of booking.
There was a problem hiding this comment.
Same response, e.g. if the request is to start at 12:34:56.789, may the implementation make adjustments to e.g. the nearest second or the returned value must be exactly the same value.
With respect to UNAVAILABLE, the spec now says:
- If the operator determines synchronously that the booking request cannot be fulfilled, the response will be 201 with status = UNAVAILABLE.
so currently a response with status = UNAVAILABLE means that the booking is not created and it cannot evolve to AVAILABLE at startTime. When the booking is created the status must be SCHEDULED.
There was a problem hiding this comment.
Regarding with UNAVAILBLE status that is also related to the next comment, I feel that update of the status diagram, which is created in our very initial discussion, is needed for our clarification.
| # startTime has to have same value as in the request body or implementations can make adjustments, to be returned when status = UNAVAILABLE? | ||
| # duration to be returned when when status = UNAVAILABLE? | ||
| # bookingId to be returned when when status = UNAVAILABLE? | ||
| # statusInfo should have another value when status = UNAVAILABLE after creation, e.g. BOOKING_CANNOT_BE_FULFILLED or open string |
There was a problem hiding this comment.
We can discuss on this. There is status diagram I assume so far here.
There was a problem hiding this comment.
I commented about this above. In the diagram that you link there is no status for a booking request that has been declined, but the v0.1 spec says that the status for a booking declined during creation is UNAVAILABLE. That is something that we need to decide upon.
| # duration to be returned when when status = UNAVAILABLE? | ||
| # bookingId to be returned when when status = UNAVAILABLE? | ||
| # statusInfo should have another value when status = UNAVAILABLE after creation, e.g. BOOKING_CANNOT_BE_FULFILLED or open string | ||
| # sinkCredential may be returned in the response in any case or it should be removed for security |
There was a problem hiding this comment.
I think it is good way to align with other QoD APIs on how sinkCredential is returned.
There was a problem hiding this comment.
Agree. Even more, it should be aligned at Commonalities across all CAMARA APis
There was a problem hiding this comment.
As far as I see QoD API and QoS Provisioning, sinkCrediential in the response is not specified, I think, due to security reason. I prefer to keep as it is (I mean no sinkCredential should be returned).
|
|
||
| # This step is optional, for cases when the implementation did not grant the booking synchronously and later rejects the booking | ||
| # or when it was granted (SCHEDULED) but implementation decides to revoke it before the start | ||
| # TBD if a better status or statusInfo is specified for this use case, e.g. status = "REJECTED" or status = "UNAVAILABLE" + statusInfo = "NOT_SCHEDULED" or "REVOKED" |
There was a problem hiding this comment.
We can discuss on this. There is status diagram I assume so far camaraproject/QualityOnDemand#337.
There was a problem hiding this comment.
as commented above the v0.1 is not aligned with that diagram, so we have to work towards v0.2 on a better definition of statuses in the spec, which is what prevails.
What type of PR is this?
What this PR does / why we need it:
Creates a new version of the test plan, substituting the single file that we had in the last meta, with a level of detail similar to what we have in other CAMARA APIs and aligned with the guidelines.
There is a separate file per operation, covering all expected responses. Comments are included when issues with the current spec are detected, which may be treated separately.
Which issue(s) this PR fixes:
Fixes #69
Special notes for reviewers:
Once merged, this test plan should evolve and be aligned with the API spec towards the next version.
Changelog input