-
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
Add query to qos-profiles to query profiles available on a given device #318
Add query to qos-profiles to query profiles available on a given device #318
Conversation
If you made
|
@eric-murray Good points. I went back and forth on the convince of keeping the get vs forcing everything to a post. If no one objects to just using the post, I'll make this change. |
This covers all query options with a single call.
Co-authored-by: Herbert Damker <herbert.damker@telekom.de>
@camaraproject/quality-on-demand_maintainers within final review, please provide your comments - if any - until tomorrow, Tuesday, eob. |
@RandyLevensalor added some of the changes which I applied in #326 to quality-on-demand here as comments. |
@RandyLevensalor seems that there weren't further comments. Will you address my comments? |
Co-authored-by: Herbert Damker <herbert.damker@telekom.de>
@hdamker Yes. I'm not sure how I missed MNO being added to the comments. So, I'll replace that mobile specific terminology with service provider to be more inclusive of wireline operators. |
I missed that as well. And worse: it is defined with the MNO abbreviation in CAMARA_common.yaml. I will open an issue on that. |
@hdamker I believe that your comments have been resolved. |
@RandyLevensalor Looks good to me. BTW: in #326 I also replaced now all "...Notfound404" with the "Generic404" as recommend by @jlurien in #326 (comment). All references were in lines you haven't touched in your PR, so I hope it will merge together without conflicts 🤞 |
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
@camaraproject/quality-on-demand_codeowners are we ready to merge this? |
@RandyLevensalor: I would like to merge first #326 ... in best case there are no conflicts but if yes, the a rebase might be needed. |
@hdamker it doesn't show any merge conflicts |
@RandyLevensalor Yes, no merge conflict, but unfortunately I've missed to integrate the changes from 55c609a also proactive into qos-profiles.yaml. So either you rebase and apply the changes in your PR or we need to create a new PR for the following changes afterwards:
|
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.
See #318 (comment) for needed changes
@hdamker Thanks for the feedback. Rebased, updated the device and added 422 and 429 as well. |
Perfect, almost done then ... just delete lines 609 - 614 from |
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
One type found.
Co-authored-by: Herbert Damker <herbert.damker@telekom.de>
Added changelog entries for PR camaraproject#318
What type of PR is this?
What this PR does / why we need it:
Adds a post call to query for qos profiles based on a device. It uses the same device paramaters as the QoS Session create call.
The original get call was not modified.
Which issue(s) this PR fixes:
Fixes #166
Special notes for reviewers:
Since this code was largely copied from Quality-On-Demand end point, please check for copy past errors.
Changelog input
Additional documentation
This section can be blank.