Update from notificationsUri to notificationsUrl

camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family

https://wiki.camaraproject.org/x/zwOeAQ

Apache License 2.0

41 stars 59 forks source link

Update from notificationsUri to notificationsUrl #89

Closed maxl2287 closed 1 year ago

maxl2287 commented 1 year ago

fixes #88

hdamker commented 1 year ago

LGTM, at least one further review from @eric-murray @jlurien or @patrice-conil would be good.

eric-murray commented 1 year ago

I had a couple of questions / comments on this PR:

Why are modifications being proposed for the "legacy" implementation within the main QualityOnDemand repository rather than moving the implementation to the new QualityOnDemand_PI1 repository? I had understood that implementations would no longer be stored within the main API repository.
A change is being proposed to a copy of v0.1.0 stored within the implementation, but no change is being proposed to the version number of that copy. This is potentially confusing if such files are viewed out of context. I'd prefer if modifications made for the purposes of an implementation have a suitably extended version number. I'd propose a company specific extension to the base version number (e.g. 0.1.0-dtag.1) when such changes are made. This is the approach that we took within Vodafone when modifying the API specification for our specific implementation (e.g. update of apiRoot and addition of OAuth scope).

jlurien commented 1 year ago

I'm OK with renaming parameter to notificationUrl, but I agree with @eric-murray. I think we should fix code/API_definitions/qod-api.yaml and doc here, and move implementations to the new repos.

We could upgrade yaml version to 0.8,1 and take advantage of the change to incorporate the other typos identified.

maxl2287 commented 1 year ago

@jlurien and @eric-murray thanks for your feedback. I will then revert the implementation changes and just change the qod-api.yaml.

hdamker commented 1 year ago

@maxl2287 please keep also the change in documentation. And I recommend not to touch the version number in the spec file until we decide to make a new release (like v0.8.1).

jlurien commented 1 year ago

@maxl2287 please keep also the change in documentation. And I recommend not to touch the version number in the spec file until we decide to make a new release (like v0.8.1).

@hdamker Should we resolve the release first and then apply this change towards 0.8.1?

maxl2287 commented 1 year ago

Branch is now updated without implementation changes

eric-murray commented 1 year ago

@maxl2287 please keep also the change in documentation. And I recommend not to touch the version number in the spec file until we decide to make a new release (like v0.8.1).

@hdamker Will you create the release v0.8.0 first before merging this PR? Although the change is small, our current implementation uses the current v0.8.0 and not this update, and anyone looking for v0.8.0 in the github should find the current version and not subsequent updates that have not incremented the version number.

hdamker commented 1 year ago

@eric-murray

Will you create the release v0.8.0 first before merging this PR?

Yes, will do today. I have exactly the same intention to get the v0.8.0 release before any further fix merged.

And, to answer the other comments: the v0.1.0 implementation code will not be part of the v0.8.0 release (deleted it from the repo already).

hdamker commented 1 year ago

@jlurien @patrice-conil can you approve (again)? Then we should be ready here.