-
Notifications
You must be signed in to change notification settings - Fork 61
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
Align quality-on-demand with Commonalties regarding optional device object and error messages #326
Conversation
Add documentation about "Handling of device information" within the API description. Made device parameter optional.
Aligned Device object and info with Commonalities
Updated errorMessage according to Commonalities. Added 422, removed 501 (not used). Reduced 400 examples within createSession to the ones not covered by other codes according to chapter 6.2 of Design Guidelines.
@maxl2287 @akoshunyadi @jlurien I created the PR by intention first as draft, but would be great if you can already review. @eric-murray the documentation I copied from Commonalities is only referring to "device" associated with the access token. I get your point that "end user" is more correct. But do you have a proposal not to confuse API consumers here? My proposal to put "(and device)" behind end user is most probably not the best ... |
Removing trailing spaces
@hdamker are you planning to remove the draft status? This PR is key for the meta-release as it incorporates the alignments to guidelines. Once this is merged, we can adapt the remaining ones (listSessions, Provisioning) to the updated content. For listSessions I have to define same behaviour for device optionality and adopt the new errors. |
@jlurien Yes, removed the draft status now to be able to discuss it within the meeting. We should agree who (which PR) is changing the 404 errors. |
@camaraproject/quality-on-demand_maintainers:We are most probably also impacted by the discussion in camaraproject/Commonalities#259 about potential misuse of the API behaviour. Let's discuss how to deal with it in the meeting. |
…ision-about-optional-device-object-and-respective-documentation
As discussed within QoD Call:
|
Added error messages from Commonalities in 404, 422
space removed after colon
Added description for SessionInfo schema to address camaraproject#328
Remove trailing space
Added clarification to all relevant endpoints: "The session must must have been created by the same API client given in the access token"
Removed trailing space
More trailing spaces deleted
Added (mandatory) description in info object about "Identifying a device from the access token" Updated most error messages with copies from https://github.com/camaraproject/Commonalities/blob/main/artifacts/CAMARA_common.yaml
Some other fixes:
should be externalDocs:
|
Addressed review comments
Address review comment
externalDocs updated
ExternalDocs object updated in both APIs (I had planned it for the release PR).
Ah, yes - due to the new |
Another ones:
Long but it should be the new API name, if we finally keep the proposal.
|
That outside of the scope of this PR, at least we should open an issue for that to make the change more transparent. |
I agree that it is clearer if it is visible which examples are belonging to which operation. But in https://github.com/camaraproject/Commonalities/blob/a4f409d3c9e278bd30afc838cee8a261ff450939/artifacts/CAMARA_common.yaml#L209 Generic404 is defined with both NOT_FOUND and DEVICE_NOT_FOUND and even a placeholder for specific codes. But ok, I will apply the approach from your #299. |
Good question. I though it was intended to match the major version of the API, but I guess the major version of the API could change without evolving the event version (but not the opposite). |
Created #332 for the larger topic that we missed the changes within the Event subscription model, which impacts also our implicit subscriptions. Changing the event name is the smaller problem here (btw: I would stay here with v0, not using "wip" in between - the risk is too high to oversee this in the release PR. The version of the event will get only "visible", when there is a released API Version). |
Added error schema "GenericDevice404" as discussed in camaraproject#326 (comment)
|
@jlurien @maxl2287 @eric-murray @RandyLevensalor The PR should be ready to be merged. Would be also good to get it merged soon, so that we can base the other PRs on it. Can you do a timely final review? |
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.
LGTM, I assume #335 as a separate one but it is quite independent so merging order should not be relevant
MNO replaced with network operator
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.
LGTM
Given the two approvals from @RandyLevensalor and @jlurien (before the last trivial change) I will merge this one now, so that we can continue to align the other open PRs (if needed). Please open issues if there other alignment points with Commonalities are open. |
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it (Updated):
Which issue(s) this PR fixes:
Fixes #313
Fixes #315
Fixes #316
Special notes for reviewers:
Intentionally only done for quality-on-demand to reduce potential merge conflicts.Done for both APIs quality-on-demand and qos-profiles.
Changelog input (Updated)