Skip to content
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

Merged
merged 13 commits into from
Aug 6, 2024

Conversation

RandyLevensalor
Copy link
Collaborator

@RandyLevensalor RandyLevensalor commented Jul 11, 2024

What type of PR is this?

  • enhancement/feature

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

 release-note

Added /qos-profiles-by-device post method to enable serching for QoS profiles which are available on a given device.

Additional documentation

This section can be blank.

docs

@RandyLevensalor RandyLevensalor changed the title Feat/qos-profile-device Add query to qos-profiles to query profiles available on a given device Jul 11, 2024
@eric-murray
Copy link
Collaborator

If you made device optional, then the existing GET /qos-profiles would become redundant, and a single endpoint would satisfy all use cases:

  • A 3-legged token (with or without device specified in the body) would return all profiles available to that device
  • A 2-legged token with device specified in the body would return all profiles available to that device
  • A 2-legged token with no device specified in the body would return all possible profiles

@RandyLevensalor
Copy link
Collaborator Author

If you made device optional, then the existing GET /qos-profiles would become redundant, and a single endpoint would satisfy all use cases:

  • A 3-legged token (with or without device specified in the body) would return all profiles available to that device
  • A 2-legged token with device specified in the body would return all profiles available to that device
  • A 2-legged token with no device specified in the body would return all possible profiles

@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.

Co-authored-by: Herbert Damker <herbert.damker@telekom.de>
@hdamker
Copy link
Collaborator

hdamker commented Jul 29, 2024

@camaraproject/quality-on-demand_maintainers within final review, please provide your comments - if any - until tomorrow, Tuesday, eob.

@hdamker hdamker requested review from a team and hdamker July 29, 2024 16:33
code/API_definitions/qos-profiles.yaml Outdated Show resolved Hide resolved
code/API_definitions/qos-profiles.yaml Outdated Show resolved Hide resolved
code/API_definitions/qos-profiles.yaml Show resolved Hide resolved
@hdamker
Copy link
Collaborator

hdamker commented Jul 29, 2024

@RandyLevensalor added some of the changes which I applied in #326 to quality-on-demand here as comments.

@hdamker
Copy link
Collaborator

hdamker commented Aug 1, 2024

@RandyLevensalor seems that there weren't further comments. Will you address my comments?

Co-authored-by: Herbert Damker <herbert.damker@telekom.de>
@RandyLevensalor
Copy link
Collaborator Author

@RandyLevensalor seems that there weren't further comments. Will you address my comments?

@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.

@hdamker
Copy link
Collaborator

hdamker commented Aug 1, 2024

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.

@RandyLevensalor
Copy link
Collaborator Author

@hdamker I believe that your comments have been resolved.

@hdamker
Copy link
Collaborator

hdamker commented Aug 1, 2024

@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 🤞

Copy link
Collaborator

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RandyLevensalor
Copy link
Collaborator Author

@camaraproject/quality-on-demand_codeowners are we ready to merge this?

@hdamker
Copy link
Collaborator

hdamker commented Aug 5, 2024

@RandyLevensalor: I would like to merge first #326 ... in best case there are no conflicts but if yes, the a rebase might be needed.

@RandyLevensalor
Copy link
Collaborator Author

@hdamker it doesn't show any merge conflicts

@hdamker
Copy link
Collaborator

hdamker commented Aug 5, 2024

@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:

  • Split of Generic404 into Generic404 (with only "NOT_FOUND") and GenericDevice404 (with "NOT_FOUND" and "DEVICE_NOT_FOUND")
  • within the responses the POST endpoint with device: changing "Generic404" to "GenericDevice404", and adding 422 and 429 as valid responses (Generic422 and Generic429 are already there)

Copy link
Collaborator

@hdamker hdamker left a 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

@RandyLevensalor
Copy link
Collaborator Author

@hdamker Thanks for the feedback. Rebased, updated the device and added 422 and 429 as well.

@hdamker
Copy link
Collaborator

hdamker commented Aug 5, 2024

@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 Generic404 (otherwise are Generic404 and GenericDevice404 identical 😃).

hdamker
hdamker previously approved these changes Aug 5, 2024
Copy link
Collaborator

@hdamker hdamker left a 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.

code/API_definitions/qos-profiles.yaml Outdated Show resolved Hide resolved
Co-authored-by: Herbert Damker <herbert.damker@telekom.de>
@RandyLevensalor RandyLevensalor merged commit 243b34c into camaraproject:main Aug 6, 2024
1 check passed
hdamker added a commit to hdamker/QualityOnDemand that referenced this pull request Aug 7, 2024
Added changelog entries for PR camaraproject#318
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend QoS Profile queries to list profiles based on specific users or devices
4 participants