-
Notifications
You must be signed in to change notification settings - Fork 7
Release r1.1 with v0.1.0-rc.1 #59
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
Conversation
|
Created camaraproject/ReleaseManagement#218 for the review by Release Management team. Note: as this is a release candidate for Fall25 it needs to be aligned with the upcoming releases r3.2 of Commonalities and ICM. |
Thanks herbert, I have modified based on the release r3.2 of Commonalities and ICM. However there is not release r3.2 yet, I can't link to it. So I linked to r3.1 for the moment. |
tanjadegroot
left a 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.
Starts looking great ! please apply proposed changes.
| | 2 | Design guidelines from Commonalities applied | O | M | M | M | Y | | | ||
| | 3 | Guidelines from ICM applied | O | M | M | M | Y | | | ||
| | 4 | API versioning convention applied | M | M | M | M | Y | v0.1.0 | | ||
| | 2 | Design guidelines from Commonalities applied | O | M | M | M | Y | [r3.1](https://github.com/camaraproject/Commonalities/releases/tag/r3.1) | |
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.
| | 2 | Design guidelines from Commonalities applied | O | M | M | M | Y | [r3.1](https://github.com/camaraproject/Commonalities/releases/tag/r3.1) | | |
| | 2 | Design guidelines from Commonalities applied | O | M | M | M | Y | [r3.2](https://github.com/camaraproject/Commonalities/releases/tag/r3.2) | |
| | 3 | Guidelines from ICM applied | O | M | M | M | Y | | | ||
| | 4 | API versioning convention applied | M | M | M | M | Y | v0.1.0 | | ||
| | 2 | Design guidelines from Commonalities applied | O | M | M | M | Y | [r3.1](https://github.com/camaraproject/Commonalities/releases/tag/r3.1) | | ||
| | 3 | Guidelines from ICM applied | O | M | M | M | Y | [r3.1](https://github.com/camaraproject/IdentityAndConsentManagement/releases/tag/r3.1) | |
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.
| | 3 | Guidelines from ICM applied | O | M | M | M | Y | [r3.1](https://github.com/camaraproject/IdentityAndConsentManagement/releases/tag/r3.1) | | |
| | 3 | Guidelines from ICM applied | O | M | M | M | Y | [r3.2](https://github.com/camaraproject/IdentityAndConsentManagement/releases/tag/r3.2) | |
documentation/API_documentation/network-slice-booking-API-Readiness-Checklist.md
Show resolved
Hide resolved
Co-authored-by: Tanja de Groot <87864067+tanjadegroot@users.noreply.github.com>
Co-authored-by: Tanja de Groot <87864067+tanjadegroot@users.noreply.github.com>
Co-authored-by: Tanja de Groot <87864067+tanjadegroot@users.noreply.github.com>
Co-authored-by: Tanja de Groot <87864067+tanjadegroot@users.noreply.github.com>
Co-authored-by: Tanja de Groot <87864067+tanjadegroot@users.noreply.github.com>
|
@tanjadegroot Thank you for such detailed suggestions. I've finished the revisions based on your recent comments. Please take a moment to review them. |
|
@hdamker @tanjadegroot : Thank you for the detailed review, we have now fixed all mandatory changes.
|
tanjadegroot
left a 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.
I would recommend making the changes on the property names to lowerCamelCase as soon as possible to avoid redoing API implementations, and the description fields for M4.
Once the lowerCamelCase updates are done (I do not put them as blocking the merge)
/LGTM from Release Management - great work by the team !
| post: | ||
| tags: | ||
| - Network Slice Booking Sessions | ||
| summary: Creates a new session |
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.
The description field is missing. This should normally contain the API endpoint documentation to help the developers understand the specificities of the endpoint API.
| tags: | ||
| - Network Slice Booking Sessions | ||
| summary: Delete a NSB session | ||
| description: Deleting NSB session |
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.
Ideally this should have some explanation about delete usage e.g. what is the effect of deleting the session befor the endtime that is was booked for, etc.
| tags: | ||
| - Network Slice Booking Sessions | ||
| summary: Get NSB session information | ||
| description: Querying for NSB session resource information details |
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.
The description may contain more details about the provided resource information that is returned when calling this end-point.
NOTE. you can improve the descriptions also later for M4
| description: Attributes required to create a session | ||
| type: object | ||
| properties: | ||
| ServiceTime: |
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.
property and similar names follow lowerCamelCase naming pattern: serviceTime, serviceArea, qosProfile, maxNumOfDevices, downStreamRatePerDevice, etc.
Schema names follow UpperCamelCase naming pattern so ideally the "QoSProfile" should be "QosProfile" (with a lower case "s").
these rules help in readability.
These updates could also wait for M4 but would have impact on test API implementations.
These updates will also impact the .feature files, and possibly other e.g. the documentation file(s)
| type: object | ||
| properties: | ||
| value: | ||
| description: Quantity of duration |
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.
| description: Quantity of duration | |
| description: Length of duration |
@tanjadegroot : Thanks Tanja,we have finished the lowerCamelCase changes per your suggestion. And for the description fileds, we created the issue #64 to track it and will fix it in M4 with more detailed description:
Requesting re-review on the updates - kindly provide your approval or comments when available. Many thanks! |
| * **serviceTime**: is defined by a start time and an end time, which indicates the period during which the network slice will be reserved. | ||
| * **serviceArea**: can be defined as a circle or a polygon, allowing for flexible geographical coverage. | ||
| * **qosProfile**: includes parameters such as maximum number of devices, downstream and upstream rate per device, and packet delay budget. | ||
| * **SessionId**: is a unique identifier for the session, which is returned when a session is created and used to retrieve or delete the session later. |
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.
Hi, many thanks for doing these updates. I did not list exhaustively all the names to be updated.
please check for other cases as well, e.g.
SessionId --> sessionId
thanks !
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.
Apologies for the oversight. We've double-checked and updated all cases. I think I have corrected all in commit. Your feedback is greatly appreciated! Let us know if you spot anything else!
| - serviceArea | ||
| - qosProfile | ||
|
|
||
| QosProfile: |
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 have already a definition for QosProfile within the API qos-profiles in QualityOnDemand which is different from the definition here (as defined as profile for one device).
But as the same schema is already used in multiple APIs, I would prefer to use another name for the profile which is specific for NetworkSliceBooking, e.g. NetworkSliceQosProfile, to avoid confusions.
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.
Thanks Herbert, this indeed makes sense. We have created issue #65 to track it. How about SliceQosProfile? Maybe it‘s more brief and also indicates the qosProfile for sclice.
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.
SliceQosProfile is also fine for me.
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.
@hdamker @tanjadegroot :
Thanks for your comments, we have corrected naming pattern in Naming pattern correction, and modified the QosProfile to SliceQosProfile in Modify the name for schema QosProfile to avoid confusions.
Could you please review the updates again and share your approval or feedback at your earliest convenience? Thank you!
hdamker
left a 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.
@XunliYang As far as I can see are all review comments from @tanjadegroot addressed which is great. Unfortunately I found with a little help of AI some further issues.
High priority (should be addressed before release)
Replace 204 usage with appropriate error codes (400/422)
- Issue: HTTP 204 means success and "No Content" and should not have a response body. Using it for failures is non-standard.
- Fix: Use appropriate 4xx codes (e.g., 422 UNPROCESSABLE_ENTITY or 400 with specific error codes). Correct also the test definition, the tests expect response body for 204, which contradicts HTTP standards.
Standardize error message enums
- Issue: Inconsistent Error Message Format
message:
enum:
- SERVICEAREA NOT SUPPORTED # Has space
- RESOURCES INSUFFICIENT
Fix: Use consistent format and consider to define as API specific: NETWORK_SLICE_BOOKING.SERVICEAREA_NOT_SUPPORTED
NETWORK_SLICE_BOOKING.RESOURCES_INSUFFICIENT
Note: the definition as specific API error codes I see as medium priority, if you address it, please check for other API specific error codes, and cf. CAMARA-API-Design-Guide.md#31-standardized-use-of-camara-error-responses
Low priority (to be considered after M3)
Add description to operation
- Missing description for POST /sessions
|
@hdamker : Thanks for pointing this out - great catch! We'd actually considered standardizing these operation error codes and creating custom business logic failure codes before realizing it wasn't the right approach. Really appreciate you flagging this. Here are the changes we've implemented:
|
tanjadegroot
left a 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.
A few more changes and your good to go !
| "404": | ||
| $ref: "#/components/responses/Generic404" | ||
| "429": | ||
| $ref: "#/components/responses/Generic429_requests" |
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.
| $ref: "#/components/responses/Generic429_requests" | |
| $ref: "#/components/responses/Generic429" |
| "404": | ||
| $ref: "#/components/responses/Generic404" | ||
| "429": | ||
| $ref: "#/components/responses/Generic429_requests" |
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.
| $ref: "#/components/responses/Generic429_requests" | |
| $ref: "#/components/responses/Generic429" |
| code: TOO_MANY_REQUESTS | ||
| message: Either out of resource quota or reaching rate limiting. | ||
|
|
||
| Generic429_requests: |
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 schema can be removed as it is the same as the Generic429. I have proposed above the update of the 2 occurrences of this one to be Generic429 instead.
Also align the examples section of the Generi429 error definition with the definitions in CAMARA_common.yaml (see https://github.com/camaraproject/Commonalities/blob/r3.2/artifacts/CAMARA_common.yaml)
Maybe also check the other error examples to be aligned
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.
Thanks Tanja. We treat Generic429_request as a separate case from Generic429 since POST operation contains the extra error code QUOTA_EXCEEDED. Thus, maybe can we just modify the code message to align with the definitions in CAMARA_common.yaml and keep the Generic429_request?
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.
Hi @XunliYang If you compare with the latest in https://github.com/camaraproject/Commonalities/blob/r3.2/artifacts/CAMARA_common.yaml the Generic429 seems the same as your definition of Generic429_requests - so ideally you would use the name from CAMARA_common one but remove the code the QUOTA_EXCEEDED one.
I will verify if that is allowed, but the Guidelines do not say that all error codes need to be supported by the API provider.
For now, let's keep what you have done to meet the M3 deadline
| @@ -1,5 +1,5 @@ | |||
| @NetworkSliceBooking | |||
| Feature: CAMARA Network Slice Booking API v0.1.0 - Operations for createSession | |||
| Feature: CAMARA Network Slice Booking API v0.1.0-rc.1 - Operations for createSession | |||
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.
| Feature: CAMARA Network Slice Booking API v0.1.0-rc.1 - Operations for createSession | |
| Feature: CAMARA Network Slice Booking API v0.1.0-rc.1 - Operation createSession |
| Given starttime, endtime, the request body property "$.ServiceArea" is set to the service area of not supported and the configuration of information of network slicing | ||
| Given starttime, endtime, the request body property "$.serviceArea" is set to the service area of not supported and the configuration of information of network slicing | ||
| When the request "createSession" is sent | ||
| Then the response status code is 204 |
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.
need to update this to the same error code as done in the API yaml, instead of a success code (204).
same for the other 204 "fail" scenarios below.
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.
line 132 and 143: update the messages to the same as in the Generic429 definition (per CAMARA_common.yaml)
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.
Apply same changes as in comments on the createSession.feature file (if applicable)
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.
Apply same changes as in comments on the createSession.feature file (if applicable)
|
@tanjadegroot: Thanks for your valuable feedback. We've implemented the following modifications accordingly in commit Modify error codes 200 201 429:
|
tanjadegroot
left a 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.
thanks for the hard work on this API.
It looks great !
/LGTM from Release Management
| code: TOO_MANY_REQUESTS | ||
| message: Either out of resource quota or reaching rate limiting. | ||
|
|
||
| Generic429_requests: |
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.
Hi @XunliYang If you compare with the latest in https://github.com/camaraproject/Commonalities/blob/r3.2/artifacts/CAMARA_common.yaml the Generic429 seems the same as your definition of Generic429_requests - so ideally you would use the name from CAMARA_common one but remove the code the QUOTA_EXCEEDED one.
I will verify if that is allowed, but the Guidelines do not say that all error codes need to be supported by the API provider.
For now, let's keep what you have done to meet the M3 deadline
|
@hdamker: We believe all requested changes have been addressed. Could you kindly re-review and share your feedback or approval? Thank you for your time! |
I trust that the changes are done, but don't have the time for a review.
|
@XunliYang the review is complete. I checked on the error code, and indeed you are allowed to remove error code from the list for the Generic429 to adust it to the API need (as for other optional error codes (see Commonalities API Guide section 3.1 under the tables): Error statuses 400, 404, 409, 422, ****: These error statuses SHOULD be documented based on the API design and the functionality involved. Subprojects evaluate the relevance and necessity of including these statuses in API specifications. so I would still recommend to align the error name to Generic429 instead of an API specific one before merging if you have time, or create an issie in the repo to address it later. |
@tanjadegroot @hdamker Thanks for your suggestions. We will merge this PR to meet the M3 deadline. Also we have created an issue #68 to track it later and modify after this PR. However I see that other APIs also have specific error names such as QOD's |
yes, you can make your own error codes as needed, but then they are not "GenericXxx", but it has a specific name for the specific error. In your case, the error is the same as Generic429 with one less option in the codes enum, so no need for a specific error definition. The QoD API was one of the first APIs defined in CAMARA, and was done before the Commonalities rules for error codes were established. So they may have some deviations for backward compatibility reasons. For new APIs, we try to make developers life as easy as possible by having consistent usage in specifications. Thank you very much for considering this update. |
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Prepare the release r1.1 with v0.1.0-rc.1 API version:
Which issue(s) this PR fixes:
Fixes #58
Changelog input
Additional documentation
This section can be blank.