search

camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family
https://wiki.camaraproject.org/x/zwOeAQ
Apache License 2.0
42 stars 59 forks source link

Introduce 'qosStatus' and corresponding notification event to fix issue #38 #67

Closed emil-cheung closed 1 year ago

emil-cheung commented 1 year ago

A proposal to fix: https://github.com/camaraproject/QualityOnDemand/issues/38

emil-cheung commented 1 year ago

The deck I presented in the WG meeting associated to this PR: QoS Status and Corresponding Notification Events

emil-cheung commented 1 year ago

To the reviewers who haven't conduct the review, could you please review this PR at your earliest convenience so that we can move forward.

Thanks :)

jpengar commented 1 year ago

To the reviewers who haven't conduct the review, could you please review this PR at your earliest convenience so that we can move forward. Thanks :)

@emil-cheung I see you have already resolved all comments from last @jlurien review. But actually, some of them were open to discussion. In any case, from TEF side, we are internally reviewing this proposal with Product/Network teams and we will come back with the results of our analysis.

emil-cheung commented 1 year ago

Hello Reviewed with @patrice-conil

Few comments:

  • It will be probably helpful to get the session status in the notification/eventReport to avoid listener to do a GET
  • You have introduced use of discriminator for event report (line 357). We get the idea but do you think it's a bit premature as we have only one kind of event?

Thanks Ludovic/Patrice

For the point #1: 'qosStatus' is also in 'SessionInfo' datatype, which is also applicable to GET /sessions operation. For the point #2: somebody proposed to use discriminator previously. For me, I am open. Let our community to vote for it.

emil-cheung commented 1 year ago

To the reviewers who haven't conduct the review, could you please review this PR at your earliest convenience so that we can move forward. Thanks :)

@emil-cheung I see you have already resolved all comments from last @jlurien review. But actually, some of them were open to discussion. In any case, from TEF side, we are internally reviewing this proposal with Product/Network teams and we will come back with the results of our analysis.

Sure. Please free feel to reopen the comments if you still want to further discuss.

emil-cheung commented 1 year ago

@hdamker shall I rebase this PR to the main branch?

hdamker commented 1 year ago

shall I rebase this PR to the main branch?

@emil-cheung sorry for the late answer (thought I had done it already): yes, please rebase the PR to main branch as this is now the branch for the latest changes.

emil-cheung commented 1 year ago

shall I rebase this PR to the main branch?

@emil-cheung sorry for the late answer (thought I had done it already): yes, please rebase the PR to main branch as this is now the branch for the latest changes.

@hdamker I have changed the base branch from dev-0.9.0 to main, please check.

hdamker commented 1 year ago

I have changed the base branch from dev-0.9.0 to main, please check.

@emil-cheung Thanks a lot! LGTM, we are ready for a final review by those who have previously approved.

hdamker commented 1 year ago

@jlurien @jlurien @eric-murray Could you have a final view?